Co powiesz na temat przeglądu kodu, gdy druga osoba zbudowała zbyt skomplikowane rozwiązanie? [Zamknięte]

37

Któregoś dnia sprawdziłem kod napisany przez kogoś z mojego zespołu. Rozwiązanie nie było w pełni funkcjonalne, a projekt był zbyt skomplikowany - co oznacza, że ​​zawiera niepotrzebne informacje, zawiera niepotrzebne funkcje i zasadniczo kod ma wiele niepotrzebnych złożoności, takich jak pozłacanie, i próbował rozwiązać problemy, które nie istnieją.

W tej sytuacji pytam „dlaczego to zrobiono w ten sposób?”

Odpowiedź brzmi: druga osoba miała ochotę to zrobić w ten sposób.

Następnie pytam, czy którakolwiek z tych funkcji była częścią specyfikacji projektu lub czy ma ona jakiekolwiek zastosowanie dla użytkownika końcowego, czy też jakieś dodatkowe dane zostaną przedstawione użytkownikowi końcowemu.

Odpowiedź brzmi nie.

Zatem sugeruję, aby usunął całą niepotrzebną złożoność. Odpowiedź, którą zwykle otrzymuję, brzmi: „dobrze, że już zostało zrobione”.

Uważam, że nie jest to zrobione, jest błędne, nie robi tego, czego chcą użytkownicy, a koszty utrzymania będą wyższe niż gdyby wykonano to w prostszy sposób, który zasugerowałem.

Równoważny scenariusz to:
Kolega spędza 8 godzin na ręcznym refaktoryzacji kodu, co można było automatycznie wykonać w Resharper w 10 sekund. Oczywiście nie ufam ręcznemu refaktoryzacji, ponieważ jest wątpliwej jakości i nie w pełni przetestowany.
Ponownie otrzymuję odpowiedź: „dobrze, że już zostało to zrobione”.

Jaka jest odpowiednia reakcja na to podejście?

dan
źródło
47
„Zbudowałeś zbyt skomplikowane rozwiązanie”
Dante
2
Która kwestia jest przedmiotem tego pytania: mentalność / postawa programisty, zarządzanie projektem (w szczególności zarządzanie czasem) lub poziom umiejętności?
rwong
6
prawdopodobnie należy to do miejsca pracy - to nie jest kwestia programowania.
GrandmasterB

Odpowiedzi:

25

Mentalność / postawa

  • Dawaj dobry przykład
  • Upominaj na osobności (jeden do jednego, poza przeglądem kodu)
  • Zachęcaj członków zespołu do zachowania prostoty

Zarządzanie zespołem

  • Poświęć więcej czasu na specyfikację elementu pracy (np. Architektura, kontur algorytmu, szkielet interfejsu użytkownika itp.)
  • Zachęć członków zespołu do uzyskania wyjaśnień dotyczących zakresu elementu pracy
  • Zachęć członków zespołu do omówienia sposobów implementacji elementu pracy
  • Dokonaj rozsądnych szacunków dla każdego elementu pracy przed rozpoczęciem i dołóż wszelkich starań, aby je zrealizować
  • Monitoruj „poprawę” członków zespołu.
    • Po upomnieniu lub pokazaniu właściwego sposobu robienia rzeczy sprawdź, czy członek zespołu się poprawi.

Poziom umiejętności

  • Przeznacz trochę czasu na sesje programowania w parach lub sesje szkoleniowe jeden na jeden, aby jak najlepiej wykorzystać narzędzia programistyczne (refaktoryzacja, przegląd kodu)

Zarządzanie projektem (ryzykiem)

  • Przegląd kodu przeprowadzaj częściej, asynchronicznie (uwaga)
    • Uwaga na temat „asynchronicznie”
      • Kontroler kodu powinien otrzymywać powiadomienia / zaproszenia do przeglądu zmian, jak tylko zostaną zatwierdzone
      • Kontroler kodu powinien mieć możliwość przejrzenia kodu przed jakimkolwiek spotkaniem z programistą.
      • Jeśli potrzebne jest wyjaśnienie od dewelopera, zrób to nieoficjalnie na czacie / e-mailu, nie wydając negatywnej opinii
