Dodatkowa linia w bloku vs dodatkowy parametr w Clean Code

33

Kontekst

W Clean Code , strona 35, to mówi

Oznacza to, że bloki w instrukcjach if, instrukcjach else, instrukcjach while i tak dalej powinny mieć długość jednego wiersza. Prawdopodobnie ta linia powinna być wywołaniem funkcji. To nie tylko sprawia, że ​​funkcja zamykająca jest mała, ale także dodaje wartość dokumentalną, ponieważ funkcja wywoływana w bloku może mieć ładnie opisową nazwę.

Zgadzam się całkowicie, to ma sens.

Później, na stronie 40, mówi o argumentach funkcji

Idealna liczba argumentów dla funkcji wynosi zero (niladic). Dalej jest jeden (monadyczny), a następnie dwa (dyadyczny). Tam, gdzie to możliwe, należy unikać trzech argumentów (triadycznych). Więcej niż trzy (poliady) wymagają bardzo specjalnego uzasadnienia - i i tak nie powinny być używane. Argumenty są trudne. Biorą dużo mocy pojęciowej.

Zgadzam się całkowicie, to ma sens.

Kwestia

Jednak często zdarza mi się tworzyć listę z innej listy i będę musiał żyć z jednym z dwóch złych.

Albo używam dwóch wierszy w bloku , jeden do stworzenia rzeczy, jeden do dodania go do wyniku:

    public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            Flurp flurp = CreateFlurp(badaBoom);
            flurps.Add(flurp);
        }
        return flurps;
    }

Lub dodaję argument do funkcji listy, do której rzecz zostanie dodana, co czyni go „jednym argumentem gorszym”.

    public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            CreateFlurpInList(badaBoom, flurps);
        }
        return flurps;
    }

Pytanie

Czy są jakieś (nie) zalety, których nie widzę, które sprawiają, że jedna z nich jest ogólnie lepsza? Czy są takie zalety w niektórych sytuacjach; w takim przypadku, na co powinienem zwrócić uwagę przy podejmowaniu decyzji?

R. Schmitz
źródło
58
Co jest nie tak z flurps.Add(CreateFlurp(badaBoom));?
cmaster
47
Nie, to tylko jedno stwierdzenie. To tylko trywialnie zagnieżdżone wyrażenie (pojedynczy poziom zagnieżdżony). A jeśli proste f(g(x))jest sprzeczne z twoim przewodnikiem po stylu, cóż, nie mogę naprawić twojego przewodnika po stylu. To znaczy, ty też nie dzielisz się sqrt(x*x + y*y)na cztery linie, prawda? I to trzy (!) Zagnieżdżone podwyrażenia na dwóch (!) Wewnętrznych poziomach zagnieżdżenia (wstrzymanie oddechu!). Twoim celem powinna być czytelność , a nie oświadczenia jednego operatora. Jeśli chcesz później, cóż, mam dla Ciebie idealny język: asembler.
cmaster
6
@cmaster Nawet zestaw x86 nie ma ściśle instrukcji jednego operatora. Tryby adresowania pamięci obejmują wiele skomplikowanych operacji i mogą być używane do arytmetyki - w rzeczywistości można wykonać komputer z funkcją Turinga, używając tylko movinstrukcji x86 i jednego jmp toStartna końcu. Ktoś faktycznie stworzył kompilator, który robi dokładnie to: D
Luaan
5
@Luaan Nie mówiąc już o niesławnej rlwimiinstrukcji dotyczącej PPC. (To znaczy Obróć lewe słowo Natychmiastowe wstawienie maski). Polecenie to zajęło nie mniej niż pięć operandów (dwa rejestry i trzy bezpośrednie wartości) i wykonało następujące operacje: Zawartość jednego rejestru została obrócona przez natychmiastowe przesunięcie, maska ​​była utworzony za pomocą pojedynczego przebiegu 1 bitów, który był kontrolowany przez dwa inne bezpośrednie argumenty, a bity odpowiadające 1 bitowi w tej masce w drugim argumencie rejestru zostały zastąpione odpowiednimi bitami obróconego rejestru. Bardzo fajna instrukcja :-)
cmaster
7
@ R. Schchitz „Robię programowanie ogólnego przeznaczenia” - właściwie nie, nie, robisz programowanie w określonym celu (nie wiem w jakim celu, ale zakładam, że tak ;-). Istnieją dosłownie tysiące celów programowania, a optymalne style kodowania są różne - więc to, co jest odpowiednie dla ciebie, może nie być odpowiednie dla innych i odwrotnie: Często porady tutaj są absolutne („ zawsze rób X; Y jest złe „itp.) ignorowanie tego, że w niektórych domenach trzymanie się jest całkowicie niepraktyczne. Dlatego rada w książkach takich jak Clean Code powinna być zawsze przyjmowana ze szczyptą (praktycznej) soli :)
psmears

