Kiedy paradygmat „Do One Thing” staje się szkodliwy?

21

Dla celów argumentu oto przykładowa funkcja, która drukuje zawartość danego pliku linia po linii.

Wersja 1:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  string line;
  while (std::getline(file, line)) {
    cout << line << endl;
  }
}

Wiem, że zalecane jest, aby funkcje wykonywały jedną rzecz na jednym poziomie abstrakcji. Dla mnie powyższy kod robi prawie jedną rzecz i jest dość atomowy.

Niektóre książki (takie jak Czysty kod Roberta C. Martina) wydają się sugerować podzielenie powyższego kodu na osobne funkcje.

Wersja 2:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  printLines(file);
}

void printLines(fstream & file) {
  string line;
  while (std::getline(file, line)) {
    printLine(line);
  }
}

void printLine(const string & line) {
  cout << line << endl;
}

Rozumiem, co chcą osiągnąć (otwórz plik / przeczytaj linie / wydrukuj linię), ale czy nie jest to trochę przesada?

Oryginalna wersja jest prosta i w pewnym sensie już robi jedną rzecz - drukuje plik.

Druga wersja doprowadzi do dużej liczby naprawdę małych funkcji, które mogą być znacznie mniej czytelne niż pierwsza wersja.

Czy nie byłoby w tym przypadku lepiej mieć kod w jednym miejscu?

W którym momencie paradygmat „Do One Thing” staje się szkodliwy?

Petr
źródło
13
Ten rodzaj praktyki kodowania jest zawsze oparty na indywidualnych przypadkach. Nigdy nie ma jednego podejścia.
iammilind
1
@Alex - Akceptowana odpowiedź dosłownie nie ma nic wspólnego z pytaniem. Uważam to za bardzo dziwne.
ChaosPandion
2
Zwracam uwagę, że Twoja refaktoryzowana wersja jest odwrócona, co przyczynia się do braku czytelności. Czytanie w dół pliku, można oczekiwać, aby zobaczyć printFile, printLinesi wreszcie printLine.
Anthony Pegram
1
@ Kev, po raz kolejny mogę się nie zgodzić, szczególnie z tą kategoryzacją. To nie pedanteria, o to chodzi! To OP wyraźnie mówi, że druga wersja może nie być tak czytelna. Jest to OP, który konkretnie wymienia Clean Code jako inspirację dla drugiej wersji. Mój komentarz jest zasadniczo taki, że Clean Code nie kazałby mu tak pisać kodu. Kolejność jest tak naprawdę ważna dla czytelności, czytasz plik tak, jak czytasz artykuł w gazecie, zdobywając coraz więcej szczegółów, aż w zasadzie nie jesteś zainteresowany.
Anthony Pegram
1
Jak byś nie spodziewał się czytać wiersza wstecz, nie spodziewałbyś się, że najniższy poziom szczegółowości będzie pierwszą rzeczą w danej klasie. Do tego stopnia, że ten kod szybko zajmuje sortowanie, ale zakładam tylko, że ten kod nie jest jedynym kodem, który zamierza napisać. Moim zdaniem, jeśli ma zamiar zacytować Clean Code, przynajmniej mógł to zrobić . Jeśli kod jest niesprawny, z pewnością będzie mniej czytelny niż w innym przypadku.
Anthony Pegram

Odpowiedzi:

15

Oczywiście, to tylko nasuwa pytanie „Co to jest jedna rzecz?” Czy czytanie linii to jedno, a pisanie innego? Czy też kopiowanie wiersza z jednego strumienia do drugiego można uznać za jedną rzecz? Lub kopiowanie pliku?

Nie ma na to twardej, obiektywnej odpowiedzi. To zależy od Ciebie. Możesz zdecydować. Ty musisz zdecydować. Głównym celem paradygmatu „rób jedną rzecz” jest prawdopodobnie stworzenie kodu tak łatwego do zrozumienia, jak to tylko możliwe, abyś mógł użyć go jako wskazówki. Niestety, nie jest to również obiektywnie mierzalne, więc musisz polegać na swoich przeczuciach i „WTF?” liczyć w przeglądzie kodu .

