Dobry, zły kod: w jaki sposób code review (przegląd kodu) ratuje projekt

Dobry, zły kod: w jaki sposób code review (przegląd kodu) ratuje projekt

Cześć wszystkim! Nazywam się Bogdan Czupika, pracuję w startup edtech Mate academy na stanowisku Team Lead/ Java Coach. Do moich obowiązków należy przegląd kodów, tworzonych przez nasz zespół, weryfikacja kodów, tworzonych przez studentów i przeprowadzenie rozmów kwalifikacyjnych z kandydatami, starającym się o pracę w naszym zespole.

W procesie pisania oprogramowania w czasie rzeczywistym (live coding) podczas rozmów kwalifikacyjnych – jest to jeden z obowiązkowych etapów — spotkałem się z wieloma przypadkami, kiedy kandydaci piszą zły kod. Nawet mimo próśb o edytowanie i dostosowanie, aby dodać do Master. Przy czym błędy są popełniane zarówno przez juniorów, jak i seniorów (na mniejszą skalę, ale jednak).

W niniejszym artykule podsumowałem nie tylko własne doświadczenie, ale także doświadczenie zespołu liczącego 70 developerów i mentorów pracujących w naszej firmie. I najważniejsze – odpowiem na pytanie: Jak pisać kod wysokiej jakości? Oczywiście podając przykłady i udzielając rad. Materiał przyda się zarówno osobom piszącym kod, jak i czytającym.

Musisz przyznać, że przyjemniej i szybciej się czyta ten kod, którego styl jest podobny do tego pisanego przez Ciebie w ramach bieżącego projektu. Gdyby w stylu kodowania panowała anarchia, wkrótce mielibyśmy problem z czytaniem i utrzymywaniem kodu.

Kiepski kod nie powinien znaleźć się w gałęzi głównej (master). I na tym właśnie polega cel przeglądu kodu (code review).

Code review (przegląd kodu)

Zwykle przegląd kodu należy do obowiązków najbardziej doświadczonych kolegów w zespole oraz pracowników, którzy są dobrze się orientują w zadaniu, nad którym obecnie pracują. Jednak to nie oznacza, że juniorzy nie mogą dokonać przeglądu kodu stworzonego przez Team Leadera. Mogą i powinni. Jeżeli ko, napisany przez Team Leadera lub Tech Leadera jest niezrozumiała dla juniora, warto zastanowić się nad prostszym i bardziej „eleganckim” rozwiązaniem.

Czy jakość pisanego kodu może zostać potwierdzona bez przeglądu kodu? Moja subiektywna opinia – nie. Roboczego kodu – tak, kodu wysokiej jakości – nie. Jeżeli nie robić przeglądu kodu, z czasem utrzymanie takiego kodu będzie skomplikowane i skutkowało dodatkowymi kosztami dla biznesu. Traktuj przegląd kodu jako wspomagające (powiedziałabym nawet obowiązkowe) narzędzie, które pozwoli poprawić jakość stworzonego kodu.

Aby proces przeglądu kodu był zrozumiały zarówno dla współtwórcy, jak i dla osoby dokonującej przeglądu kodu, określ zasady, których należy przestrzegać. Pomogą w tym wytyczne (guides), które należy wdrożyć w ramach projektu / firmy.

Zbiór wytycznych (Style guides)

Zbiór wytycznych (Style guides) stanowi podstawę przeglądu systemowego. Jak opisano w zbiorze wytycznych, opracowanym przez Google:

«Style» covers a lot of ground, from «use camelCase for variable names» to «never use global variables» to «never use exceptions».

Oprócz wspomnianych powyżej zbiorów wytycznych, opracowanych przez Google, występuje mnóstwo innych publicznych style guides. Tu na przykład mamy świetny i wysokiej jakości style guide dla JS opracowany przez Airbnb. W przypadku Python, oprócz zbioru opracowanego przez Google, można posługiwać się Black code style (bardziej mi się podoba) albo PEP8.

Jestem przekonany, że dla Twojego języka programowania też dość łatwo możesz znaleźć swój style guide. Ponadto każdy projekt / firma może mieć własne opracowane style guides.

Swoją drogą, swego rodzaju style guides występują również w designie. Tu mamy przykład typowych list kontrolnych (checklist), którymi należy posługiwać się podczas opracowywania designu.

Jedna z największych korzyści z posługiwania się zbiorem wytycznych polega na tym, że można je importować do swego IDE (przykład dla Intellij Idea) i po automatycznym formatowaniu uzyskujesz kod gotowy do wypchnięcia do Master. Prawie))

Narzędzia do przeglądu kodu 

