Czy w recenzjach kodu recenzent powinien zawsze przedstawiać rozwiązanie problemów? [Zamknięte]


93

Podczas przeglądania kodu zwykle próbuję podać konkretne zalecenia dotyczące rozwiązania problemów. Ale ze względu na ograniczony czas, który można poświęcić na przegląd, nie zawsze działa to dobrze. W takich przypadkach uważam, że jest bardziej wydajny, jeśli programista sam wymyśli rozwiązanie.

Dzisiaj przejrzałem trochę kodu i stwierdziłem, że klasa nie była oczywiście dobrze zaprojektowana. Miał szereg opcjonalnych atrybutów, które zostały przypisane tylko do niektórych obiektów i pozostawione puste dla innych. Standardowym sposobem rozwiązania tego problemu byłoby podzielenie klasy i zastosowanie dziedziczenia. Jednak w tym konkretnym przypadku rozwiązanie to wydawało się nadmiernie komplikować. Nie brałem udziału w tworzeniu tego oprogramowania i nie znam wszystkich modułów. Dlatego nie czułem się wystarczająco kompetentny, aby podjąć konkretną decyzję.

Innym typowym przypadkiem, z którym wiele razy się spotkałem, jest to, że znajduję oczywiście pozbawioną znaczenia lub nawet wprowadzającą w błąd funkcję, nazwę klasy lub zmiennej, ale sam nie jestem w stanie wymyślić dobrego imienia.

Więc ogólnie, jako recenzent, czy można powiedzieć „ten kod jest wadliwy, ponieważ ... zrób to inaczej”, czy też musisz wymyślić konkretne rozwiązanie?



24
@gnat: Nie, kod nie jest zbyt skomplikowany. I to tylko przykład. Ogólnie pytam, czy recenzent jest odpowiedzialny za przedstawienie rozwiązania.
Frank Puffer

5
nie, powiedziałbym, że jako recenzent nie jesteś zobowiązany powiedzieć, jak to poprawić. Jeśli potrafisz opisać, co jest tam źle, zrób to; jeśli nie - po prostu podaj ogólny komentarz. (Jeden z najbardziej użytecznych komentarzy w recenzjach, które, jak pamiętam, dosłownie brzmiał: „ta klasa to wszystko śmieci”)
komara

5
„Standardowym sposobem rozwiązania tego problemu byłoby podzielenie klasy i zastosowanie dziedziczenia.”. Mam nadzieję, że nie sprawdzasz mojego kodu!
ogrodnik

7
Wskazanie potencjalnych problemów może wystarczyć. Recenzent patrzy na kod jako użytkownik, opiekun lub projektant. Zapewnienie innego kąta widzenia lub problemów z wykrywaniem, które programista mógł jeszcze nie zauważyć, może pomóc koderowi poprawić jego pracę. A jeśli zauważysz coś, co lubisz, nie zaszkodzi to również zgłosić. To nie powinno być ćwiczenie korygujące, ale raczej pouczające. Dlatego nazywa się to „recenzowaniem”.
Martin Maat

Odpowiedzi:


139

Jako recenzent Twoim zadaniem jest sprawdzenie, czy fragment kodu (lub dokumentu) spełnia określone cele uzgodnione przed przeglądem.

Niektóre z tych celów zazwyczaj wymagają oceny sytuacji, czy cel został osiągnięty, czy nie. Na przykład cel, jakim jest utrzymanie kodu, zwykle wymaga wywołania oceny.

Jako recenzent Twoim zadaniem jest wskazanie, gdzie cele nie zostały osiągnięte, a zadaniem autora jest upewnienie się, że jego praca rzeczywiście spełnia te cele. W ten sposób nie musisz informować, w jaki sposób należy wprowadzić poprawki.

Z drugiej strony samo powiedzenie autorowi „to jest wada. Napraw to” zazwyczaj nie prowadzi do pozytywnej atmosfery w zespole. Dla pozytywnej atmosfery dobrze jest przynajmniej wskazać, dlaczego coś jest wadliwe w twoich oczach i zapewnić lepszą alternatywę, jeśli masz taką.
Poza tym, jeśli recenzujesz coś, co wygląda „źle”, ale tak naprawdę nie masz lepszej alternatywy, możesz również zostawić komentarz w stylu „Ten kod / projekt nie pasuje do mnie, ale ja nie mamy jasnej alternatywy. Czy możemy to omówić? ” a następnie spróbuj uzyskać coś lepszego razem.


23
+1 za dyskusję, aby wspólnie znaleźć rozwiązanie - w ten sposób najwięcej uczę się od starszych programistów przeglądających mój kod
dj18

