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

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?

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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