Jesteś programistą! Zautomatyzuj swoją pracę!

Zasady opisane w style guides – oczywiście jest to całkiem fajne, ale lepiej, żebyś posługiwał się nimi od razu automatycznie podczas pull requestów. W tym celu możemy korzystać ze standardowych narzędzi, które rozwiązują ten problem. Takie narzędzia są nazywane statycznymi analizatorami kodu. Najpopularniejszy to  SonarQube. Istnieje mnóstwo alternatyw typu DeepSource, ale posługujemy się Apache Maven Checkstyle Plugin dla Java code i ESLint dla TypeScript. Jak to się mówi KISS.

Jest to wygodne, gdy integrujesz te automatyczne narzędzia do swoich CI pipelines i automatyczne weryfikacje będą dokonywane automatycznie podczas pull requestów. Tu mamy przykład, w jaki sposób możemy zainstalować ESLint za pomocą GitHub actions.

Dlaczego style guide to super, ale to za mało

Style guides pozwalają regulować składnię, a nie logikę. Szczególnie nie logikę nazewnictwa zmiennych, metod lub klas, które mogą być uzależnione od kontekstu zadania.

Aby to unaocznić, zajmijmy się live coding.

Live coding

Weźmiemy jako przykład zadanie: Length of the Last Word z Leetcode. Wyobraź sobie, że jesteś na rozmowie kwalifikacyjnej i masz przed sobą pustą klasę w IDE. Zadanie jest następujące: „Utwórz metodę, która „obsługuje” łańcuch, składający się z  jednego i więcej słów. Powinieneś zwrócić ilość znaków ostatniego słowa”.

Z punktu widzenia rozmowy kwalifikacyjnej interesuje mnie poniższe:

  • nazwa metody;
  • nazwa parametru wejściowego;
  • nazwa zmiennych wewnątrz metody;
  • interpunkcja i składnia;
  • czytelność kodu;
  • performance (wykonanie).

A więc zacznijmy. Zaznaczę, że tu i w dalszej kolejności kod będzie pisany w języku JavaScript i będę powoływać się na Google style guide dla Java.
Zawsze zaczynamy od tworzenia klasy i metody, gdzie będzie odbywać się wykonanie:

class a {
      public int method(String s){ return null;    }
}

Teraz przeanalizujemy błędy w tym kodzie.

1. Nieinformatywna nazwa klasy a. Ale mamy zbyt mało kontekstu, by zrozumieć, w jakim celu będzie stosowana nasza metoda. Dlatego ustawiam poniższy kontekst: takiego rozwiązania potrzebujemy tylko w ramach rozmowy kwalifikacyjnej i nie będziemy nim się posługiwać  w gotowym kodzie. Przy czym, kierując się zasadami style guide, klasa powinna być rzeczownikiem albo spójnikiem i ma być pisana w UpperCamelCase. Biorąc pod uwagę taki kontekst zaproponowałbym nazwę klasy Solution (w Leetcode to standardowa nazwa klas).

2. Nieinformatywna nazwa metody. Nazwa metody powinna opisywać działanie, wykonywane w tej metodzie i powinna być czasownikiem  albo zaczynać się od czasownika. Biorąc pod uwagę warunki zadania, nazwałbym metodę getLengthOfLastWord.

3. Kilka rekomendacji dot. nazewnictwa metod:

  • Jeżeli metoda zwraca dane typu boolean jako wynik stosowania metody, wtedy warto zacząć nazwę od czasowników, które na wszelkiego rodzaju pytania odpowiadają tylko „tak” lub „nie”. Na przykład: hasSomething(), isSomething(), canDoSomething().
  • Metody, które niczego nie zwracają, nazywaj w ten sposób, by nazwa zawierała opis działania. Na przykład, metodę, która będzie weryfikowała indeks tablicy, warto nazwać void checkIndex(int index). Jeżeli trzeba zweryfikować ważność danych, to wtedy void validate(User user), ale nie void isValid(User user). Jeżeli chcesz nazwać metodę isValid(User user), warto zwracać typ danych boolean, jako wynik stosowania metody boolean isValid(User user).

4. Prawie nieinformatywna nazwa parametru wejściowego s, dlatego zmieniłbym nazwę na input. Dlaczego nie inputString? Dlatego że Java to silnie typowany język oprogramowania i dodawanie typu zmiennej do nazwy nie ma sensu. Na przykład:

a) int[] intArr zmieniłbym na int[] numbers.
b) List<User> userList nazwałbym List<User> users.

5. Odstępy przed i po nawiasie. Zgodnie z Google Style guide:
a. Line break after the opening brace.
b. Line break before the closing brace.