19
+1. Przekazując informacje zwrotne, najlepiej, jeśli to możliwe, dać konstruktywną krytykę.
FrustratedWithFormsDesigner

7
Szczególnie zgadzam się z ostatnim. Można powiedzieć, że „to rozwiązanie jest złe, ponieważ ...” lub „Martwię się, że ta część może być problematyczna, ponieważ ...” bez podania rozwiązania.
Daniel T.

1
@dotancohen: „Czy możemy to omawiać” miało być pytaniem. Osobiście i tak prowadziłbym dyskusję, nawet jeśli chciałbym się czegoś nauczyć.
Bart van Ingen Schenau

1
Subtelne niebezpieczeństwo polega na tym, że przy wystarczającej liczbie dyskusji i komunikacji dotyczącej wdrażania przestaje to być przegląd i staje się programowaniem w parach. Programowanie w parach jest dobre, ale kiedy to się skończy, potrzebujesz trzeciej osoby do sprawdzenia, ponieważ utracono niezależność.
candied_orange

35

Kilka dobrych odpowiedzi tutaj, ale myślę, że brakuje jednej ważnej kwestii. Duże znaczenie ma to, czyj kod przeglądasz, jak doświadczona jest ta osoba i jak obsługuje takie sugestie. Jeśli znasz dobrze swojego kolegę z drużyny i spodziewasz się, że taka notatka „ten kod jest wadliwy, ponieważ ... zrób to inaczej” wystarczy, aby wymyślił lepsze rozwiązanie, taki komentarz może być w porządku. Ale są zdecydowanie osoby, w których taki komentarz nie jest wystarczający, i którym należy dokładnie powiedzieć, jak poprawić swój kod. Więc IMHO jest to wezwanie do osądu, które możesz wykonać tylko dla indywidualnej sprawy.


29

Czy ogólnie, jako recenzent, czy można powiedzieć „ten kod jest wadliwy, zrób to inaczej”, czy też trzeba wymyślić konkretne rozwiązanie?

Żadne z tych dwóch nie jest idealnym IMO. Najlepiej jest porozmawiać z autorem i wspólnie rozwiązać problem.

Recenzje kodu nie muszą być asynchroniczne. Wiele problemów zostanie odblokowanych, jeśli przestaniesz postrzegać je jako biurokratyczny proces i poświęcisz trochę czasu na komunikację na żywo.


„Proces biurokratyczny” to taki dobry sposób na wyrażenie tego!
od

17

Czy w recenzjach kodu recenzent powinien zawsze przedstawiać rozwiązanie problemów?

Nie. Jeśli to robisz, nie jesteś recenzentem, jesteś kolejnym programistą.

Czy w recenzjach kodu recenzent nigdy nie powinien przedstawiać rozwiązania problemów?

Nie. Twoim zadaniem jest poinformowanie o problemie. Jeśli przedstawienie rozwiązania wyjaśni problem, zrób to. Tylko nie oczekuj, że pójdę za twoim rozwiązaniem. Jedyne, co możesz tutaj zrobić, to wskazać. Nie musisz dyktować wdrożenia.

Kiedy recenzent powinien przedstawić rozwiązanie problemów?

Kiedy jest to najbardziej skuteczny sposób komunikacji. Jesteśmy małpami kodowymi, a nie angielskimi specjalnościami. Czasami najlepszym sposobem na pokazanie, że kod jest do bani ... jest mniej niż optymalny ... jest pokazanie im kodu, który jest do bani mniej ... jest więcej opcji ... och, do diabła, wiesz o co mi chodzi.


8
Nie koduj w próżni, ponieważ jest do bani.

Hm Kiedy sugeruję rozwiązanie problemu, często przynosi korzyści, o których jestem świadomy, ale po prostu zajęłoby to zbyt wiele czasu, aby podać wyczerpującą listę wszystkich. (Często wiąże się to ze stabilnością i pewnością, że dana rzecz będzie działać, gdy będziemy zmieniać inne rzeczy wokół niej.) Więc jeśli zrobisz coś innego, co nie rozwiąże tylu problemów, nie byłbym do końca zadowolony (przynajmniej nie, chyba że możesz mi powiedzieć dobry powód, dlaczego to, co zasugerowałem, nie zadziałało). Jak sobie z tym poradzisz?
jpmc26

1
PS: „Małpa kodowa” jest często używana do opisania niewykwalifikowanego, bezmyślnego programisty, który po prostu robi to, co im powiedziano, nawet jeśli jest to zły pomysł i nie ma dobrej wrażliwości projektowej. Zobacz słownik miejski . Nawet Wikipedia zauważa, że ​​czasami jest to uwłaczające.
jpmc26