Odpowiedzi:

104

Te wytyczne to kompas, a nie mapa. Wskazują ci rozsądny kierunek . Ale tak naprawdę nie mogą powiedzieć ci absolutnie, które rozwiązanie jest „najlepsze”. W pewnym momencie musisz przestać iść w kierunku wskazywanym przez kompas, ponieważ dotarłeś do celu.

Clean Code zachęca do dzielenia kodu na bardzo małe, oczywiste bloki. To ogólnie dobry kierunek. Ale jeśli dojdziemy do skrajności (jak sugeruje dosłowna interpretacja cytowanej porady), wówczas podzielimy kod na bezużyteczne małe części. Nic tak naprawdę nic nie robi, wszystko tylko deleguje. Jest to zasadniczo inny rodzaj zaciemnienia kodu.

Twoim zadaniem jest zrównoważyć „mniejszy jest lepszy” z „zbyt mały jest bezużyteczny”. Zadaj sobie pytanie, które rozwiązanie jest prostsze. Dla mnie jest to oczywiście pierwsze rozwiązanie, ponieważ oczywiście tworzy listę. To dobrze rozumiany idiom. Można zrozumieć ten kod bez konieczności patrzenia na jeszcze jedną funkcję.

Jeśli można to zrobić lepiej, należy zauważyć, że „przekształcenie wszystkich elementów z listy na inną listę” jest powszechnym wzorcem, który często można wyabstrahować za pomocą map()operacji funkcjonalnej . Myślę, że w C # to się nazywa Select. Coś takiego:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    return badaBooms.Select(BadaBoom => CreateFlurp(badaBoom)).ToList();
}
amon
źródło
7
Kod jest nadal niepoprawny i bezcelowo wymyśla koło. Po co dzwonić, CreateFlurps(someList)skoro BCL już zapewnia someList.ConvertAll(CreateFlurp)?
Ben Voigt
44
@BenVoigt To pytanie dotyczy poziomu projektowania. Nie interesuje mnie dokładna składnia, zwłaszcza, że biała tablica nie ma kompilatora (a ostatnio napisałem C # w '09). Nie chodzi mi o to, że „pokazałem najlepszy możliwy kod”, ale „na marginesie, to wspólny wzór, który już został rozwiązany”. Linq jest jednym ze sposobów, aby to zrobić, ConvertWszystko, o czym wspominasz inny . Dziękujemy za zasugerowanie tej alternatywy.
amon
1
Twoja odpowiedź jest rozsądna, ale fakt, że LINQ abstrahuje logikę i ogranicza stwierdzenie do jednej linii, wydaje się być sprzeczny z twoją radą. Na marginesie, BadaBoom => CreateFlurp(badaBoom)jest zbędny; możesz przekazać CreateFlurpjako funkcję bezpośrednio ( Select(CreateFlurp)). (O ile mi wiadomo, zawsze tak było).
jpmc26
2
Zauważ, że całkowicie eliminuje to potrzebę tej metody. Nazwa CreateFlurpsjest w rzeczywistości bardziej myląca i trudniejsza do zrozumienia niż tylko widzenie badaBooms.Select(CreateFlurp). Ta ostatnia jest całkowicie deklaratywna - nie ma problemu z rozkładem, a zatem nie ma potrzeby stosowania żadnej metody.
Carl Leth,
1
@ R.Schmitz Nie jest trudno zrozumieć, ale jest trudniejszy do zrozumienia niż badaBooms.Select(CreateFlurp). Tworzysz metodę, aby jej nazwa (wysoki poziom) oznaczała jej implementację (niski poziom). W tym przypadku są na tym samym poziomie, więc aby dowiedzieć się dokładnie, co się dzieje, muszę po prostu przyjrzeć się metodzie (zamiast widzieć ją w linii). CreateFlurps(badaBooms)może mieć niespodzianki, ale badaBooms.Select(CreateFlurp)nie może. Jest także mylące, ponieważ błędnie prosi o Listzamiast IEnumerable.
Carl Leth,
61

Idealna liczba argumentów dla funkcji wynosi zero (niladic)

Nie! Idealna liczba argumentów dla funkcji to jeden. Jeśli wynosi zero, to gwarantujesz, że funkcja musi uzyskać dostęp do informacji zewnętrznych, aby móc wykonać akcję. „Wujek” Bob bardzo źle to zrozumiał.

Jeśli chodzi o kod, twój pierwszy przykład ma tylko dwa wiersze w bloku, ponieważ tworzysz zmienną lokalną w pierwszym wierszu. Usuń to zadanie, a postępujesz zgodnie z tymi wytycznymi dotyczącymi czystego kodu:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    List<Flurp> flurps = new List<Flurp>();
    foreach (BadaBoom badaBoom in badaBooms)
    {
        flurps.Add(CreateFlurp(badaBoom));
    }
    return flurps;
}