Dodatkowo pamiętamy o stawianiu spacji.

6. Indentations. В Google style guide odstępy wynoszące 2 spacji (w Intellij Idea, na przykład, wynoszą 4 spacji).

Po refaktoryzacji pośredniej nasz kod może wyglądać następująco:

class Solution {
  public int getLengthOfLastWord(String input) {
    return null;
  }
}

Teraz przeanalizujmy wykonanie.

Wybrałem jedno z rozwiązań tego zadania z Leetcode i wyglądało ono jak niżej:

class Solution {
    public int getLengthOfLastWord(String s) {

     
        String res = "";
        int n = s.length()-1;
        if(s.charAt(0) == ' ' && s.length() == 1 ) return 0;

        for(int i = n ; i >= 0;i--){
           if(s.charAt(i) != ' '){
               res = res + s.charAt(i);
           }
           if(res.length() > 0 && s.charAt(i) == ' ') break;
        }
return res.length();
    }

}


Przeanalizujmy problemy:

1.     Wiersz 2: indentation 4 spacje zamiast 2 (P.S. w niniejszym przykładzie powołuję się na Google Style guide. W Twoim projekcie spokojnie mogą być odstępy wynoszące 4, 6, 8 spacji)

2.     Wiersz 2: nieinformatywna nazwa zmiennej s.

3.     Wiersze 3 i 4. Dwa puste wiersze z rzędu. Z Google Style guide: Multiple consecutive blank lines are permitted, but never required (or encouraged).

4.     Wiersz 5. Nazwa zmiennej String res = «„; jest niezbyt informatywna. Lepiej nie skracać nazw. Lepiej dodaj 2-3 symbole, aby zwiększyć czytelność kodu. Na przykład, String result = “»; dodatkowo takie rozwiązanie nie jest optymalne pod kątem wykonania, lepiej posługiwać się StringBuilder zamiast konkatenacji wierszy.

5.     Wiersz 6: int n = s.length()-1; brakuje spacji po i przed znakiem -.

6.     Wiersz 6: generalnie tymczasowa zmienna n nie jest nam potrzebna.  Tak naprawdę tu popełniono kilka błędów:
а) nieinformatywna nazwa zmiennej n;
b) zmienna została zadeklarowana dość daleko od miejsca jej użytkowania (a to wiersz 9). Zalecane jest deklarowanie i inicjalizacja zmiennych jak najbliżej miejsca ich pierwszego użycia. W takim przypadku zmienną n warto by było zadeklarować w wierszu 8, ponieważ pierwsze (i jedyne) miejsce używania zmiennej to wiersz 9;

7.     Wiersz 7 i wiersz 13 – dwie instrukcje w jednym wierszu. Powinna być jedna instrukcja w wierszu. Ponadto, return 0; i break; wymagane są nawiasy otaczające. Patrz przykład tutaj.

