Funkcje jednowierszowe, które są wywoływane tylko raz

120

Rozważ funkcję bez parametrów ( edytuj: niekoniecznie), która wykonuje pojedynczy wiersz kodu i jest wywoływana tylko raz w programie (choć nie jest niemożliwe, że będzie ona potrzebna ponownie w przyszłości).

Może wykonać zapytanie, sprawdzić niektóre wartości, zrobić coś z wyrażeniem regularnym ... wszystko niejasne lub „hacky”.

Uzasadnieniem tego byłoby uniknięcie trudnych do odczytania ocen:

if (getCondition()) {
    // do stuff
}

gdzie getCondition()jest funkcja jednowierszowa.

Moje pytanie brzmi: czy to dobra praktyka? Wydaje mi się to w porządku, ale nie wiem o długim okresie ...

vemv
źródło
9
Chyba że twój fragment kodu pochodzi z języka OOP z niejawnym odbiornikiem (np. To) z pewnością byłaby to zła praktyka, ponieważ getCondition () najprawdopodobniej opiera się na stanie globalnym ...
Ingo
2
Mógł sugerować, że getCondition () może mieć argumenty?
user606723,
24
@Ingo - niektóre rzeczy naprawdę mają stan globalny. Aktualny czas, nazwy hostów, numery portów itp. Są poprawnymi „Globalnymi”. Błąd projektowy polega na tworzeniu globalnego z czegoś, co z natury nie jest globalne.
James Anderson
1
Dlaczego po prostu nie inline getCondition? Jeśli jest tak mały i rzadko używany, jak mówisz, to nadanie mu nazwy niczego nie osiąga.
davidk01
12
davidk01: Czytelność kodu.
wjl

Odpowiedzi:

240

Zależy od tej jednej linii. Jeśli wiersz jest czytelny i zwięzły sam w sobie, funkcja może nie być potrzebna. Prosty przykład:

void printNewLine() {
  System.out.println();
}

OTOH, jeśli funkcja nadaje dobre imię linii kodu zawierającej np. Złożone, trudne do odczytania wyrażenie, jest to dla mnie całkowicie uzasadnione. Przemyślany przykład (podzielony na wiele wierszy dla czytelności tutaj):

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}
Péter Török
źródło
99
+1. Magiczne słowo to „samodokumentujący się kod”.
Konamiman,
8
Dobry przykład tego, co wujek Bob nazwałby „enkapsulowaniem warunków”.
Anthony Pegram
12
@Aditya: taxPayerW tym scenariuszu nic nie mówi, że ma charakter globalny. Być może ta klasa jest TaxReturni taxPayerjest atrybutem.
Adam Robinson,
2
Dodatkowo pozwala ci udokumentować funkcję w sposób, który może być wybrany np. Przez javadoc i być publicznie widoczny.
2
@dallin, Clean Code Boba Martina pokazuje wiele przykładów kodu z pół-prawdziwego życia. Jeśli masz zbyt wiele funkcji w jednej klasie, może Twoja klasa jest za duża?
Péter Török,
67

Tak, można to wykorzystać w celu spełnienia najlepszych praktyk. Na przykład lepiej, aby funkcja o wyraźnej nazwie działała, nawet jeśli ma tylko jeden wiersz, niż mieć ten wiersz kodu w większej funkcji i potrzebować komentarza wyjaśniającego, co robi. Również sąsiednie wiersze kodu powinny wykonywać zadania na tym samym poziomie abstrakcji. Kontrprzykład byłby coś w rodzaju

startIgnition();
petrolFlag |= 0x006A;
engageChoke();

W takim przypadku zdecydowanie lepiej jest przesunąć środkową linię do rozsądnie nazwanej funkcji.

Kilian Foth
źródło
15
To 0x006A jest piękne: DA dobrze znana stała zwęglonego paliwa z dodatkami a, b i c.
Koder
2
+1 Jestem zwolennikiem samodokumentującego się kodu :) tak przy okazji, myślę, że zgadzam się na utrzymanie stałego poziomu abstrakcji przez bloki, ale nie mogłem wyjaśnić dlaczego. Czy mógłbyś to rozwinąć?
vemv
3
Jeśli mówisz to po prostu petrolFlag |= 0x006A;bez podejmowania decyzji, lepiej byłoby po prostu powiedzieć petrolFlag |= A_B_C; bez dodatkowej funkcji. Można przypuszczać, że engageChoke()należy go wywołać tylko wtedy, gdy petrolFlagspełnia określone kryteria, a to powinno wyraźnie powiedzieć „potrzebuję tutaj funkcji”. Tylko drobna nitka, ta odpowiedź jest w zasadzie na miejscu :)
Tim Post
9
+1 za wskazanie tego kodu w metodzie powinno być na tym samym poziomie abstrakcji. @vemv, jest to dobra praktyka, ponieważ ułatwia zrozumienie kodu, unikając potrzeby przeskakiwania między różnymi poziomami abstrakcji podczas czytania kodu. Wiązanie przełączników poziomu abstrakcji do wywołań / zwrotów metod (tj. Strukturalnych „skoków”) jest dobrym sposobem na uczynienie kodu bardziej płynnym i czystym.
Péter Török,
21
Nie, to całkowicie wymyślona wartość. Ale fakt, że się nad tym zastanawiasz, tylko podkreśla tę kwestię: gdyby kod powiedział mixLeadedGasoline(), nie musiałbyś!
Kilian Foth,
37

