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

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?

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

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?

„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.

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.)

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 .

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.

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ć)

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ć.

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.

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ć.

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.

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.

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.

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.

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?

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.

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 .

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?”

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