IMO funkcja składająca się tylko z jednego wiersza kodu rzadko jest warta kłopotów. Nie printLine()ma przewagi nad bezpośrednim użyciem std::cout << line << '\n'1 . Jeśli widzę printLine(), muszę albo założyć, że robi to, co mówi jego nazwa, albo sprawdzić i sprawdzić. Jeśli widzę std::cout << line << '\n', od razu wiem, co robi, ponieważ jest to kanoniczny sposób wyprowadzania zawartości ciągu jako linii do std::cout.

Jednak kolejnym ważnym celem tego paradygmatu jest umożliwienie ponownego użycia kodu, a to jest o wiele bardziej obiektywny środek. Na przykład w drugiej wersji printLines() można łatwo napisać, aby był to uniwersalnie przydatny algorytm kopiujący linie z jednego strumienia do drugiego:

void copyLines(std::istream& is, std::ostream& os)
{
  std::string line;
  while( std::getline(is, line) );
    os << line << '\n';
  }
}

Taki algorytm mógłby zostać ponownie wykorzystany również w innych kontekstach.

Następnie możesz umieścić wszystko specyficzne dla tego jednego przypadku użycia w funkcji, która wywołuje ten ogólny algorytm:

void printFile(const std::string& filePath) {
  std::ifstream file(filePath.c_str());
  printLines(file, std::cout);
}

1 Zauważ, że użyłem '\n'raczej niż std::endl. '\n'powinien być domyślnym wyborem dla wypisywania nowego wiersza , std::endlto nieparzysty przypadek .

sbi
źródło
2
+1 - w większości się zgadzam, ale myślę, że jest w tym coś więcej niż „przeczucie”. Problem polega na tym, że ludzie oceniają „jedną rzecz”, licząc szczegóły implementacji. Dla mnie funkcja powinna implementować (a jej nazwa opisuje) jedną wyraźną abstrakcję. Nigdy nie powinieneś nazywać funkcji „do_x_i_y”. Implementacja może i powinna zrobić kilka (prostszych) rzeczy - a każda z tych prostszych rzeczy może zostać rozłożona na kilka jeszcze prostszych rzeczy i tak dalej. Jest to po prostu rozkład funkcjonalny z dodatkową zasadą - że funkcje (i ich nazwy) powinny opisywać jedno jasne pojęcie / zadanie / cokolwiek.
Steve314,
@ Steve314: Nie wymieniłem szczegółów implementacji jako możliwości. Kopiowanie wierszy z jednego strumienia do drugiego jasno jest jedna rzecz abstrakcji. Albo to jest? I łatwo jest tego uniknąć do_x_and_y(), nazywając funkcję do_everything(). Tak, to głupi przykład, ale pokazuje, że ta reguła nawet nie zapobiega najbardziej ekstremalnym przykładom złego projektu. IMO jest to decyzja odczuwana z jelit, tak jak decyzja podyktowana konwencjami. W przeciwnym razie, jeśli byłby obiektywny, mógłbyś wymyślić dla niego metrykę - czego nie możesz.
sbi
1
Nie miałem zamiaru zaprzeczać - tylko sugerować dodanie. Wydaje mi się, że zapomniałem powiedzieć, że z pytania, rozkład do printLineetc jest prawidłowy - każdy z nich jest pojedynczą abstrakcją - ale to nie znaczy, że jest konieczne. printFilejest już „jedną rzeczą”. Chociaż możesz to rozłożyć na trzy osobne abstrakcje niższego poziomu, nie musisz rozkładać się na każdym możliwym poziomie abstrakcji. Każda funkcja musi robić „jedną rzecz”, ale nie każda możliwa „jedna rzecz” musi być funkcją. Przenoszenie zbyt dużej złożoności do wykresu połączeń może samo w sobie stanowić problem.
Steve314,
7