Myślę, że w wielu przypadkach taka funkcja jest dobrym stylem, ale możesz rozważyć lokalną zmienną logiczną jako alternatywę w przypadkach, gdy nie musisz używać tego warunku gdzieś w innych miejscach, np .:

bool someConditionSatisfied = [complex expression];

To da podpowiedź dla czytnika kodów i pozwoli zaoszczędzić na wprowadzeniu nowej funkcji.

cybevnm
źródło
1
Wartość bool jest szczególnie korzystna, jeśli nazwa funkcji warunku byłaby trudna lub być może wprowadzająca w błąd (np IsEngineReadyUnlessItIsOffOrBusyOrOutOfService.).
dbkk
2
Doświadczyłem tej porady złym pomysłem: bool i warunek odwracają uwagę od podstawowej działalności tej funkcji. Ponadto styl preferujący zmienne lokalne utrudnia refaktoryzację.
Wolf
1
@Wolf OTOH, wolę to od wywołania funkcji w celu zmniejszenia „głębokości abstrakcji” kodu. IMO, przeskakiwanie do funkcji jest większym przełącznikiem kontekstu niż jawna i bezpośrednia para wierszy kodu, szczególnie jeśli zwraca tylko wartość logiczną.
Kache
@Kache Myślę, że to zależy od tego, czy kodujesz obiektowo, czy nie, w przypadku OOP użycie funkcji członka utrzymuje elastyczność projektu. To zależy od kontekstu ...
Wolf,
23

Oprócz odpowiedzi Piotra , jeśli ten warunek może wymagać aktualizacji w pewnym momencie w przyszłości, poprzez włączenie go do sposobu, w jaki sugerujesz, że będziesz miał tylko jeden punkt edycji.

Idąc za przykładem Piotra, jeśli to

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

staje się tym

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isMutant() 
        && (taxPayer.getNumberOfThumbs() > 2 
        || (taxPayer.getAge() > 123 && taxPayer.getEmployer().isXMan()));
}

Wykonujesz jedną edycję, która jest aktualizowana uniwersalnie. Jeśli chodzi o łatwość konserwacji, jest to plus.

Pod względem wydajności najbardziej optymalizujące kompilatory usuwają wywołanie funkcji i wstawiają mały blok kodu. Optymalizacja czegoś takiego może faktycznie zmniejszyć rozmiar bloku (poprzez usunięcie instrukcji potrzebnych do wywołania funkcji, powrotu itp.), Więc zwykle jest to bezpieczne nawet w warunkach, które w przeciwnym razie mogłyby uniemożliwić wstawianie.

Stephen
źródło
2
Jestem już świadomy korzyści płynących z utrzymywania jednego punktu edycji - dlatego pytanie brzmi „... zadzwoniono raz”. Niemniej jednak wspaniale jest wiedzieć o tych optymalizacjach kompilatora, zawsze myślałem, że postępują zgodnie z tego rodzaju instrukcjami dosłownie.
vemv
Podział metody na testowanie prostego warunku sugeruje, że warunek można zmienić przez zmianę metody. Taka implikacja jest przydatna, gdy jest prawdą, ale może być niebezpieczna, gdy nie jest. Załóżmy na przykład, że kod musi dzielić rzeczy na lekkie, ciężkie zielone i ciężkie niebiałe. Obiekty mają szybką właściwość „wydaje się zieloną”, która jest niezawodna w przypadku ciężkich obiektów, ale może zwracać fałszywe alarmy w przypadku lekkich obiektów; mają również funkcję „MeasureSpectralResponse”, która działa wolno, ale będzie działać niezawodnie dla wszystkich obiektów.
supercat
Fakt, że seemsGreenbędzie niewiarygodny w przypadku lekkich obiektów, nie będzie miał znaczenia, jeśli kod nigdy nie będzie obchodził, czy lekkie obiekty są zielone. Jeśli jednak definicja „lekkiej” ulegnie zmianie, tak że niektóre niezielone obiekty, które zwracają wartość true, seemsGreennie są zgłaszane jako „lekkiej”, wówczas taka zmiana w definicji „lekkiej” może złamać kod testujący obiekty byc zielonym". W niektórych przypadkach jednoczesne testowanie zieloności i wagi w kodzie może sprawić, że związek między testami będzie wyraźniejszy niż w przypadku oddzielnych metod.
supercat
5