rwong
źródło
69

Co powiesz na temat przeglądu kodu, gdy druga osoba zbudowała zbyt skomplikowane rozwiązanie?

Mówisz: „zbudowałeś zbyt skomplikowane rozwiązanie”.

Zatem sugeruję, aby usunął całą niepotrzebną złożoność. Odpowiedź, którą zwykle otrzymuję, brzmi: „dobrze, że już zostało zrobione”.

Jeśli jest za późno, aby cokolwiek zmienić, dlaczego przeprowadzasz przegląd kodu?

Hermann Ingjaldsson
źródło
Mówisz w zasadzie, że przegląd kodu działa tylko z ładnymi, zawsze rozsądnymi i racjonalnymi znakami. Rzeczywisty świat wygląda inaczej ...
Philip
3
Czasami musisz robić rzeczy, których nie lubisz, na przykład mówić komuś, kto poświęca cały dzień na pisanie złożonego kodu, że „nie jest dobrze, przywróć go i zacznij od nowa” lub coś w tym stylu. To jest do bani, ale będziesz wdzięczny, że to zrobiłeś.
joshin4colours
3
Prosta odpowiedź, która dokładnie podsumowuje sytuację. Inną odpowiedzią na „To już zostało zrobione” jest wyjaśnienie, że zbyt skomplikowane rozwiązanie będzie kosztowało utratę czasu na konserwację, a jego przeróbka pozwoli zaoszczędzić czas na dłuższą metę.
DJClayworth
30
+ ∞ dla „Jeśli jest już za późno, aby cokolwiek zmienić, to dlaczego przeglądasz kod?”
mskfisher
16

„To już zrobione” nie jest satysfakcjonującą odpowiedzią. Gotowe oznacza przetestowane i działa. Każdy dodatkowy kod, który nie robi nic pożytecznego, powinien być utrzymywany we właściwy sposób (usunięty).

Przydziel mu to zadanie ponownie, prosząc o refaktoryzację i optymalizację jego rozwiązania. Jeśli tego nie zrobi, przydziel mu programistę par i mam nadzieję, że nauczy się czegoś od kolegi.

Andrzej Bobak
źródło
Jeśli naprawdę trudno jest pozbyć się dodatkowego kodu, możesz pozwolić mu go zatrzymać, JEŻELI I TYLKO JEŚLI może stworzyć pełny pakiet testów jednostkowych, aby upewnić się, że nadal działa. Tak czy inaczej, musi on właściwie ZAKOŃCZYĆ pracę.
Michael Kohne
+1 za prosty fakt, że kod zawiera (oczywiste) błędy i dlatego nie został przetestowany.
Ramhound,
8

Zatem sugeruję, aby usunął całą niepotrzebną złożoność. Odpowiedź, którą zwykle otrzymuję, brzmi: „dobrze, że już zostało zrobione”.

To nie jest do przyjęcia odpowiedź:

  • Jeśli naprawdę jest już za późno na zmianę, przegląd kodu jest w dużej mierze stratą czasu i zarząd musi o tym wiedzieć.

  • Jeśli tak naprawdę można powiedzieć „nie chcę się zmieniać”, musisz przyjąć stanowisko, że dodatkowa złożoność jest ZŁA dla bazy kodów, PONIEWAŻ problemów / kosztów, które będzie ponosić później. I przede wszystkim ograniczenie potencjalnego problemu. Prawdziwy powód, dla którego przeprowadzasz przegląd kodu.

I ...

... rozwiązanie nie było w pełni funkcjonalne ...

Jest to prawdopodobnie bezpośredni skutek niepotrzebnej złożoności. Programista uczynił to tak skomplikowanym, że nie w pełni go rozumie i / lub zmarnował czas na wdrażanie swojej złożoności, a nie punktów funkcyjnych. Warto wskazać programiście, że zmniejszenie złożoności może szybciej doprowadzić go do działającego programu.

Teraz wygląda na to , że nie masz siły (a może i pewności siebie), by „odepchnąć się mocno”. Ale mimo to warto trochę o tym hałasować (bez personalizacji) w nadziei, że obrażający programista wykona lepszą pracę ... następnym razem.