Wykonywanie funkcji tylko „jedna rzecz” oznacza środek do dwóch pożądanych celów, a nie przykazanie od Boga:

  1. Jeśli twoja funkcja wykonuje tylko „jedną rzecz”, pomoże ci to uniknąć duplikacji kodu i rozdęcia API, ponieważ będziesz w stanie komponować funkcje w celu wykonania bardziej złożonych rzeczy zamiast kombinatorycznej eksplozji funkcji wyższego poziomu, mniej dających się skomponować .

  2. Posiadanie funkcji wykonujących tylko jedną rzecz może sprawić, że kod będzie bardziej czytelny. Zależy to od tego, czy zyskujesz większą jasność i łatwość rozumowania, oddzielając rzeczy, niż tracisz na szczegółowości, pośredniczości i narzutach konstrukcyjnych, które pozwalają ci oddzielić rzeczy.

Dlatego „jedna rzecz” jest nieuchronnie subiektywna i zależy od tego, jaki poziom abstrakcji jest istotny dla twojego programu. Jeśli printLinesjest uważany za pojedynczą, podstawową operację i jedyny sposób drukowania linii, na którym ci zależy lub na którą masz nadzieję, to printLinestylko dla jednej rzeczy. Chyba że druga wersja jest bardziej czytelna (ja nie), pierwsza wersja jest w porządku.

Jeśli zaczniesz potrzebować większej kontroli nad niższymi poziomami abstrakcji, a skończysz na subtelnym powielaniu i eksplozji kombinatorycznej (tj. printLinesDla nazw plików i całkowicie oddzielny printLinesdla fstreamobiektów, printLinesdla konsoli i printLinesdla plików), wtedy printLinesrobi więcej niż jedną rzecz na poziomie abstrakcji, na której ci zależy.

dsimcha
źródło
Dodałbym trzeci, a mianowicie, że mniejsze funkcje są łatwiej testowane. Ponieważ prawdopodobnie wymagana jest mniejsza liczba danych wejściowych, jeśli funkcja robi tylko jedną rzecz, ułatwia to niezależne testowanie.
PersonalNexus,
@PersonalNexus: W pewnym stopniu zgadzam się co do kwestii testowania, ale IMHO głupio jest testować szczegóły implementacji. Dla mnie test jednostkowy powinien przetestować „jedną rzecz”, jak zdefiniowano w mojej odpowiedzi. Wszystko, co jest bardziej szczegółowe, powoduje, że testy stają się kruche (ponieważ zmiana szczegółów implementacji będzie wymagać zmian w testach), a kod będzie denerwująco gadatliwy, pośredni itp. (Ponieważ będziesz dodawał pośrednie tylko w celu wsparcia testowania).
dsimcha
6

W tej skali to nie ma znaczenia. Implementacja jednofunkcyjna jest całkowicie oczywista i zrozumiała. Jednak dodanie nieco większej złożoności sprawia, że ​​bardzo atrakcyjne jest podzielenie iteracji od akcji. Załóżmy na przykład, że musisz wydrukować linie z zestawu plików określonego przez wzorzec taki jak „* .txt”. Następnie oddzieliłbym iterację od akcji:

printLines(FileSet files) {
   files.each({ 
       file -> file.eachLine({ 
           line -> printLine(line); 
       })
   })
}

Teraz iterację plików można osobno przetestować.

Dzielę funkcje, aby uprościć testowanie lub poprawić czytelność. Gdyby akcja wykonana na każdej linii danych była wystarczająco złożona, aby uzasadnić komentarz, z pewnością podzieliłbym go na osobną funkcję.

Kevin Cline
źródło
4
Myślę, że to przybiłeś. Jeśli potrzebujemy komentarza do wyjaśnienia linii, zawsze jest czas na wyodrębnienie metody.
Roger CS Wernersson
5