Oprócz czytelności (lub jej uzupełnienia) pozwala to pisać funkcje na odpowiednim poziomie abstrakcji.

tylermac
źródło
1
Obawiam się, że nie rozumiem, co miałeś na myśli ...
vemv
1
@vemv: Myślę, że rozumie przez „odpowiedni poziom abstrakcji”, że nie kończy się to na kodzie, który ma różne poziomy abstrakcji mieszane (tj. co powiedział już Kilian Foth). Nie chcesz, aby Twój kod obsługiwał pozornie nieistotne szczegóły (np. Tworzenie się wszystkich wiązań w paliwie), gdy otaczający kod rozważa bardziej ogólny widok aktualnej sytuacji (np. Uruchomienie silnika). Pierwszy z nich zaśmieca drugi, co sprawia, że ​​kod jest mniej czytelny i utrzymywany, ponieważ będziesz musiał rozważyć wszystkie poziomy abstrakcji jednocześnie.
Egon
4

To zależy. Czasami lepiej jest zawrzeć wyrażenie w funkcji / metodzie, nawet jeśli jest to tylko jedna linia. Jeśli czytanie jest skomplikowane lub potrzebujesz go w wielu miejscach, uważam to za dobrą praktykę. W dłuższej perspektywie łatwiej jest utrzymać, ponieważ wprowadziłeś jeden punkt zmiany i lepszą czytelność .

Czasami jednak jest to po prostu coś, czego nie potrzebujesz. Jeśli mimo wszystko wyrażenie jest łatwe do odczytania i / lub pojawia się w jednym miejscu, nie zawijaj go.

Sokół
źródło
3

Myślę, że jeśli masz ich tylko kilka, to jest w porządku, ale problem pojawia się, gdy jest ich dużo w kodzie. A kiedy kompilator działa lub interpitor (w zależności od używanego języka) Przechodzi do tej funkcji w pamięci. Powiedzmy, że masz 3 z nich. Nie sądzę, aby komputer to zauważył, ale jeśli zaczniesz mieć 100 takich drobiazgów, system będzie musiał zarejestrować funkcje w pamięci, które są wywoływane tylko raz, a następnie nie są niszczone.

WojonsTech
źródło
Zgodnie z odpowiedzią Stephena to nie zawsze może się zdarzyć (chociaż i tak ślepe zaufanie w magię kompilatorów nie może być dobre)
vemv 13.09.11
1
tak, powinno się to wyjaśnić w zależności od wielu faktów. Jeśli jest to język zinterpretowany, system za każdym razem będzie działał w funkcjach jednowierszowych, chyba że zainstalujesz coś dla pamięci podręcznej, co wciąż może nie dać rady. Teraz, jeśli chodzi o kompilatory, to tylko w dniu słabości i umieszczeniu samolotów ważne jest, czy kompilator będzie twoim przyjacielem i usunie twój mały bałagan, czy też jeśli pomyśli, że naprawdę go potrzebujesz. Pamiętam, że jeśli znasz dokładną liczbę razy, pętla zawsze będzie działała lepiej, wystarczy skopiować i wkleić ją tyle razy, aby zapętlić.
WojonsTech,
+1 za wyrównanie planet :) ale twoje ostatnie zdanie brzmi dla mnie totalnie szalone, prawda?
vemv
To naprawdę zależy Przez większość czasu nie, nie robię, chyba że dostaję odpowiednią kwotę płatności, aby sprawdzić, czy to przyspiesza wciskanie i inne podobne rzeczy. Ale w starszych kompilatorach lepiej było skopiować i wkleić go, a następnie pozostawić kompilator, aby to rozgryźć 9/10.
WojonsTech
3

Niedawno zrobiłem dokładnie to samo w aplikacji, którą refaktoryzowałem, aby jasno określić rzeczywiste znaczenie kodu bez komentarzy:

protected void PaymentButton_Click(object sender, EventArgs e)
    Func<bool> HaveError = () => lblCreditCardError.Text == string.Empty && lblDisclaimer.Text == string.Empty;

    CheckInputs();

    if(HaveError())
        return;

    ...
}
joshperry
źródło
Ciekawe, co to za język?
vemv
5
@vemv: Wygląda mi na C #.
Scott Mitchell,
Wolę też dodatkowe identyfikatory niż komentarze. Ale czy tak naprawdę różni się to od wprowadzenia zmiennej lokalnej, aby była ifkrótka? To lokalne podejście (lambda) sprawia, że PaymentButton_Clickfunkcja (jako całość) jest trudniejsza do odczytania. W lblCreditCardErrortwoim przykładzie wydaje się być członkiem, więc HaveErrorjest także predykatem (prywatnym), który jest poprawny dla obiektu. Mam tendencję do głosowania za tym, ale nie jestem programistą C #, więc się opieram.
Wolf,
@Wolf Hej, stary. Napisałem to jakiś czas temu :) Zdecydowanie teraz robię rzeczy zupełnie inaczej. W rzeczywistości, patrząc na treść etykiety, aby zobaczyć, czy wystąpił błąd, robię się kulony ... Dlaczego nie zwróciłem bool z CheckInputs()???
joshperry
0