Jaka jest odpowiednia reakcja na to podejście?

Ostatecznie, zwróć na to uwagę kierownictwa ... chyba że masz moc, aby to naprawić samodzielnie. (Oczywiście to nie sprawi, że będziesz popularny.)

Stephen C.
źródło
7

Miałeś rację, mylili się:

  • złamana zasada YAGNI
  • złamana zasada KISS
  • czy kod jest w pełni przetestowany? Jeśli nie, to nie jest to zrobione

Jaka jest odpowiednia reakcja na to podejście?

Wykonaj prawidłowy przegląd kodu. Jeśli odmówią wdrożenia sugerowanych zmian bez powodu, przestań marnować czas na sprawdzenie jednego kodu. Możesz także przekazać problem szefowi .

BЈовић
źródło
5

Jednym z działań, które podjął nasz zespół, który radykalnie poprawił sytuację w takich przypadkach, było przejście na znacznie mniejsze zestawy zmian .

Zamiast pracować nad jednym zadaniem przez jeden dzień lub dłużej, a następnie mieć (duży) przegląd kodu, staramy się zameldować znacznie częściej (do 10 razy dziennie). Oczywiście ma to również pewne wady, np. Recenzent musi bardzo szybko reagować, co zmniejsza jego własną wydajność (z powodu częstych zakłóceń).

Zaletą jest to, że problemy są wykrywane i można je rozwiązać wcześniej, zanim zostanie wykonana duża ilość pracy w niewłaściwy sposób.

stefan.s
źródło
Powiedziałbym, że 10 razy w ciągu jednego dnia to niewiele. Jeśli naprawdę chcesz to przeforsować, 3 lub 4 meldowanie będzie w porządku, oznacza to meldowanie średnio co 2 godziny, dając typowy 8-godzinny dzień. Ale 10 meldowań wydaje się, że nie ma czasu na przeglądanie niczego, zgłaszanie raportów ani wdrażanie zmian w oparciu o samą recenzję.
Ramhound
@Ramhound Tak, 10 meldowań to ekstremalny przypadek, 3 do 4 razy jest o wiele bardziej typowy. I potrzebuje trochę czasu, aby się do tego przyzwyczaić ...
stefan.s
2

Powinieneś skupić się na głównej przyczynie problemu:

  1. Edukacja programistów koncentruje się na zwiększaniu złożoności programistów. Zdolność do tego została przetestowana przez szkołę. Dlatego wielu programistów pomyśli, że jeśli wdrożą proste rozwiązanie, nie wykonają poprawnie swojej pracy.
  2. Jeśli programista podąża tym samym wzorem, który robił setki razy na uniwersytecie, tak właśnie myślą programiści - większa złożoność jest trudniejsza, a zatem lepsza.
  3. Aby to naprawić, musisz zachować ścisłe oddzielenie wymagań Twojej firmy względem złożoności w porównaniu do tego, co jest zwykle wymagane w edukacji programisty. Dobry plan to zasada, że ​​„najwyższy poziom złożoności powinien być zarezerwowany tylko dla zadań mających na celu poprawę twoich umiejętności - i nie powinien być stosowany w kodzie produkcyjnym”.
  4. Wielu programistów zdziwi się, że nie wolno im robić najbardziej szalonych projektów w ważnym środowisku kodu produkcyjnego. Po prostu zarezerwuj czas dla programistów na eksperymentalne projekty, a następnie zachowaj całą złożoność po tej stronie ogrodzenia.

(w przeglądzie kodu jest już za późno, aby go zmienić)

tp1
źródło
2

Nie wiem nic, co działałoby po napisaniu kodu.

Przed napisaniem kodu ludzie mogą omówić alternatywne sposoby jego wykonania. Kluczem jest wzajemne przekazywanie pomysłów, więc mam nadzieję, że zostanie wybrany rozsądny.

Istnieje inne podejście, które współpracuje z kontrahentami - umowy o stałej cenie. Im prostsze rozwiązanie, tym więcej $$ programista może zachować.

Mike Dunlavey
źródło
1

Nie możesz naprawić świata.