Wyodrębnij metody, gdy poczujesz potrzebę komentarza w celu wyjaśnienia rzeczy.

Napisz metody, które albo robią to, co mówi ich nazwa w oczywisty sposób, albo opowiedz historię, wywołując sprytnie nazwane metody.

Roger CS Wernersson
źródło
3

Nawet w twoim prostym przypadku brakuje Ci szczegółów, że zasada jednolitej odpowiedzialności pomogłaby ci lepiej zarządzać. Na przykład, co się dzieje, gdy coś pójdzie nie tak z otwarciem pliku. Dodanie obsługi wyjątków w celu zabezpieczenia przed przypadkami krawędzi dostępu do plików dodałoby 7-10 linii kodu do twojej funkcji.

Po otwarciu pliku nadal nie jesteś bezpieczny. Może być oderwany od ciebie (szczególnie jeśli jest to plik w sieci), możesz zabraknąć pamięci, ponownie może zdarzyć się kilka przypadków krawędzi, które chcesz zahartować i zepsuć swoją monolityczną funkcję.

Jednowarstwowa linia wydruku wydaje się wystarczająco niewinna. Ale wraz z dodawaniem nowych funkcji do drukarki plików (parsowanie i formatowanie tekstu, renderowanie na różnego rodzaju wyświetlaczach itp.) Będzie się powiększać, a później podziękujesz sobie.

Celem SRP jest umożliwienie ci myślenia o pojedynczym zadaniu na raz. To jak podzielenie dużego bloku tekstu na wiele akapitów, aby czytelnik mógł zrozumieć punkt, który próbujesz osiągnąć. Trochę więcej czasu zajmuje napisanie kodu zgodnego z tymi zasadami. Robiąc to, ułatwiamy czytanie tego kodu. Pomyśl, jak szczęśliwy będzie twój przyszły ja, gdy będzie musiał wyśledzić błąd w kodzie i znaleźć go porządnie podzielony na partycje.

Michael Brown
źródło
2
Poparłem tę odpowiedź, ponieważ podoba mi się logika, mimo że się z nią nie zgadzam! Zapewnienie struktury opartej na złożonym myśleniu o tym, co może się zdarzyć w przyszłości, przynosi efekt przeciwny do zamierzonego. Faktoryzuj kod, kiedy potrzebujesz. Nie abstraktuj rzeczy, dopóki nie musisz. Współczesny kod jest nękany przez ludzi, którzy próbują niewolniczo przestrzegać reguł zamiast pisać kod, który działa i niechętnie go dostosowuje . Dobrzy programiści są leniwi .
Yttrill
Dziękuję za komentarz. Uwaga: Nie opowiadam się za przedwczesną abstrakulacją, po prostu dzielę logiczne operacje, aby łatwiej było to zrobić później.
Michael Brown
2

Ja osobiście wolę to drugie podejście, ponieważ oszczędza ci to pracy w przyszłości i wymusza sposób myślenia „jak to zrobić w sposób ogólny”. Mimo to w twoim przypadku wersja 1 jest lepsza niż wersja 2 - tylko dlatego, że problemy rozwiązane przez wersję 2 są zbyt trywialne i specyficzne dla fstream. Myślę, że należy to zrobić w następujący sposób (w tym poprawkę błędu zaproponowaną przez Nawaza):

Ogólne funkcje narzędziowe:

void printLine(ostream& output, const string & line) { 
    output << line << endl; 
} 

void printLines(istream& input, ostream& output) { 
    string line; 
    while (getline(input, line)) {
        printLine(output, line); 
    } 
} 

Funkcja specyficzna dla domeny:

void printFile(const string & filePath, ostream& output = std::cout) { 
    fstream file(filePath, ios::in); 
    printLines(file, output); 
} 

Teraz printLinesi printLinemoże współpracować nie tylko fstreamz dowolnym strumieniem.