Przeniesienie tego wiersza do dobrze nazwanej metody ułatwia odczytanie kodu. Wiele innych już o tym wspomniało („kod samodokumentujący”). Inną zaletą przeniesienia go do metody jest to, że ułatwia testowanie jednostkowe. Kiedy jest izolowany we własnej metodzie i testowany jednostkowo, możesz być pewien, że jeśli / kiedy zostanie znaleziony błąd, nie będzie w tej metodzie.

Andrzej
źródło
0

Istnieje już wiele dobrych odpowiedzi, ale warto wspomnieć o specjalnym przypadku .

Jeśli instrukcja jednowierszowa wymaga komentarza i jesteś w stanie jednoznacznie zidentyfikować (co oznacza: nazwa) jej cel, rozważ wyodrębnienie funkcji przy jednoczesnym rozszerzeniu komentarza do dokumentu API. W ten sposób sprawisz, że wywołanie funkcji będzie szybsze i łatwiejsze do zrozumienia.

Co ciekawe, to samo można zrobić, jeśli obecnie nie ma nic do zrobienia, ale potrzebny jest komentarz przypominający o rozszerzeniach (w najbliższej przyszłości 1) ), więc to,

def sophisticatedHello():
    # todo set up
    say("hello")
    # todo tear down

równie dobrze mógł się w to zmienić

def sophisticatedHello():
    setUp()
    say("hello")
    tearDown()

1) powinieneś być tego pewien (patrz zasada YAGNI )

Wilk
źródło
0

Jeśli język to obsługuje, zwykle używam do tego celu anonimowych funkcji z etykietami.

someCondition = lambda p: True if [complicated expression involving p] else False
#I explicitly write the function with a ternary to make it clear this is a a predicate
if (someCondition(p)):
    #do stuff...

IMHO jest to dobry kompromis, ponieważ daje korzyść z czytelności, ponieważ nie ma skomplikowanego wyrażenia zaśmiecającego ifwarunek, jednocześnie unikając zaśmiecania przestrzeni nazw globalnych / pakietów małymi etykietami wyrzucania. Dodatkową zaletą jest to, że funkcja „definicja” jest odpowiednia tam, gdzie jest używana, co ułatwia modyfikację i odczyt definicji.

Nie muszą to być tylko funkcje predykatów. Lubię też umieszczać powtarzające się płytki kotła w małych funkcjach takich jak ta (działa szczególnie dobrze w przypadku generowania listy pythonowej bez zaśmiecania składni nawiasu). Na przykład poniższy uproszczony przykład podczas pracy z PIL w Pythonie

#goal - I have a list of PIL Image objects and I want them all as grayscale (uint8) numpy arrays
im_2_arr = lambda im: array(im.convert('L')) 
arr_list = [im_2_arr(image) for image in image_list]
poważny
źródło
Dlaczego „lambda p: prawda, jeśli [skomplikowane wyrażenie obejmujące p] jeszcze False” zamiast „lambda p: [skomplikowane wyrażenie obejmujące p]”? 8-)
Hans-Peter Störr,
@ hstoerr jest w komentarzu pod tym wierszem. Chciałbym wyraźnie powiedzieć, że my someCondition jest predykatem. Chociaż jest to absolutnie niepotrzebne, piszę wiele skryptów naukowych czytanych przez ludzi, którzy nie kodują zbyt wiele, osobiście uważam, że bardziej odpowiednie jest zachowanie większej zwięzłości niż mylenie moich kolegów, ponieważ nie wiedzą o tym [] == Falseani w żaden inny sposób podobna pythoniczna równoważność, która nie zawsze jest intuicyjna. Jest to w zasadzie sposób na oznaczenie, że someCondition jest w rzeczywistości predykatem.
crasic
Wystarczy wyczyścić oczywisty błąd, [] != Falseale []jest False jako kiedy rzucić do bool
crasic
@crasic: kiedy nie oczekuję od moich czytelników, że [] ocenia na False, wolę robić len([complicated expression producing a list]) == 0, niż używać, True if [blah] else Falseco nadal wymaga od czytelnika wiedzy, że [] to False.
Lie Ryan,