Nie możesz nawet naprawić całego kodu w swoim projekcie. Prawdopodobnie nie możesz naprawić praktyk programistycznych w swoim projekcie, przynajmniej nie w tym miesiącu.

Niestety, to, czego doświadczasz podczas przeglądania kodu, jest zbyt powszechne. Pracowałem w kilku organizacjach, w których często stwierdzałem, że przeglądam 100 wierszy kodu, który mógł zostać napisany w dziesięciu, i otrzymałem tę samą odpowiedź, co ty: „Jest już napisany i przetestowany” lub „Szukamy błędy, a nie przeprojektowanie. ”

Faktem jest, że niektórzy z twoich kolegów nie potrafią programować tak dobrze, jak ty. Niektóre z nich mogą być w tym dość złe. Nie martw się o to. Kilka klas ze złymi dodatkami nie sprowadzi projektu. Zamiast tego skup się na częściach ich pracy, które będą miały wpływ na innych. Czy testy jednostkowe są odpowiednie (jeśli je masz)? Czy interfejs jest użyteczny? Czy to jest udokumentowane?

Jeśli interfejs do złego kodu jest w porządku, nie przejmuj się nim, dopóki nie będziesz musiał go utrzymać, a następnie przepisz go. Jeśli ktoś narzeka, po prostu nazwij to refaktoryzacją. Jeśli nadal narzekają, poszukaj pracy w bardziej wyrafinowanej organizacji.

Kevin Cline
źródło
0

W projekcie powinna obowiązywać standardowa polityka kontrolująca procedury kontroli jakości i stosowane narzędzia.

Ludzie powinni wiedzieć, co powinni robić i jakie narzędzia są akceptowane w tym projekcie.

Jeśli jeszcze tego nie zrobiłeś, uporządkuj swoje myśli i zrób to.

Przegląd kodu powinien zawierać listę kontrolną standardowych elementów. Jeśli dostaniesz komunikat „zostało już zrobione”, a nie jest to osobiście, nie chciałbym być odpowiedzialny za pracę tego programisty jako kierownik projektu lub starszy programista. Tego podejścia nie wolno tolerować. Rozumiem kłótnię o to, jak coś zrobić, a nawet każdą rzecz, ale po zaakceptowaniu rozwiązania kłamstwo nie powinno być tolerowane i należy to jasno stwierdzić.

Bez szans
źródło
0

Twój sklep musi egzekwować niektóre metodologie projektowania.

  • Wymagania muszą być jasno określone.
  • Musisz opracować przypadki użycia, które obsługują wymagania.
  • Musisz określić funkcje potrzebne do zaimplementowania przypadków użycia.
  • Musisz określić wszelkie niefunkcjonalne wymagania (czasy reakcji, dostępność itp.)
  • Potrzebujesz RTM (Requiements Tracabilty Matrix), aby odwzorować każdą funkcję systemu z powrotem na przypadek użycia i prawdziwe wymaganie.
  • Usuń dowolną funkcję, która nie obsługuje rzeczywistych wymagań.
  • Wreszcie w przeglądzie kodu zaznacz dowolny kod, który nie implementuje ani nie obsługuje bezpośrednio zdefiniowanych funkcji.
James Anderson
źródło
0

Prawdopodobnie nie jest to zbyt skomplikowane, ponieważ powoduje to, że większość ludzi źle się potem czuje. Zakładam, że kiedy tak się dzieje, napisano już dużo kodu, nie mówiąc o tym ani słowa. (Dlaczego tak jest? Ponieważ ta osoba ma wystarczające uprawnienia, więc jej kod nie musi być sprawdzany w rzeczywistości?)

W przeciwnym razie przegląd kodu powinien być mniej formalny, ale częstszy. A zanim napiszesz duże moduły, być może powinieneś szybko omówić, jakie podejście wybrać.

Mówienie „to jest zbyt skomplikowane” nigdzie nie prowadzi.

Philip
źródło
0

Jest to niefortunne, ale Recenzje Kodowe są, wiele razy, bardziej na przyszłość niż na teraźniejszość. Zwłaszcza w środowisku korporacyjnym / korporacyjnym dostarczany kod jest zawsze cenniejszy niż kod nieprzydzielony.