Ale to jest bardzo długi kod (C #). Po prostu zrób to jako:

IEnumerable<Flurp> CreateFlurps(IEnumerable<BadaBoom> badaBooms) =>
    from badaBoom in babaBooms select CreateFlurp(badaBoom);
David Arno
źródło
14
Funkcja z zerowymi argumentami ma sugerować, że obiekt zawiera wymagane dane, a nie że rzeczy istnieją w stanie globalnym poza obiektem.
Ryathal
19
@Ryathal, dwa punkty: (1) jeśli mówisz o metodach, to w przypadku większości (wszystkich?) Języków OO obiekt ten jest wywnioskowany (lub wyraźnie określony, w przypadku Pythona) jako pierwszy parametr. W Javie, C # itp. Wszystkie metody są funkcjami z co najmniej jednym parametrem. Kompilator po prostu ukrywa przed Tobą ten szczegół. (2) Nigdy nie wspominałem o „globalnym”. Stan obiektu jest na przykład zewnętrzny względem metody.
David Arno
17
Jestem całkiem pewien, że kiedy wujek Bob napisał „zero”, miał na myśli „zero (nie licząc tego)”.
Doc Brown
26
@DocBrown, prawdopodobnie ponieważ jest wielkim fanem mieszania stanu i funkcjonalności w obiektach, więc przez „funkcję” prawdopodobnie odnosi się konkretnie do metod. I nadal się z nim nie zgadzam. O wiele lepiej jest podać metodę, której potrzebuje, niż przeszukiwać obiekt, aby uzyskać to, czego chce (tj. Jest to klasyczny „powiedz, nie pytaj” w akcji).
David Arno
8
@AlessandroTeruzzi, Ideał to jeden parametr. Zero to za mało. Dlatego na przykład języki funkcjonalne przyjmują jeden jako liczbę parametrów do celów curry (w rzeczywistości w niektórych językach funkcjonalnych wszystkie funkcje mają dokładnie jeden parametr: nie więcej; nie mniej). Curry z zerowymi parametrami byłoby nonsensowne. Stwierdzenie, że „ideał jest jak najmniej, ergo zero jest najlepszy” jest przykładem reductio ad absurdum .
David Arno
19

Porada dotycząca „czystego kodu” jest całkowicie błędna.

Użyj dwóch lub więcej linii w swojej pętli. Ukrywanie tych samych dwóch linii w funkcji ma sens, gdy są to losowe matematyki, które wymagają opisu, ale nic nie robi, gdy linie są już opisowe. „Utwórz” i „Dodaj”

Druga metoda, o której wspominasz, nie ma żadnego sensu, ponieważ nie musisz dodawać drugiego argumentu, aby uniknąć dwóch linii.

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            flurps.Add(badaBoom .CreateFlurp());
            //or
            badaBoom.AddToListAsFlurp(flurps);
            //or
            flurps.Add(new Flurp(badaBoom));
            //or
            //make flurps a member of the class
            //use linq.Select()
            //etc
        }
        return flurps;
    }