gwiazdorrr
źródło
2
Nie zgadzam się. Ta printLine()funkcja nie ma wartości. Zobacz moją odpowiedź .
sbi
1
Cóż, jeśli zachowamy printLine (), możemy dodać dekorator, który doda numery linii lub kolorowanie składni. Powiedziawszy to, nie wyciągnę tych metod, dopóki nie znajdę powodu.
Roger CS Wernersson 24.11.11
2

Każdy paradygmat (niekoniecznie ten, który zacytowałeś), którego należy przestrzegać, wymaga pewnej dyscypliny, a zatem ograniczenia „wolności słowa” - powoduje początkowy narzut (przynajmniej dlatego, że musisz się go nauczyć!). W tym sensie każdy paradygmat może stać się szkodliwy, gdy koszt tego narzutu nie zostanie nadmiernie zrekompensowany przez zaletę, jaką ten paradygmat zaprojektowano tak, aby był sam w sobie.

Prawdziwa odpowiedź na to pytanie wymaga zatem dobrej zdolności do „przewidywania” przyszłości, na przykład:

  • Jestem teraz zobowiązany do zrobienia AiB
  • Jakie jest prawdopodobieństwo, że w najbliższej przyszłości będę musiał to zrobić A-i B+(tj. Coś, co wygląda jak A i B, ale tylko trochę inaczej)?
  • Jakie jest prawdopodobieństwo, że w dalszej przyszłości A + stanie się A*lub A*-?

Jeśli to prawdopodobieństwo jest względnie wysokie, to będzie duża szansa, jeśli - myśląc jednocześnie o A i B - zastanowię się również nad ich możliwymi wariantami, aby w ten sposób wyodrębnić części wspólne, aby móc je ponownie wykorzystać.

Jeśli to prawdopodobieństwo jest bardzo niskie (jakikolwiek wariant wokół Ajest zasadniczo niczym więcej niż Asobą samym), przestudiuj, jak rozłożyć A, najprawdopodobniej spowoduje zmarnowany czas.

Jako przykład pozwól, że opowiem ci tę prawdziwą historię:

Podczas mojego poprzedniego życia jako nauczyciel, odkryłem, że -on większość projects- studenta praktycznie wszystkie z nich zapewniają własną funkcję, aby obliczyć długość łańcucha C .

Po pewnym dochodzeniu odkryłem, że będąc częstym problemem, wszyscy uczniowie wpadli na pomysł, aby użyć do tego funkcji. Po powiedzeniu im, że istnieje funkcja biblioteczna ( strlen), wielu z nich odpowiedziało, że ponieważ problem był tak prosty i trywialny, bardziej efektywne było napisanie własnej funkcji (2 linii kodu) niż szukanie podręcznika biblioteki C (to był 1984 rok, zapomniałem WEB i google!) w ścisłej kolejności alfabetycznej, aby sprawdzić, czy jest na to gotowa funkcja.

Jest to przykład, w którym również paradygmat „nie wymyślaj na nowo koła” może stać się szkodliwy bez skutecznego katalogu kół!

Emilio Garavaglia
źródło
2

Twój przykład jest odpowiedni do użycia w narzędziu do wyrzucania, które jest potrzebne wczoraj do wykonania określonego zadania. Lub jako narzędzie administracyjne, które jest bezpośrednio kontrolowane przez administratora. Teraz spraw, aby był solidny i odpowiedni dla Twoich klientów.

Dodaj odpowiednią obsługę błędów / wyjątków za pomocą znaczących komunikatów. Być może potrzebujesz weryfikacji parametrów, w tym decyzji, które należy podjąć, np. Jak obsługiwać nieistniejące pliki. Dodaj funkcję rejestrowania, być może z różnymi poziomami, takimi jak informacje i debugowanie. Dodaj komentarze, aby Twoi koledzy z zespołu wiedzieli, co się tam dzieje. Dodaj wszystkie części, które zwykle są pomijane ze względu na zwięzłość i pozostawione jako ćwiczenie dla czytelnika, podając przykłady kodu. Nie zapomnij o testach jednostkowych.