@ jpmc26, jeśli zamierzasz używać kodu do komunikowania się ze mną, to mam nadzieję, że użyjesz kodu, który pokazuje, w jaki sposób można lepiej rozwiązać problem. Ponadto Code Monkey można używać z czułością. Na pewno więcej uczuć niż angielskie kierunki
candied_orange

„Małpa z kodem wstaje po kawę. Małpa z kodem idzie do pracy. Małpa z kodem ma nudne spotkanie, z nudnym kierownikiem Robem. Rob mówi, że małpa z kodem jest bardzo pracowita, ale jego wydajność śmierdzi ...”
Baldrickk

13

Głównym problemem jest to, że jeśli ludzie wiedzieliby, jak lepiej napisać kod, zwykle zrobiliby to w pierwszej kolejności. To, czy krytyka jest wystarczająco szczegółowa, zależy w dużej mierze od doświadczenia autora. Bardzo doświadczeni programiści mogą być w stanie skrytykować „ta klasa jest zbyt skomplikowana” i wrócić do deski kreślarskiej i wymyślić coś lepszego, ponieważ właśnie mieli wolny dzień z powodu bólu głowy lub byli niechlujni, ponieważ byli w pośpiechu.

Zwykle jednak musisz przynajmniej zidentyfikować źródło komplikacji. „Ta klasa jest zbyt skomplikowana, ponieważ łamie prawo Demeter w każdym miejscu”. „Ta klasa miesza obowiązki warstwy prezentacji i warstwy trwałości”. Nauczenie się identyfikować te przyczyny i zwięźle je wyjaśniać jest częścią stawania się lepszym recenzentem. Rzadko musisz szczegółowo omawiać rozwiązania.


4
+100 moją najczęstszą frustracją związaną z recenzjami Code jest to, że gdybym znał lepszy sposób, prawdopodobnie napisałbym w ten sposób.
RubberDuck

Kocham twoje pierwsze zdanie. Sprawiło, że pomyślałem o zadaniu sobie pytania: „czy ten kod jest wystarczająco dobry?” następnie przerzucając monetę, aby zdecydować, czy zawracać sobie głowę poprawą, czy nie! (Zwykle trzymam się tego, dopóki nie skończy mi się czas, ale może mógłbym przestać, gdy będzie wystarczająco dobry?)

IMO „Ten kod jest skomplikowany, ponieważ łamie Prawo Demetera” to zły komentarz. „Ten kod jest skomplikowany, ponieważ X jest zbyt sprzężony z Y, a Z” jest lepszy.
immibis

„Gdyby ludzie wiedzieli, jak lepiej napisać kod, zwykle zrobiliby to przede wszystkim”. Istnieją wyjątki. Opracowałem ten kod, który działa, ale w przyszłości ugryzie nas w tyłek. Kierownik nietechniczny nie rozumie „Nie podoba mi się ten kod i chcę go ulepszyć przez trzy dni”. Kierownik nietechniczny rozumie: „Joe sprawdził i odrzucił ten kod i potrzebuję trzech dni, aby go ulepszyć”.
gnasher729

4

Istnieją dwa rodzaje złych programistów: ci, którzy nie przestrzegają standardowych praktyk i ci, którzy „tylko” stosują standardowe praktyki.

Kiedy miałem ograniczony kontakt w pracy / udzielanie komuś informacji zwrotnych, nie powiedziałbym: „To zły projekt”. ale coś w stylu „Czy możesz mi wyjaśnić tę klasę?” Możesz odkryć, że jest to dobre rozwiązanie, twórca szczerze zrobił wszystko, co mógł, a nawet uznał, że jest to złe rozwiązanie, ale wystarczająco dobre.

W zależności od odpowiedzi będziesz miał lepszy pomysł, jak podejść do każdej sytuacji i osoby. Mogą szybko rozpoznać problem i samodzielnie odkryć poprawkę. Mogą poprosić o pomoc lub postarają się rozwiązać ją samodzielnie.

W naszej firmie sugerowane są praktyki, ale prawie wszystkie mają wyjątki. Jeśli rozumiesz projekt i sposób, w jaki zespół do niego podchodzi, może to być kontekst dla określenia celu przeglądu kodu i sposobu rozwiązania problemów.

Zdaję sobie sprawę, że jest to bardziej podejście do problemu niż jednoznaczne rozwiązanie. Będzie zbyt duża różnorodność, aby uwzględnić wszystkie sytuacje.