Zależy to oczywiście od tego, kiedy przegląd kodu zostanie zakończony. Jeśli jest to część procesu rozwoju, możesz teraz zyskać jakieś korzyści. Ale jeśli CR jest traktowany bardziej jako sekcja zwłok, najlepiej wskazać, co można zrobić lepiej w przyszłości. W twoim przypadku (jak powiedzieli inni), wskaż ogólnie YAGNI i KISS, a być może niektóre ze szczególnych dziedzin, w których można zastosować te zasady.

Ryan Kinal
źródło
0

Co oznacza zbyt skomplikowane? Składasz dwuznaczne oświadczenie, a następnie w odpowiedzi otrzymasz niejednoznaczną / niezadowalającą odpowiedź. To, co jest zbyt skomplikowane dla jednej osoby, jest idealne dla drugiej.

Celem przeglądu jest wskazanie konkretnych problemów i błędów, a nie stwierdzenie, że się nie podoba, co sugeruje stwierdzenie „zbyt skomplikowane”.

Jeśli zauważysz problem (zbyt skomplikowany), powiedz coś bardziej konkretnego:

  • Czy zmiana części X na Y nie uprości kodu ani nie ułatwi zrozumienia?
  • Nie rozumiem, co tu robisz w części X, myślę, że starałeś się to zrobić. Przedstaw czystszy sposób na zrobienie tego.
  • Jak to przetestowałeś? Testowałeś to? Jeśli jest to zbyt skomplikowane, zwykle prowadzi to do pustych spojrzeń. Zapytanie o testy często powoduje, że osoba sama upraszcza swój kod, gdy nie może wymyślić, jak przetestować oryginalny kod.
  • Wygląda na to, że wystąpił tutaj błąd, zmiana kodu na to rozwiązałaby problem.

Każdy może wskazać problemy, szczególnie dwuznaczne. Istnieje znacznie mniejszy podzbiór, który może przedstawiać rozwiązania. Komentarze do recenzji powinny być jak najbardziej szczegółowe. Powiedzenie, że coś jest zbyt skomplikowane, nie znaczy wiele, może nawet spowodować, że inni będą myśleć, że jesteś niekompetentny, ponieważ nie rozumiesz kodu. Pamiętaj, że większość programistów nie ma pojęcia różnicy między dobrym a złym projektem.

Maczać
źródło
Kod zawiera oczywiste błędy. Fakt, że autor uważa również, że samo rozwiązanie jest nieprawidłowe, podkreśla fakt, że istnieją błędy. Jeśli masz błędy w kodzie i nie mówię o nieoczywistych błędach, których nie można złapać bez pełnego testu regresji, istnieje problem z tym kodem.
Ramhound,
@Ramhound: Jeśli występują błędy, wskaż konkretne błędy. Jeśli naprawianie błędów nie jest częścią procesu recenzji, to po co przeprowadzać recenzję? Jak powiedziałem, zbyt skomplikowane nie jest błędem. Jest to z pewnością niedociągnięcie, ale jeśli jedyną osobą, która uważa, że ​​jest zbyt skomplikowana, jest OP, a nikt inny nie ma się dobrze. Pracuj ciężko, stań się w tym czasie ołowiu i jakości dekretów. Mogę sympatyzować z PO, przeszedłem przez te same problemy, teraz, kiedy mam uprawnienia do kierowania ludźmi, aby wprowadzali zmiany, których chcę, stwierdzam, że inne rzeczy stają się wyższymi priorytetami.
Dunk
0

Czasami warto jako grupa skupić się na niektórych zasadach „zwinnych” - mogą pomóc grupie lub osobie, która wydaje się być trochę zła.

Koncentrowanie się nie musi oznaczać wielkiej wielkiej przeróbki zespołu, ale wszyscy powinniście usiąść i przedyskutować, jakie praktyki są dla was najważniejsze. Sugeruję omówienie przynajmniej tych (i prawdopodobnie kilku innych):

  • Czy najprostsza rzecz, która może działać?
  • Nie będziesz go potrzebować (czy rozwiązujesz problemy, których nie było w specyfikacji)
  • Napisz test przed kodowaniem (pomaga skoncentrować kod)
  • Nie powtarzaj się

