Czy definiowanie zmiennej w celu nazwania argumentu metody jest dobrą praktyką?

Ze względu na czytelność często zdarza mi się definiować zmienne tymczasowe podczas wywoływania funkcji, takich jak poniższy kod

var preventUndo = true;
doSomething(preventUndo);

Krótszą wersją tego byłoby:

doSomething(true);

Ale kiedy wracam do kodu, często zastanawiam się, o co chodzi true. Czy istnieje konwencja dla tego rodzaju zagadki?

Wyjaśnianie zmiennych

Twój przypadek jest przykładem wprowadzenia wyjaśniającego refaktoryzację zmiennych . Krótko mówiąc, zmienna objaśniająca to zmienna, która nie jest absolutnie niezbędna, ale pozwala nadać czemuś wyraźną nazwę w celu zwiększenia czytelności.

Kod dobrej jakości komunikuje intencje czytelnikowi; a jako profesjonalny programista czytelność i łatwość konserwacji są Twoimi najważniejszymi celami.

Dlatego zalecam następującą zasadę: jeśli cel twojego parametru nie jest od razu oczywisty, możesz użyć zmiennej, aby nadać jej dobrą nazwę. Myślę, że jest to ogólnie dobra praktyka (chyba że jest nadużywana). Oto szybki, wymyślony przykład - zastanów się:

editButton.Enabled = (_grid.SelectedRow != null && ((Person)_grid.SelectedRow).Status == PersonStatus.Active);

w porównaniu z nieco dłuższym, ale prawdopodobnie wyraźniejszym:

bool personIsSelected = (_grid.SelectedRow != null);
bool selectedPersonIsEditable = (personIsSelected && ((Person)_grid.SelectedRow).Status == PersonStatus.Active)
editButton.Enabled = (personIsSelected && selectedPersonIsEditable);

Parametry boolowskie

Twój przykład pokazuje, dlaczego booleany w interfejsach API są często złym pomysłem - po stronie wywołującej nic nie robią, aby wyjaśnić, co się dzieje. Rozważać:

ParseFolder(true, false);

Musisz sprawdzić, co oznaczają te parametry; gdyby były wyliczeniami, byłoby o wiele bardziej jasne:

ParseFolder(ParseBehaviour.Recursive, CompatibilityOption.Strict);

Edytować:

Dodano nagłówki i zamieniłem kolejność dwóch głównych akapitów, ponieważ zbyt wiele osób skupiało się na części z parametrami boolowskimi (szczerze mówiąc, był to pierwszy akapit). Dodano także przykład do pierwszej części.

Nie pisz kodu, którego nie potrzebujesz.

Jeśli doSomething(true)trudno ci zrozumieć, dodaj komentarz:

// Do something and prevent the undo.
doSomething(true);

lub, jeśli język to obsługuje, dodaj nazwę parametru:

doSomething(preventUndo: true);

W przeciwnym razie polegaj na swoim IDE, aby uzyskać podpis wywoływanej metody:

Przypadki, w których jest to przydatne

Przydanie dodatkowej zmiennej może być przydatne:

1. Do celów debugowania:

   var productId = this.Data.GetLastProductId();
   this.Data.AddToCart(productId);
   

   Ten sam kod można zapisać w jednym wierszu, ale jeśli chcesz dodać punkt przerwania przed dodaniem produktu do koszyka, aby sprawdzić, czy identyfikator produktu jest poprawny, dobrym pomysłem jest napisanie dwóch wierszy zamiast jednego.

2. Jeśli masz metodę z dużą ilością parametrów, a każdy parametr przechodzi od oceny. W jednym wierszu może to stać się całkowicie nieczytelne.

   // Even with indentation, this is unreadable.
   var doSomething(
       isSomethingElse ? 0 : this.getAValue(),
       this.getAnotherOne() ?? this.default,
       (a + b + c + d + e) * f,
       this.hello ? this.world : (this.hello2 ? this.world2 : -1));
   

