Po poważnych problemach z jakością w ostatnim roku moja firma niedawno wprowadziła recenzje kodów. Proces przeglądu kodu został szybko wprowadzony, bez wytycznych i jakiejkolwiek listy kontrolnej.
Wybrano mnie i innego programistę do przeglądu wszystkich zmian dokonanych w systemach, zanim zostaną one scalone z bagażnikiem.
Zostaliśmy również wybrani jako „Technical Lead”. Oznacza to, że jesteśmy odpowiedzialni za jakość kodu, ale nie mamy żadnych uprawnień do wprowadzania zmian w procesie, ponownego przypisywania programistów lub wstrzymywania projektów.
Technicznie możemy odrzucić połączenie, przywracając je do rozwoju. W rzeczywistości kończy się to prawie zawsze, gdy nasz szef żąda dostarczenia go na czas.
Nasz menedżer jest MBA, który zajmuje się głównie tworzeniem harmonogramu nadchodzących projektów. Podczas gdy próbuje, prawie nie ma pojęcia, co robi nasze oprogramowanie z biznesowego punktu widzenia, i stara się zrozumieć nawet najbardziej podstawowe wymagania klientów bez wyjaśnienia ze strony dewelopera.
Obecnie rozwój odbywa się w oddziałach programistycznych w SVN, po tym, jak deweloper myśli, że jest gotowy, ponownie przypisuje bilet w naszym systemie biletowym do naszego menedżera. Menedżer następnie przypisuje nam to.
Recenzje kodu spowodowały pewne napięcia w naszym zespole. Zwłaszcza niektórzy starsi członkowie kwestionują zmiany (tj. „Zawsze tak robiliśmy” lub „Dlaczego metoda powinna mieć sensowną nazwę, wiem, co robi?”).
Po kilku pierwszych tygodniach moja koleżanka zaczęła się przesuwać, aby nie sprawiać kłopotów współpracownikom (powiedziała mi, że po zgłoszeniu błędu przez klienta, wiedziała o błędzie, ale obawiała się, że deweloper byłby na nią zły za to, że to wskazała).
Z drugiej strony jestem teraz znany z tego, że jestem dupkiem, który wskazuje problemy z popełnionym kodem.
Nie sądzę, że moje standardy są zbyt wysokie.
Moja lista kontrolna w tej chwili to:
- Kod się skompiluje.
- Jest co najmniej jeden sposób działania kodu.
- Kod będzie działał z większością normalnych przypadków.
- Kod będzie działał z większością przypadków skrajnych.
- Kod zgłosi uzasadniony wyjątek, jeśli wstawione dane są nieprawidłowe.
Ale w pełni akceptuję sposób, w jaki udzielam informacji zwrotnej. Podaję już praktyczne punkty wyjaśniające, dlaczego coś należy zmienić, czasem nawet po prostu pytam, dlaczego coś zostało zaimplementowane w określony sposób. Kiedy myślę, że jest źle, zwracam uwagę, że opracowałbym go w inny sposób.
Brakuje mi umiejętności znalezienia czegoś, co można by nazwać „dobrym”. Czytałem, że należy próbować przekazywać złe wiadomości w dobrych wiadomościach.
Ale trudno mi znaleźć coś dobrego. „Hej, tym razem faktycznie popełniłeś wszystko, co zrobiłeś” jest bardziej protekcjonalne niż miłe lub pomocne.
Przykładowy przegląd kodu
Hej Joe,
Mam pytania dotyczące twoich zmian w klasie Library \ ACME \ ExtractOrderMail Class.
Nie rozumiem, dlaczego oznaczyłeś „TempFilesToDelete” jako statyczny? W tej chwili drugie wywołanie „GetMails” wyrzuciłoby wyjątek, ponieważ dodajesz do niego pliki, ale nigdy ich nie usuwasz po ich usunięciu. Wiem, że funkcja jest wywoływana tylko raz na przebieg, ale w przyszłości może się to zmienić. Czy możesz po prostu uczynić go zmienną instancji, wtedy moglibyśmy mieć wiele obiektów równolegle.
... (Niektóre inne punkty, które nie działają)
Drobne punkty:
- Dlaczego „GetErrorMailBody” przyjmuje wyjątek jako parametr? Przegapiłem coś? Nie rzucasz wyjątku, po prostu przekazujesz go i wywołujesz „ToString”. Dlaczego?
- SaveAndSend To nie jest dobra nazwa dla metody. Ta metoda wysyła wiadomości z błędami, jeśli przetwarzanie wiadomości nie powiodło się. Czy możesz zmienić nazwę na „SendErrorMail” lub coś podobnego?
- Nie komentuj starego kodu, usuń go całkowicie. Nadal mamy to w obaleniu.