Pomocne mogą być również okazjonalne (tygodniowe?) Recenzje tego, co działa, a co nie, a co jest nadal potrzebne ... Jeśli nic innego, dlaczego nie poświęcić godziny na tydzień na omówienie wartości i praktyk zespołu?

Bill K.
źródło
0

Eskalacja, jeśli masz kierownika technicznego. To brzmi jak nawyk, który należy zerwać.

Jeśli kod nie jest zbudowany zgodnie ze specyfikacją, to z definicji powinien zakończyć przegląd kodu. Nie rozumiem pojęcia „dobrze zrobiliśmy coś, o co nikt nie prosił, i to nie działa, więc zostawimy to tam, zamiast robić coś, o co ktoś prosił o to, co działa”.

To zły nawyk dla każdego programisty. Jeśli pracował nad specyfikacją projektu, to nie pasowanie jej bez uzasadnionego powodu jest nie.

kuszenie
źródło
0

Jedno słowo: zwinny

Z pewnością nie rozwiązuje wszystkiego. Ale panując w swoich iteracjach (na przykład 1-2 tygodnie), ograniczając trwające prace i wykorzystując planowanie / przegląd sprintu, powinieneś unikać tych błędów podobnych do wodospadu. Potrzebujesz lepszego wglądu w to, co faktycznie jest robione - podczas gdy jest to wykonywane.

W przypadku normalnego rozwoju opartego na projektach zaleciłbym przyjęcie podejścia Scrum . W przypadku środowisk ciągłego programowania / integracji, a zwłaszcza jeśli masz wielu programistów pracujących nad tymi samymi lub pokrewnymi projektami, rozważ włączenie elementów Kanban . Innym skutecznym podejściem jest wykorzystanie programowania par , zdefiniowanej praktyki programowania ekstremalnego .

Twoja sytuacja nie jest niczym wyjątkowym. Nawet w małych zespołach proces może pomóc w uniknięciu obecnej sytuacji. Przy odpowiedniej widoczności i zadbanym zaległości, takie pytania stają się decyzjami planowania sprintu - ratując cię przed zarządzaniem długiem technicznym .

Adam
źródło
-1

W przeszłości powiedziałem: „ten kod jest złożony i nie jestem pewien, co on chce zrobić, czy można go uprościć lub napisać jaśniej?”

Vatine
źródło
-2

Musisz napisać kod po usunięciu / przywróceniu kodu: „Ups, twój kod zniknął. Przepisz go. Jak już go napisałeś, będziesz potrzebować mniej niż dwadzieścia minut, aby podać TYLKO kod wymagany przez specyfikację.

„Moja następna recenzja nastąpi za 20 minut.

"Dobry dzień."

NIE przyjmuj żadnych argumentów!

Gotowe, IMHO

Chris

kolan
źródło
Cieszę się, że mój szef nie działa w ten sposób.
@Jon: Kiedy ludzie reagują nieprofesjonalnie, jak w „dobrze już zrobione”, jak powiedziałby mój sześciolatek, musisz traktować ich jak dzieci.
cneed
2
Nie mogę powiedzieć, że się zgadzam. Jakich rezultatów oczekujesz od swoich ludzi, jeśli „traktujesz ich jak dzieci”? Istnieją inne podejścia, które według IMHO są bardziej konstruktywne.
Nie opowiadam się za traktowaniem profesjonalistów jak dzieci. W podanym przykładzie mamy upartego, rozdrażnionego kogoś, kto pisze błędny kod z nieuzasadnioną funkcjonalnością i zwraca dziecinne odpowiedzi na uzasadnione pytania. Dan prosi o najlepszy sposób, aby sobie z tym poradzić. Nie najpopularniejszy sposób.
cneed
Na szczęście w moim zespole nie ma „dzieci”, więc nie trzeba traktować ich jak profesjonalistów. Nie dodają niezadawanych funkcji (marnują mój czas i pieniądze), piszą dość solidny kod, a kiedy są proszeni o ponowne sprawdzenie lub poprawienie czegoś, robią to i uczą się z tego doświadczenia.
cneed