1
Ale jeśli wymaga to wyjaśnienia, aby można było rozpoznać dobry projekt, brakuje wbudowanych komentarzy.
Wildcard

1
Czasami reguły nie mają wyjątków, ale zwykle nie.

@Wildcard - zależy to od umiejętności i preferencji / opinii osób, które na nią patrzą.
JeffO

1
@Wildcard Przyjmuję podejście, że informacje zwrotne powinny być sformułowane jako pytanie, ale odpowiedź (ostatecznie) przyjmie formę komentarza lub zmiany kodu (np. Lepsze nazewnictwo). Dzięki temu programiści mogą wyjaśnić swoje przemyślenia i omówić opcje, zamiast czuć się popytem lub przypadkowo wykonując za nich pracę.
IMSoP

3

Kiedy przeglądam kod, dostarczam tylko rozwiązanie problemów, które stwierdzam, czy mogę to zrobić przy niewielkim wysiłku. Staram się jednak sprecyzować, na czym polega problem, odwołując się w miarę możliwości do istniejącej dokumentacji. Oczekiwanie, że recenzent zapewni rozwiązanie każdego zidentyfikowanego problemu, stwarza przewrotną zachętę - zniechęci recenzenta do wskazywania problemów.


3

Moja opinia jest coraz silniejsza w kierunku nieudostępnienia kodu w większości przypadków z kilku powodów:

  • Jeśli samo wyjaśnienie nie wystarczy, zawsze mogą poprosić o próbkę tego, o czym myślisz.
  • Nie marnujesz czasu, próbując zaznajomić się z kodem, którego nie dotykałeś od dłuższego czasu, po prostu go nieco zmodyfikować, podczas gdy ktoś inny właśnie spędził ten czas.
  • Jeśli są zaznajomieni z fragmentem kodu, a ty nie, podanie tylko informacji zwrotnych może dać lepszy kod, niż byś napisał. Podanie komuś gotowego rozwiązania często powoduje, że po prostu z niego korzysta, bez rozważania dalszej poprawy.
  • Zawsze zapewnianie rozwiązania graniczy z protekcjonizmem. Pracujesz z kimś, mam nadzieję, że byli wystarczająco dobrzy, aby zostać zatrudnionym. Jeśli udało ci się dowiedzieć, dlaczego coś jest złym pomysłem, dlaczego nie mieliby się tego nauczyć, słuchając opinii i robiąc to sami?
  • Zawsze zapewnianie rozwiązania jest po prostu dziwne. Wyobraź sobie, że programujesz w parach przy biurku: właśnie ukończyli kilka linii, które Twoim zdaniem nie są świetne. Czy po prostu mówisz im, co zauważyłeś i dlaczego, czy chwytasz ich klawiaturę i od razu wyświetlasz swoją wersję? To właśnie zawsze zapewnia Twoje rozwiązanie innym osobom.
  • Zawsze możesz powiedzieć, co zrobiłbyś zamiast tego, nie tracąc czasu na napisanie tego. Zrobiłeś to, opisując pierwszy problem w pytaniu.
  • Nie rozdawaj jedzenia, ucz jak łowić ryby;)

Jasne, są przypadki, w których zastanawiasz się nad jakąś konkretną alternatywą i warto ją dołączyć. Ale to naprawdę rzadkie z mojego doświadczenia. (wiele recenzji, duże projekty) Oryginalny autor może zawsze poprosić cię o próbkę, jeśli zajdzie taka potrzeba.

Nawet wtedy, z trzeciego powodu, podając próbkę, warto powiedzieć na przykład: „użycie x.foo()uprościłoby to”, a nie pełne rozwiązanie - i niech autor to napisze. To również oszczędza Twój czas.


Twój piąty punkt sprawił, że się uśmiechnęłam, wyobrażałam sobie „pojedynek na klawiaturach”, żeby zobaczyć, kto pierwszy może znaleźć świetne rozwiązanie. Kto wiedział, że programowanie w parach może być jak te dwie gry zręcznościowe z wyścigów samochodowych lub sport w pełnym kontakcie? „ Steve brutalnie zameldował Rona w BSOD, 2 minuty w polu karnym ...

2

Myślę, że kluczem do kodowania recenzji jest uzgodnienie zasad przed recenzją.

Jeśli masz przejrzysty zestaw reguł, nie powinno być potrzeby oferowania rozwiązania. Sprawdzasz tylko, czy przestrzegano reguł.