Twoja ładna i dość liniowa mała funkcja nagle kończy się złożonym bałaganem, który zaczyna być dzielony na osobne funkcje.

Bezpieczne
źródło
2

IMO staje się szkodliwe, gdy posuwa się tak daleko, że funkcja prawie nic nie robi, ale deleguje pracę do innej funkcji, ponieważ jest to znak, że nie jest to już abstrakcja niczego, a sposób myślenia prowadzący do takich funkcji zawsze jest zagrożony robić gorsze rzeczy ...

Z oryginalnego postu

void printLine(const string & line) {
  cout << line << endl;
}

Jeśli jesteś wystarczająco pedantyczny, możesz zauważyć, że printLine nadal robi dwie rzeczy: zapisuje linię do cout i dodaje znak „linii końcowej”. Niektóre osoby mogą chcieć sobie z tym poradzić, tworząc nowe funkcje:

void printLine(const string & line) {
  reallyPrintLine(line);
  addEndLine();
}

void reallyPrintLine(const string & line) {
  cout << line;
}

void addEndLine() {
  cout << endl;
}

O nie, teraz problem jeszcze się pogorszył! Teraz jest nawet oczywiste, że printLine robi DWIE rzeczy !!! 1! Nie jest głupotą tworzenie najbardziej absurdalnych „obejść”, jakie można sobie wyobrazić, aby pozbyć się tego nieuchronnego problemu polegającego na tym, że wydrukowanie linii polega na wydrukowaniu samej linii i dodaniu znaku końca linii.

void printLine(const string & line) {
  for (int i=0; i<2; i++)
    reallyPrintLine(line, i);
}

void reallyPrintLine(const string & line, int action) {
  cout << (action==0?line:endl);
}
użytkownik 281377
źródło
1

Krótka odpowiedź ... to zależy.

Pomyśl o tym: co jeśli w przyszłości nie będziesz chciał drukować tylko na standardowym wyjściu, ale na pliku.

Wiem, co to jest YAGNI, ale mówię tylko, że mogą istnieć przypadki, w których niektóre implementacje są potrzebne, ale odłożone. Więc może architekt lub cokolwiek wie, że ta funkcja musi być w stanie drukować również do pliku, ale nie chce teraz wykonać implementacji. Więc tworzy tę dodatkową funkcję, więc w przyszłości wystarczy zmienić wynik tylko w jednym miejscu. Ma sens?

Jeśli jednak jesteś pewien, że potrzebujesz tylko wyjścia w konsoli, to nie ma większego sensu. Pisanie „otoki” cout <<wydaje się bezużyteczne.

Luchian Grigore
źródło
1
Ale ściśle mówiąc, czy funkcja printLine nie różni się poziomem abstrakcji od iteracji po liniach?
@Petr Chyba tak, dlatego sugerują oddzielenie funkcjonalności. Myślę, że koncepcja jest poprawna, ale musisz ją zastosować w poszczególnych przypadkach.
1

Cały powód, dla którego istnieją książki poświęcające rozdziały cnotom „rób jedną rzecz”, polega na tym, że wciąż istnieją programiści, którzy piszą funkcje o długości 4 stron i zagnieżdżają warunki warunkowe na 6 poziomach. Jeśli Twój kod jest prosty i przejrzysty, zrobiłeś to dobrze.

Kevin
źródło
0

Jak skomentowali inni plakaty, zrobienie jednej rzeczy jest kwestią skali.

Sugerowałbym również, że pomysłem One Thing jest powstrzymanie ludzi kodujących na skutek efektów ubocznych. Przykładem tego jest sekwencyjne sprzężenie, w którym metody muszą być wywoływane w określonej kolejności, aby uzyskać „właściwy” wynik.

NWS
źródło