3. Jeśli ocena parametru jest zbyt skomplikowana. Przykład kodu, który widziałem:

   // Wouldn't it be easier to have several if/else's (maybe even in a separate method)?
   do(something ? (hello ? world : -1) : (programmers ? stackexchange : (com ? -1 : 0)));
   

Dlaczego nie w innych przypadkach?

Dlaczego nie powinieneś tworzyć dodatkowych zmiennych w prostych przypadkach?

1. Nie z powodu wpływu na wydajność. Byłoby to bardzo błędne założenie początkującego programisty, który mikrooptymalizuje swoją aplikację. W większości języków nie ma wpływu na wydajność , ponieważ kompilator wstawi zmienną. W tych językach, w których kompilator tego nie robi, możesz uzyskać kilka mikrosekund, wkładając go ręcznie, co nie jest tego warte. Nie rób tego

2. Ale ze względu na ryzyko zdysocjowania nazwy, którą nadajesz zmiennej do nazwy parametru.

   Przykład:

   Powiedzmy, że oryginalny kod to:

   void doSomething(bool preventUndo)
   {
       // Does something very interesting.
       undoHistory.removeLast();
   }
   
   // Later in code:
   var preventUndo = true;
   doSomething(preventUndo);
   

   Później programista pracujący nad doSomethingpowiadomieniami, że ostatnio historia cofania wprowadziła dwie nowe metody:

   undoHistory.clearAll() { ... }
   undoHistory.disable() { ... }
   

   Teraz preventUndowydaje się niezbyt jasne. Czy to oznacza, że ​​zapobiegnie się tylko ostatniej czynności? A może oznacza to, że użytkownik nie będzie już mógł korzystać z funkcji cofania? A może historia cofania zostanie wyczyszczona? Jaśniejsze nazwy to:

   doSomething(bool hideLastUndo) { ... }
   doSomething(bool removeAllUndo) { ... }
   doSomething(bool disableUndoFeature) { ... }
   

   Więc teraz masz:

   void doSomething(bool hideLastUndo)
   {
       // Does something very interesting.
       undoHistory.removeLast();
   }
   
   // Later in code, very error prone, while the signature of the method is clear:
   var preventUndo = true;
   doSomething(preventUndo);
   

Co z wyliczeniami?

Niektóre inne osoby sugerowały użycie wyliczeń. Chociaż rozwiązuje natychmiastowy problem, tworzy większy. Spójrzmy:

enum undoPrevention
{
    keepInHistory,
    prevent,
}

void doSomething(undoPrevention preventUndo)
{
    doTheJob();
    if (preventUndo == undoPrevention.prevent)
    {
        this.undoHistory.discardLastEntry();
    }
}

doSomething(undoPrevention.prevent);

Problemy z tym:

1. To za dużo kodu. if (preventUndo == undoPrevention.prevent)? Poważnie?! Nie chcę pisać za ifkażdym razem.

2. Dodanie elementu do wyliczenia jest później bardzo kuszące, jeśli używam tego samego wyliczenia gdzie indziej. Co jeśli zmodyfikuję to w ten sposób:

   enum undoPrevention
   {
       keepInHistory,
       prevent,
       keepButDisable, // Keeps the entry in the history, but makes it disabled.
   }
   

   Co się teraz stanie? Czy doSomethingmetoda będzie działać zgodnie z oczekiwaniami? Aby temu zapobiec, od początku wymagałoby zapisania tej metody:

   void doSomething(undoPrevention preventUndo)
   {
       if (![undoPrevention.keepInHistory, undoPrevention.prevent].contains(preventUndo))
       {
           throw new ArgumentException('preventUndo');
       }
   
       doTheJob();
       if (preventUndo == undoPrevention.prevent)
       {
           this.undoHistory.discardLastEntry();
       }
   }
   

   Wariant wykorzystujący booleany zaczyna wyglądać tak ładnie!

   void doSomething(bool preventUndo)
   {
       doTheJob();
       if (preventUndo)
       {
           this.undoHistory.discardLastEntry();
       }
   }
   