Jedynym przypadkiem, gdy pojawiłoby się pytanie o zastępcę, byłoby, gdyby pierwotny twórca nie wymyślił sposobu na wdrożenie funkcji zgodnej z regułami. Załóżmy, że masz wymagania dotyczące wydajności, ale funkcja ta trwa dłużej niż próg po kilku próbach optymalizacji.

Jednak! jeśli twoje zasady są subiektywne „Nazwiska muszą być przeze mnie zatwierdzone!” więc tak, właśnie mianowałeś się głównym nadawcą i powinieneś spodziewać się wniosków o listy akceptowalnych nazwisk.

Twój przykład dziedziczenia (parametry opcjonalne) jest być może lepszy, ponieważ widziałem reguły przeglądu kodu, które zabraniają długich metod i „zbyt wielu” parametrów funkcji. Ale normalnie można je w prosty sposób rozwiązać przez podział. Jest to część „to rozwiązanie wydawało się nadmiernie komplikować rzeczy”, gdzie twoja obiektywność jest ingerująca i być może wymaga uzasadnienia lub alternatywnego rozwiązania.


2
„Myślę, że kluczem do kodowania recenzji jest uzgodnienie zasad przed recenzją”. To byłby idealny przypadek. W praktyce nie możemy zakładać, że każdy programista zna wszystkie zasady. Recenzje kodu są przydatne do rozpowszechnienia tej wiedzy i wyjaśnienia zasad praktycznymi przykładami. Może to jedna z największych zalet robienia recenzji kodu.
Frank Puffer

zapisz zasady w dokumencie standardów kodowania i przekaż je nowym twórcom
Ewanowi

1
Zapisaliśmy standardy kodowania, które są przekazywane nowym programistom. Działa to przez większość czasu, ale czasami występują błędne interpretacje. Oprócz zapisanych standardów kodowania istnieją ogólne zasady, takie jak DRY lub SOLID, które są również omawiane w przeglądach kodów. Od naszych programistów oczekujemy podstawowej wiedzy na ich temat, a także przeprowadzamy wewnętrzne szkolenia w celu jej ulepszenia. To ciągły proces, którego częścią są przeglądy kodu.
Frank Puffer

0

Jeśli potencjalne rozwiązanie jest szybkie i łatwe do napisania, staram się zawrzeć je w komentarzach recenzentów. Jeśli nie, przynajmniej wymieniam moje obawy i dlaczego uważam ten konkretny przedmiot za problematyczny. W przypadku nazw zmiennych / funkcji, w których nie można wymyślić czegoś lepszego, zwykle potwierdzam, że nie mam lepszego pomysłu, i kończę komentarz w formie pytania otwartego na wypadek, gdyby ktoś mógł . W ten sposób, jeśli nikt nie wymyśli lepszej opcji, przegląd nie jest tak naprawdę wstrzymywany.

Na przykład, który podałeś w swoim pytaniu, ze źle zaprojektowaną klasą. Chciałbym zostawić kilka uwag, że chociaż wydaje się, że może to być przesada, dziedziczenie byłoby prawdopodobnie najlepszym sposobem rozwiązania problemu, który kod próbuje rozwiązać, i zostaw to. Spróbuję sformułować w sposób, który wskazuje, że nie jest to przeszkodą w pokazie i zależy od autora, czy to naprawić, czy nie. Chciałbym również potwierdzić, że nie jesteś szczególnie zaznajomiony z tą częścią kodu, i zachęcić bardziej kompetentnych programistów i / lub recenzentów, aby wyjaśnili, czy istnieje powód, dla którego zrobiono to tak, jak jest.


0

Idź i porozmawiaj z osobą, której kod sprawdzasz. Poinformuj ich w przyjazny sposób, że trudno ci zrozumieć, a następnie przedyskutuj z nimi, jak można to wyjaśnić.

Pisemna komunikacja prowadzi do ogromnej ilości zmarnowanego czasu, a także do urazy i nieporozumień.

Twarzą w twarz przepustowość jest znacznie wyższa, a kanał emocjonalny jest boczny, aby zapobiec wrogości.

Jeśli faktycznie rozmawiasz z facetem, jest to znacznie szybsze i możesz zaprzyjaźnić się z nowym przyjacielem i odkryć, że oboje bardziej lubicie swoją pracę.


wydaje się, że nie oferuje to nic istotnego w porównaniu z punktami poczynionymi i wyjaśnionymi w poprzednich 11 odpowiedziach
komnata
Korzystając z naszej strony potwierdzasz, że przeczytałeś(-aś) i rozumiesz nasze zasady używania plików cookie i zasady ochrony prywatności.
Licensed under cc by-sa 3.0 with attribution required.