8.     Wiersze 7, 9, 10, 13 brakuje spacji między if i ( lub między for i (. Tak samo między ) i {.

9.     Wiersz 10: co to takiego ` `? Jest to separator. Więc nazwijmy tę zmienną w oparciu o to. Jednak nie będzie to zmienna, tylko konstanta. W przypadku konstant obowiązują osobne zasady nazewnictwa.
Po refaktoryzacji pośredniej nasz kod może wyglądać następująco:

class Solution {
 public static final char WORDS_SEPARATOR = ' ';

 public int getLengthOfLastWord(String s) {
   StringBuilder result = new StringBuilder();
   if (s.charAt(0) == WORDS_SEPARATOR && s.length() == 1) {
     return 0;
   }

   for (int i = s.length() — 1; i >= 0; i--) {
     if (s.charAt(i) != WORDS_SEPARATOR) {
       result.append(s.charAt(i));
     }
     if (result.length() > 0 && s.charAt(i) == WORDS_SEPARATOR) {
       break;
     }
   }
   return result.length();
 }
}


Ponieważ wykonanie jest bardzo ważne dla nas, takie rozwiązanie można by było usprawnić, usuwając StringBuilder, ponieważ konkatenacja symboli nam nie jest potrzebna. Powinniśmy wiedzieć, jaka jest długość słowa, a nie samo słowo. Zatem nasze rozwiązanie może wyglądać jak poniżej:

class Solution {
 public static final char WORDS_SEPARATOR = ' ';

 public int getLengthOfLastWord(String s) {
   if (s.charAt(0) == WORDS_SEPARATOR && s.length() == 1) {
     return 0;
   }

   int wordLength = 0;
   for (int i = s.length() — 1; i >= 0; i--) {
     if (s.charAt(i) != WORDS_SEPARATOR) {
       wordLength++;
     }
     if (wordLength > 0 && s.charAt(i) == WORDS_SEPARATOR) {
       break;
     }
   }
   return wordLength;
 }
}

Wykonanie tego rozwiązania jest całkiem dobre:

  • Czasowo:
  • Pamięciowo:

P.S. Aby rozwiązanie zostało dopasowane do Leetcode, zmieniłem nazwę na lengthOfLastWord. Ponieważ w przypadku Leetcode mogą być stosowane inne zasady nazewnictwa metod 🙂.

Generalnie wykonanie jest dla nas ważne, ale chcielibyśmy zwiększyć czytelność kodu. Alternatywne rozwiązanie (krótsze i bardziej eleganckie) może wyglądać jak niżej:

class Solution {
 public int getLengthOfLastWord(String input) {
   String[] arr = input.trim().split(" ");
   return arr[arr.length — 1].length();
 }
}

W niniejszym rozwiązaniu nie podobają mi się poniższe elementy:

1.     Nazwa zmiennej String[] arr. Nieinformatywna nazwa. Zupełnie inaczej to wygląda, nazwa wskazuje cel, do czego służy. Podany kod `String[] arr` wskazuje na to, że  jest to łańcuch. Zatem tak nazwijmy tę zmienną: String[] words.

2.     Tu mamy separator w postaci <code style="background:none;font:inherit;padding:0;">” “. To też warto poprawić.

Po refaktoryzacji nasz kod może wyglądać następująco:

class Solution {
 private static final String WORDS_SEPARATOR = " ";

 public int getLengthOfLastWord(String input) {
   String[] words = input.trim().split(WORDS_SEPARATOR);
   return words[words.length — 1].length();
 }
} 

Oczywiście pod względem pamięci przegramy z poprzednim rozwiązaniem, ale ono jest bardziej zwięzłe. Dlatego tutaj, jak wszędzie, powinniśmy mieć świadomość, że zawsze coś za coś. Akurat w tym przypadku ja bym poświęcił wyniki na rzecz lakoniczności.

Generalnie powyższe rozwiązanie jest dość zrozumiałe i uważam, że taki kod może znaleźć się w gałęzi głównej (master).

Reasumując

  • Code style odgrywa ważną rolę w projekcie. Występowanie jasnych zasad pisania kodu zwiększa jego czytelność i ułatwia utrzymanie.
  • Posługuj się gotowymi style guides lub twórz własne.
  • Zautomatyzuj proces weryfikowania zasad, opisanych w style guides i w trakcie pisania kodu (za pomocą ustawień w IDE) i podczas CI.
  • Automatyzacja  nie zadziała w 100% przypadków, dlatego dokonaj przeglądu kodu. Zwracaj uwagę na nazewnictwo pól, metod, klas. Miej zawsze z tyłu głowy, że nazewnictwo zawsze ma wskazywać na kontekst zadania, nad którym pracujesz.

P. S. W artykule nie umieściłem linku do Clean Code od Boba Martina, ale rekomenduję przeczytać, jeżeli tego jeszcze nie zrobiłeś. Dodatkowo tu możesz zapoznać się z krótkim podsumowaniem głównych punktów. Mam nadzieję, że to przyda ci się wielokrotnie 

Gru 6
Rekrutacja i okres próbny w Netflix: doświadczenia inżyniera UI
Vlad Kampov niedawno zakończył okres próbny w Netflix jako inżynier UI i podzielił się z nami swoimi pierwszymi przemyśleniami na temat pracy i kultury, na której opiera się Netflix.
Kwi 17
Twórcy "Pierwszego inżyniera oprogramowania AI" oskarżeni o kłamstwo
Autorka blogu 80lv przeanalizowała, jak Devin rozumie wysłane mu zadania i oto, co znalazła. Publikujemy przetłumaczony blog.
Paz 25
Mobile Trends for Experts w Warszawie: o trendach i wpływie wojny na branżę aplikacji mobilnych
25 i 26 października w Warszawie odbywa się jedna z największych konferencji poświęconych branży mobile w Polsce - Mobile Trends for Experts. To ogólnokrajowe wydarzenie dla przedstawicieli sektora mobile, którzy szukają okazji do networkingu oraz zdobycia cennej na rynku wiedzy.

Ta strona używa plików cookie, aby zapewnić Ci lepsze wrażenia podczas przeglądania.

Dowiedz się więcej o tym, jak używamy plików cookie i jak zmienić preferencje dotyczące plików cookie w naszej Polityka plików cookie.

Zmień ustawienia
Zapisz Akceptuj wszystkie cookies