lub

foreach(var flurp in ConvertToFlurps(badaBooms))...

Jak zauważyli inni, rada, że ​​najlepszą funkcją jest ta bez argumentów, jest w najlepszym razie wypaczona dla OOP, aw najgorszym przypadku zwykła zła rada

Ewan
źródło
Może chcesz edytować tę odpowiedź, aby była bardziej zrozumiała? Moje pytanie dotyczyło tego, czy jedna rzecz przeważa nad inną w ramach Czystego kodu. Mówisz, że wszystko jest źle, a następnie opisujesz jedną z opcji, które dałem. W tej chwili ta odpowiedź wygląda na to, że przestrzegasz agendy czyszczenia kodu zamiast próbować odpowiedzieć na pytanie.
R. Schmitz
przepraszam, zinterpretowałem twoje pytanie jako sugerujące, że pierwszy był „normalny”, ale zostałeś zepchnięty do drugiego. Ogólnie nie jestem przeciwny czystemu kodowi, ale ten cytat jest oczywiście błędny
Ewan
19
@ R. Schmitz Sam przeczytałem „Clean Code” i śledzę większość tego, co mówi ta książka. Jednak biorąc pod uwagę, że idealny rozmiar funkcji jest właściwie pojedynczą instrukcją, jest to po prostu błędne. Jedynym efektem jest to, że zamienia kod spaghetti w kod ryżu. Czytelnik jest zagubiony w wielu trywialnych funkcjach, które dają sensowne znaczenie, gdy są rozpatrywane razem. Ludzie mają ograniczoną pojemność pamięci roboczej i możesz albo przeciążać to instrukcjami, albo funkcjami. Musisz znaleźć równowagę między nimi, jeśli chcesz być czytelny. Unikaj skrajności!
cmaster
@cmaster Odpowiedź była tylko pierwszymi 2 akapitami, kiedy napisałem ten komentarz. To zdecydowanie lepsza odpowiedź.
R. Schmitz
7
szczerze mówiąc wolałem krótszą odpowiedź. W większości tych odpowiedzi jest za dużo rozmów dyplomatycznych. Przytoczona rada jest całkowicie błędna, nie trzeba spekulować na temat „co to tak naprawdę znaczy” ani przekręcać, aby znaleźć dobrą interpretację.
Ewan
15

Drugi jest zdecydowanie gorszy, ponieważ CreateFlurpInListakceptuje listę i modyfikuje tę listę, przez co funkcja nie jest czysta i trudniejsza do uzasadnienia. Nic w nazwie metody nie sugeruje, że metoda dodaje tylko do listy.

I oferuję trzecią, najlepszą opcję:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    return badaBooms.Select(CreateFlurp).ToList();
}

I do diabła, możesz natychmiast wprowadzić tę metodę, jeśli jest tylko jedno miejsce, w którym jest używana, ponieważ jednowarstwowa jest sama w sobie czysta, więc nie musi być enkapsulowana metodą, aby nadać jej znaczenie.