Nie powinieneś także pisać metod z flagami logicznymi.

Cytując Roberta C. Martina w swojej książce Clean Code (ISBN-13 978-0-13-235088-4): „Przekazywanie funkcji logicznej do funkcji jest naprawdę okropną praktyką”.

Parafrazując go, rozumowanie jest takie, że fakt, że masz przełącznik prawda / fałsz, oznacza, że ​​twoja metoda najprawdopodobniej robi dwie różne rzeczy (tj. „Zrób coś z cofnięciem” i „Zrób coś bez cofnięcia”) i dlatego powinna być podzielone na dwie różne metody (które mogą wewnętrznie wywoływać to samo).

DoSomething()
{...

DoSomethingUndoable()
{....

Gdy spojrzysz na dwie klasy, aby zobaczyć, jak są ze sobą powiązane, jedną z kategorii jest łączenie danych , które odnosi się do kodu w jednej klasie wywołującego metody innej klasy i po prostu przekazującego dane, na przykład za jaki miesiąc chcesz raport, i inną kategoria to sprzężenie sterujące , wywoływanie metod i przekazywanie czegoś, co kontroluje zachowanie metody. Przykład, którego używam na zajęciach, to verboseflaga lub reportTypeflaga, ale preventUndotakże świetny przykład. Jak właśnie wykazałeś, sprzężenie sterujące utrudnia odczyt kodu wywoławczego i zrozumienie, co się dzieje. Naraża również kod wywołujący na zmiany, doSomething()które używają tej samej flagi do kontrolowania zarówno cofania, jak i archiwizowania lub dodawania innego parametrudoSomething() kontrolować archiwizację, w ten sposób psując kod i tak dalej.

Problem polega na tym, że kod jest zbyt ściśle powiązany. Przekazywanie wartości logicznej w celu kontrolowania zachowania jest, moim zdaniem, oznaką złego interfejsu API. Jeśli posiadasz interfejs API, zmień go. Dwie metody doSomething()i doSomethingWithUndo()byłoby lepiej. jeśli nie jesteś właścicielem, napisz dwie metody otoki w kodzie, który posiadasz, i jedną z nich wywołaj, doSomething(true)a drugą wywołaj doSomething(false).

Określenie roli argumentów nie jest zadaniem osoby wywołującej. Zawsze wybieram wersję wbudowaną i sprawdzam podpis wywoływanej funkcji, jeśli mam wątpliwości.

Zmienna powinna być nazwana na podstawie ich roli w bieżącym zakresie. Nie Zakres, w którym są wysyłane.

W JavaScripcie chciałbym, aby funkcja przyjmowała obiekt jako jedyny parametr mniej więcej taki:

function doSomething(settings) {
    var preventUndo = settings.hasOwnProperty('preventUndo') ? settings.preventUndo : false;
    // Deal with preventUndo in the normal way.
}

Następnie zadzwoń za pomocą:

doSomething({preventUndo: true});

Uwielbiam wykorzystywać funkcje językowe, aby poprawić sobie przejrzystość. Na przykład w C # można określić parametry według nazwy:

 CallSomething(name: "So and so", age: 12, description: "A random person.");

W JavaScript zazwyczaj wolę użyć obiektu opcji jako argumentu z tego powodu:

function doSomething(args) { /*...*/ }

doSomething({ name: 'So and so', age: 12, description: 'A random person.' });

Ale to tylko moje preferencje. Przypuszczam, że będzie to w dużej mierze zależeć od wywoływanej metody i tego, w jaki sposób IDE pomaga deweloperowi zrozumieć podpis.

Uważam, że jest to najprostszy i najłatwiejszy do odczytania:

enum Undo { ALLOW, PREVENT }

doSomething(Undo u) {
    if (Undo.ALLOW == u) {
        // save stuff to undo later.
    }
    // do your thing
}

doSomething(Undo.ALLOW);

MainMa tego nie lubi. Być może będziemy musieli zgodzić się nie zgadzać lub możemy skorzystać z rozwiązania, które proponuje Joshua Bloch:

enum Undo {
    ALLOW {
        @Override
        public void createUndoBuffer() {
            // put undo code here
        }
    },
    PREVENT {
        @Override
        public void createUndoBuffer() {
            // do nothing
        }
    };

    public abstract void createUndoBuffer();
}

doSomething(Undo u) {
    u.createUndoBuffer();
    // do your thing
}

Teraz, jeśli kiedykolwiek dodasz Undo.LIMITED_UNDO, Twój kod nie zostanie skompilowany, chyba że zaimplementujesz metodę createUndoBuffer (). A w doSomething () nie ma opcji if (Undo.ALLOW == u). Zrobiłem to na dwa sposoby, a druga metoda jest dość ciężka i trochę trudna do zrozumienia, gdy wyliczanie Undo rozszerza się na strony i strony kodu, ale sprawia, że ​​myślisz. Zwykle trzymam się prostszej metody, aby zastąpić prosty boolean wyliczeniem 2-wartościowym, dopóki nie mam powodu, aby to zmienić. Kiedy dodam trzecią wartość, używam mojego IDE do „Znajdź-zastosowania” i wtedy wszystko naprawiam.

Myślę, że twoje rozwiązanie sprawia, że ​​twój kod jest trochę bardziej czytelny, ale unikałbym definiowania dodatkowej zmiennej, aby wyjaśnić twoje wywołania funkcji. Ponadto, jeśli chcesz użyć dodatkowej zmiennej, oznaczę ją jako stałą, jeśli Twój język programowania ją obsługuje.

Mogę wymyślić dwie alternatywy, które nie wymagają dodatkowej zmiennej:

1. Użyj dodatkowego komentarza

doSomething(/* preventUndo */ true);

2. Użyj dwumocnego wyliczenia zamiast wartości logicznej

enum Options
{
    PreventUndo,
    ...
}

...

doSomething(PreventUndo);

Możesz użyć alternatywy 2, jeśli twój język obsługuje wyliczenia.

EDYTOWAĆ

Oczywiście użycie nazwanych argumentów jest również opcją, jeśli twój język je obsługuje.

Jeśli chodzi o dodatkowe kodowanie wymagane przez wyliczenia, wydaje mi się to naprawdę nieistotne. Zamiast

if (preventUndo)
{
    ...
}

ty masz

if (undoOption == PreventUndo)
{
    ...
}

lub

switch (undoOption)
{
  case PreventUndo:
  ...
}

I nawet jeśli jest to trochę więcej pisania, pamiętaj, że kod jest pisany raz i czytany wiele razy, więc warto teraz pisać trochę więcej, aby znaleźć bardziej czytelny kod sześć miesięcy później.

Zgadzam się z tym, co powiedział @GlenPeterson:

doSomething(Undo.ALLOW); // call using a self evident enum

ale również interesujące byłyby następujące, ponieważ wydaje mi się, że istnieją tylko dwie możliwości (prawda lub fałsz)

//Two methods    

doSomething()  // this method does something but doesn't prevent undo

doSomethingPreventUndo() // this method does something and prevents undo

Ponieważ pytasz głównie o javascript, używając coffeescript możesz mieć nazwany argument, taki jak składnia, super łatwe:

# declare method, ={} declares a default value to prevent null reference errors
doSomething = ({preventUndo} = {}) ->
  if preventUndo
    undo = no
  else
    undo = yes

#call method
doSomething preventUndo: yes
#or 
doSomething(preventUndo: yes)

kompiluje się do

var doSomething;

doSomething = function(_arg) {
  var preventUndo, undo;
  preventUndo = (_arg != null ? _arg : {}).preventUndo;
  if (preventUndo) {
    return undo = false;
  } else {
    return undo = true;
  }
};

doSomething({
  preventUndo: true
});

doSomething({
  preventUndo: true
});

Baw się dobrze na http://js2coffee.org/, aby wypróbować możliwości

Tylko dla mniej konwencjonalnego rozwiązania, a ponieważ OP wspomniał o Javascript, istnieje możliwość użycia tablicy asocjacyjnej do mapowania wartości. Przewaga nad pojedynczą wartością logiczną polega na tym, że możesz mieć tyle argumentów, ile chcesz i nie martwić się o ich kolejność.

var doSomethingOptions = new Array();

doSomethingOptions["PREVENTUNDO"] = true;
doSomethingOptions["TESTMODE"] = false;

doSomething( doSomethingOptions );

// ...

function doSomething( doSomethingOptions ){
    // Check for null here
    if( doSomethingOptions["PREVENTUNDO"] ) // ...
}

Zdaję sobie sprawę, że jest to odruch kogoś z silnie napisanego tła i może nie jest tak praktyczny, ale rozważ to z oryginalnością.

Inną możliwością jest posiadanie obiektu i jest to również możliwe w JavaScript. Nie jestem w 100% pewien, że jest to poprawna składnia. Zapoznaj się z wzorami, takimi jak Factory, aby uzyskać dalsze inspiracje.

function SomethingDoer( preventUndo ) {
    this.preventUndo = preventUndo;
    this.doSomething = function() {
            if( this.preventUndo ){
                    // ...
            }
    };
}

mySomethingDoer = new SomethingDoer(true).doSomething();

Głównym celem twojego pytania i innych odpowiedzi jest poprawa czytelności tam, gdzie wywoływana jest funkcja, co jest przedmiotem, z którym się zgadzam. Wszelkie szczegółowe wytyczne, wydarzenie „bez argumentów logicznych”, powinny zawsze funkcjonować jako środki do osiągnięcia tego celu, a nie same stawać się i kończyć.

Myślę, że warto zauważyć, że ten problem można w większości uniknąć, gdy język programowania obsługuje nazwane argumenty / argumenty słów kluczowych, jak w C # 4 i Python, lub gdy argumenty metody są przeplatane w nazwie metody, jak w Smalltalk lub Objective-C.

Przykłady:

// C# 4
foo.doSomething(preventUndo: true);

# Python
foo.doSomething(preventUndo=True)

// Objective-C
[foo doSomethingWith:bar shouldPreventUndo:YES];

Należy unikać przekazywania zmiennych boolowskich jako argumentów do funkcji. Ponieważ funkcje powinny robić jedną rzecz na raz. Przekazując zmienną logiczną funkcja ma dwa zachowania. Powoduje to także problem z czytelnością na późniejszym etapie lub dla innych programistów, którzy mają tendencję do zobaczenia twojego kodu. Stwarza to również problem podczas testowania funkcji. Prawdopodobnie w tym przypadku musisz utworzyć dwa przypadki testowe. Wyobraź sobie, że jeśli masz funkcję, która ma instrukcję switch i zmienia swoje zachowanie w zależności od typu przełącznika, musisz zdefiniować dla niej tak wiele różnych przypadków testowych.

Ilekroć ktoś napotka argument logiczny dla funkcji, musi tak dostosować kod, aby w ogóle nie zapisywał argumentu logicznego.

int preventUndo = true;
doSomething(preventUndo);

Dobry kompilator dla języków słabo rozwiniętych, takich jak C ++, zoptymalizuje kod maszynowy powyżej 2 linii, jak poniżej:

doSomething(true); 

więc nie ma znaczenia wydajność, ale zwiększy czytelność.

Pomocne jest również użycie zmiennej, na wypadek, gdyby stała się zmienna później i może zaakceptować również inne wartości.