Code review - jak krytykować

Maciej Iwanowski

Sii Polska

@iwankgb, critical.today, blog

O czym

  • o historii
  • o tym po co
  • o tym jak
  • o problemach
  • o korzyściach
Henry Oldenburg

(XVII w.)

Historia

  • Michael Fagan i IBM
  • Fagan Inspection Process
  • XP i Continunous Inspection
  • Agile Manifesto i jakość

Po co?

  1. komunikacja
  2. wczesne eliminowanie błędów
  3. redukcja liczby błędów zgłaszanych przez klientów
  4. identyfikowanie problemów w procesie

Lekkie przeglądanie

  • SmartBear i Cisco Sytems
  • 50 programistów
  • 2500 przejrzeń
  • 3.2 miliona wierszy kodu

Lekkie przeglądanie

  • przejrzyj własny pull request
  • do 200-400 wierszy kodu
  • do 300-500 wierszy na godzinę
  • do 60-90 minut
  • nie ma za małych zmian

Lekkie przeglądanie

  • 90 minut jest wartością uznaną
  • brak danych do porównania liczby błędów
  • wspomaga częste zmiany

Okiem naukowca

  • przeglądanie wszystkiego to za mało
  • dyskusja jest ważna
  • wiedza ekspercka

Tak rób

  • przygotuj się
  • pisz i przeglądaj testy
  • przeglądaj małe fragmenty
  • używaj narzędzi
  • szukaj defektów

Tak nie rób

Nie bądź ignorantem

Nie bądź ignorantem

Tak nie rób

Nie wywyższaj się

Nie wywyższaj się

Tak nie rób

Nie formułuj bezsensownych żądań

Nie formułuj bezsensownych żądań

Tak nie rób

Szanuj partnerów

Szanuj partnerów

Problemy

  • 20%-30% dłużej
  • nadmiar obowiązków
  • planowanie
  • opór przed nowym i nieznanym
  • opór ze strony menadżerów
  • opór ze strony programistów

Korzyści

accounting
  • komunikacja, głupcze!
  • oszczędności
  • dotrzymywanie terminów
  • efektywniejsze testowanie

Źródła

?

wierzę w testy jednostkowe, testy funkcjonalne,
ciągłą integrację, buildów automatyzację,
generowanie binarek i ich szybkie wdrażanie,
maszyny wirtualne, procesy preforkowalne,
połączenia trwałe, destruktory wspaniałe,
zasobów uwalnianie, zmiennych unsetowanie,
uwielbiam przesłanianie, tęsknię za przeciążaniem

imitowane obiekty już oczekują na testy,
usługi me atomowe namespace'y mają gotowe,
finalne moje klauzule łatają już każdą dziurę,
iteratory czekają, generatory podają,
metody ponotowane już listenerom są znane,
zdarzenia się wyzwalają, kernelom opowiadają,
wszystko się trigeruje, pamięci ciągle brakuje,
incepcja postępująca i tak stepuję bez końca.

Code review - how to criticize

Maciej Iwanowski

About

  • history
  • goals
  • how-to
  • issues
  • benefits
Henry Oldenburg
  • Michael Fagan and IBM
  • Fagan Inspection Process
  • XP and Continunous Inspection
  • Agile Manifesto and quality

What for?

  • communication
  • eliminate bugs early
  • reduce number of bugs reported by customers
  • identifying issues in a process

Lightweight reviewing

  • Cisco Sytems
  • 50 developers, 2500 inspections, 3.2 million lines of code
  • up to 200-400 LoC
  • up to 300-500 LoC per hour
  • up to 60-90 minutes
  • 15 bugs per hour
  • preparing code for inspection

Lightweight reviewing

  • 90 minutes is widely accepted value
  • 6 times more efficient
  • impossible to compare number of bugs per 1000 LoC
  • supports frequent changes

Dos

  • prepare yourself
  • write tests
  • review small changesets
  • use tools
  • look for defects
  • think about a checklist

Don'ts

Don't be ignoramus

Don't be ignoramus

Don'ts

Nie wywyższaj się

Don't swagger

Don'ts

Don't talk rubbish

Don't talk rubbish

Don'ts

Respect partners

Don't disrespect partners

Issues

  • 20%-30% longer development time
  • resist against new and uknown
  • resisting managers
  • resisting developers

Benefits

accounting
  • communication, stupid!
  • saving
  • delivering on time
  • more effective testing

?