Euforyk
źródło
Nie narzekałbym tak bardzo na tę metodę „nie bycia czystą i trudniejszą do uzasadnienia” (choć jest to prawda), ale na to, że jest to całkowicie niepotrzebna metoda do obsługi specjalnego przypadku. Co jeśli chcę utworzyć samodzielny Flurp, Flurp dodany do tablicy, do słownika, Flurp, który następnie zostanie wyszukany w słowniku i usunięty pasujący Flurp itp.? Z tym samym argumentem kod Flurp również potrzebowałby wszystkich tych metod.
gnasher729,
10

Wersja z jednym argumentem jest lepsza, ale nie przede wszystkim ze względu na liczbę argumentów.

Najważniejszym powodem, dla którego lepiej jest to, że ma niższe sprzężenie , co czyni go bardziej użytecznym, łatwiejszym do przemyślenia, łatwiejszym do przetestowania i mniejszym prawdopodobieństwem przekształcenia się w kopiowanie + wklejanie klonów.

Jeśli dostarczyć mi CreateFlurp(BadaBoom), że mogę korzystać z każdego rodzaju pojemnika zbiorczego: Prosty Flurp[], List<Flurp>, LinkedList<Flurp>, Dictionary<Key, Flurp>, i tak dalej. Ale z CreateFlurpInList(BadaBoom, List<Flurp>), wracam do ciebie jutro z prośbą CreateFlurpInBindingList(BadaBoom, BindingList<Flurp>), aby mój model mógł uzyskać powiadomienie o zmianie listy. Fuj!

Dodatkową korzyścią jest to, że prostszy podpis bardziej pasuje do istniejących interfejsów API. Mówisz, że masz powtarzający się problem

raczej często zdarza mi się tworzyć listę z innej listy

To tylko kwestia użycia dostępnych narzędzi. Najkrótsza, najbardziej wydajna i najlepsza wersja to:

var Flurps = badaBooms.ConvertAll(CreateFlurp);

Jest to nie tylko mniej kodu do pisania i testowania, ale także szybsze, ponieważ List<T>.ConvertAll()jest wystarczająco inteligentny, aby wiedzieć, że wynik będzie miał taką samą liczbę elementów, jak dane wejściowe, i wstępnie przydzielić listę wyników do odpowiedniego rozmiaru. Podczas gdy twój kod (obie wersje) wymagał powiększenia listy.

Ben Voigt
źródło
Nie używać List.ConvertAll. Nazywany jest idiomatyczny sposób mapowania obiektów policzalnych na różne obiekty w języku C # Select. Jedyny powód ConvertAlljest tutaj dostępny nawet dlatego, że OP błędnie prosi o Listmetodę - powinna być IEnumerable.
Carl Leth
6

Pamiętaj o ogólnym celu: ułatwieniu odczytu i utrzymania kodu.

Często możliwe jest grupowanie wielu linii w jedną znaczącą funkcję. Zrób to w tych przypadkach. Czasami trzeba ponownie rozważyć swoje ogólne podejście.

Na przykład w twoim przypadku zastąpienie całej implementacji var

flups = badaBooms.Select(bb => new Flurp(bb));

może być możliwe. Lub możesz zrobić coś takiego

flups.Add(new Flurp(badaBoom))

Czasami najczystsze i najbardziej czytelne rozwiązanie po prostu nie mieści się w jednej linii. Więc będziesz miał dwie linie. Nie utrudniaj zrozumienia kodu, wystarczy spełnić dowolną regułę.

Twój drugi przykład jest (moim zdaniem) znacznie trudniejszy do zrozumienia niż pierwszy. Nie chodzi tylko o to, że masz drugi parametr, ale o to, że parametr jest modyfikowany przez funkcję. Sprawdź, co ma do powiedzenia Clean Code. (Nie mam w tej chwili książki pod ręką, ale jestem prawie pewien, że w zasadzie „nie rób tego, jeśli możesz tego uniknąć”).

podwójnie
źródło