Praktyczne code reviews

Sebastian Marek -

Internal Systems Technical Architect

@proofek

!• ponad  12  lat  doświadczenia  w  

programowaniu  • zwolennik  automatyzacji  

procesów  • TDD  i  CI  • w  miarę  wolnego  czasu  

wspiera  open  source

h"ps://joind.in/9759

https://www.codeclub.org.uk/

Ochotnik Projekt Miejsce Dzieci

+ + +

Światowa sieć klubów programistycznych dla dzieci od 9 do 11 roku życia

Dla kogo?

Na początek

Wszystkie wydarzenia i postacie w tej prezentacji są fikcyjne.

!Jakiekolwiek podobieństwo jest całkowicie przypadkowe.

Zespół

Stefan “Ma być zrobione” –  Kierownik

Roman “Potrzebuję to na wczoraj” –  Właściciel firmy

Zespół

“Nocny Marek” – programista

Krzysiek “Haker” –  doświadczony programista

Paweł “Będzie dobrze” – praktykant

Sytuacja 1

Jak długo zajmie wam skończenie tego projektu?

Hmm, projekt, programowanie, code review, testy…

Code review? A po co code review? Co, programować nie umiecie? Poza tym przecież będą testy...?

Sytuacja 2

Hmmm… ale wszyscy mają kupę roboty! Nie ma nikogo wolnego. Dajmy sobie spokój i puśćmy to prosto do testów…

Ok. Prawie gotowe! Jeszcze tylko code review...

Sytuacja 3

Cześć Stefan, Potrzebuję Marka, żeby zrobił mi to code review.

Marek pracuje nad czymś ważnym teraz, ale Paweł może na to spojrzeć.

Ale Paweł jest stażystą i nie zna jeszcze tego systemu...

Chcesz mieć to code review zrobione czy nie?! Mam tylko Pawła.

Sytuacja 4

Ciągle robimy te code review, patrzymy na ten kod, spędzamy nad tym masę czasu, a za każdym razem jak wypuszczamy nowy produkt mamy problemy. Marnujemy tylko czas!

Code review

Marek - programistado  Krzysiek  -­‐  inspektor

21:31 (0 minut temu)

Krzysiek,  !Możesz proszę spojrzeć na to? Właśnie skończyłem nad tym pracować.  Zmiany są w moim repozytorium w gałęzi “problem-naprawiony”.  !Dzięki  !---  Marek

Kliknij tutaj, aby Odpowiedz or Przekaż dalej

Prośba o code review

Tylko  nie  (e)mail!

Jak poprosić?

Systemy  śledzenia  błędów  -­‐ JIRA  -­‐ Bugtrak  -­‐ ManFs  !Narzędzia  -­‐ Crucible/Fisheye  -­‐ Gerrit  -­‐ Github  -­‐ Phabricator

Code review

21:31 PM (13 minut temu)

Krzysiek - Inspektordo  Marek  -­‐  programista

21:44 (0 minut temu)

Marek,  !Spoko, tylko na bazie jakiej gałęzi stworzyłeś tą gałąź?  Bez tego nie dam rady zidentyfikować zestawu zmian.  !---  Krzysiek

Marek - programistado  Krzysiek  -­‐  inspektor

Krzysiek,  !Możesz proszę spojrzeć na to? Właśnie skończyłem nad tym pracować.  Zmiany są w moim repozytorium w gałęzi “problem-naprawiony”.  !Dzięki  !---  Marek

Kliknij tutaj, aby Odpowiedz or Przekaż dalej

Na co zwracać uwagę?

Systemy  kontroli  wersji  • konkretne  zestawy  zmian  (changesets)  

• unikanie  konkretnych  commitów  

• inspekcja  łatek  (patches)  ryzykowna,  chyba  że  zautomatyzowana

Code review

21:31 (25 minut temu)

Krzysiek, Możesz proszę an to spojrzeć? Właśnie skończyłem nad tym pracować…

21:44 (12 minut temu)

Marek,  !Spoko, tylko na bazie jakiej gałęzi stworzyłeś tą gałąź?  Bez tego nie dam rady zidentyfikować zestawu zmian.  !---  Krzysiek

21:56 PM (0 minut temu)

Krzysiek,  !No tak, przepraszam. Gałąź stworzyłem na podstawie mastera.  !---  Marek

Marek - programista

Krzysiek - Inspektordo  Marek  -­‐  programista

Marek - programistado  Krzysiek  -­‐  inspektor

Po co?

Co i !

dlaczego?

Tak też można…

Paweł  “Będzie  dobrze”

• ma  sens  • działa  • poprawne  syntaktycznie  • ok

Tradycyjne  komentarze:

Ale można i tak…

Krzysiek  “Doświadczony  Inspektor”

• PHP  linter  • PHP  Code  Sniffer  • PHPUnit  • phpDocumentor  • PHP  Depend  • PHP  Mess  Detector  • SonarQube

Używane  narzędzia:

Automatyzacja - PHP linter i PHP CodeSniffer

$ php -l Libraries/Action.class.php  No syntax errors detected in Libraries/Action.class.php

$ php -l Libraries/Action.class.php  Errors parsing Libraries/Action.class.php

$ phpcs –standard=Zend Libraries/Action.class.php  !FILE: /Volumes/git/modules/AccountChange/Libraries/Action.class.php  --------------------------------------------------------------------------------  FOUND 2 ERROR(S) AND 1 WARNING(S) AFFECTING 3 LINE(S)  --------------------------------------------------------------------------------   44 | ERROR | Protected member variable "arrOptions" must contain a leading   | | underscore   66 | WARNING | Line exceeds 80 characters; contains 82 characters   97 | ERROR | Line exceeds maximum limit of 120 characters; contains 135   | | characters  --------------------------------------------------------------------------------  !Time: 0 seconds, Memory: 5.75Mb

Unit testy z PHPUnit

$ phpunit  PHPUnit 3.6.12 by Sebastian Bergmann.  !Configuration read from phpunit.xml.dist  !..................IIII................IIIIIIIIIIIIIIIIIIIIIII.. 63 / 240 ( 26%)  .............................................I.....I........... 126 / 240 ( 52%)  ............................................................... 189 / 240 ( 78%)  ...................................................  !Time: 02:01, Memory: 26.75Mb

OK, but incomplete or skipped tests!  Tests: 240, Assertions: 514, Incomplete: 29.

Statyczna analiza kodu z PHP Depend

PHP_Depend 0.10.6 by Manuel Pichler  !Parsing source files:  .................... 20  !Executing CyclomaticComplexity-Analyzer:  ............. 261  !Executing ClassLevel-Analyzer:  ............ 247  !Executing CodeRank-Analyzer:  . 28  !Executing Coupling-Analyzer:  ............. 267  !Executing Hierarchy-Analyzer:  ............ 246  !Executing Inheritance-Analyzer:  . 30  !Executing NPathComplexity-Analyzer:  .............. 283  !Executing NodeCount-Analyzer:  ........ 174  !Executing NodeLoc-Analyzer:  .......... 205  !Generating pdepend log files, this may take a moment.  !Time: 00:05; Memory: 25.50Mb

Statyczna analiza kodu z PHP Mess Detector

Statyczna analiza kodu z SonarQube

Statyczna analiza kodu z SonarQube

Statyczna analiza kodu z SonarQube

Statyczna analiza kodu z SonarQube

Statyczna analiza kodu z SonarQube

Statyczna analiza kodu z SonarQube

…zwraca uwagę na najważniejsze rzeczy

Sprawdza:• przejrzystość  kodu  • wydajność  • nadmierna  złożoność  (complexity)  

• wpływ  na  inne  systemy  • czy  problem  został  rozwiązany

• duplikacja  kodu  • jakość  kodu  • problemy  z  wdrażaniem  systemu  (deployment)  

• niedociągnięcia  na  etapie  projektowania  (soZware  design)

Krzysiek  “Doświadczony  Inspektor”

Największa zaleta – korzystają na tym wszyscy!

• Przekazywanie  wiedzy  • Szkolenie  nowych/początkujących  

programistów  • Wczesne  wykrywanie  błędów  i  

problemów    • Poprawa  jakości  kodu  • Promowanie  zbiorowej  

odpowiedzialności  za  kod  

Relacje międzyludzkie - programiści

• Zaakceptuj  fakt,  że  każdy  popełnia  błędy  (ty  też!)  !

• “dziurawy”  kod  !=  kiepski  programista  !

• nieważne  jak  dużo  wiesz,  zawsze  znajdzie  się  ktoś  kto  wie  więcej  !

• Nie  przepisuj  kodu  bez  wcześniejszych  uzgodnień  

PRO

GR

AM

IŚC

I

Relacje międzyludzkie – inspektorzy

• Prawdziwy  autorytet  jest  poparty  wiedzą  i  doświadczeniem,  a  nie  stanowiskiem  !

• Krytykuj  kod  a  nie  ludzi

INSP

EKTO

RZY

Podsumowanie - co zawrzeć w code review

• Gdzie  szukać  kodu  – Repozytorium,  nazwa  gałęzi,  podstawa  gałęzi  

• Opis  zmiany  – Co  zostało  zmienione?  

• Powód  zmiany  – Dlaczego  kod  został  zmieniony?

CO

?

Podsumowanie - kto powinien przeprowadzić inspekcję kodu?

• Pytaj  ekspertów  – W  razie  wątpliwości  proś  o  pomoc    

• KwesFonuj  implementację  – Upewnij  się,  że  problem  został  właściwie  

rozwiązany

KTO

?

Podsumowanie – jak zgłosić code review?

• Historia  zmian  – systemy  śledzenia  błędów,  np.  Jira,  Trac,  ManFs,  itd.  – narzędzia  do  inspekcji  kodu,  np.  Fisheye/Crucible,  gerrit,  

Phabricator    • Rozmowa/Programowanie  w  parach  

– upewnij  się,  że  komentarze  są  udokumentowane

GD

ZIE?

Podsumowanie - jak przeprowadzić dobrą inspekcję?

• Używaj  narzędzi,  automatyzuj  • Zwracaj  uwagę  na  duplikację  i  zbyt  skomplikowany  kod  

• Oceń  wpływ  na  inne  systemy  • Upewnij  się,  że  kod  jest  dobrze  udokumentowany  i  łatwy  do  zrozumienia

JAK

?

Podziękowania…

hgp://georgegant.deviantart.com/art/Angry-­‐Nerds-­‐217554774  hgp://www.flickr.com/photos/dawgbyte77/3058349367/  hgp://www.flickr.com/photos/zzpza/3269784239/  hgp://www.flickr.com/photos/toolmanFm/6170448143/  hgp://www.flickr.com/photos/coyau/7630782996/  hgp://www.flickr.com/photos/73885983@N02/6729908421/  hgp://www.osnews.com/story/19266/WTFs_m

hgp://www.atlassian.com/angrynerds/

…i dodatkowe materiały

The  Ten  Commandments  of  Egoless  Programming:  hgp://alturl.com/q4dpa  !The  Code  review:  hgp://www.soulbroken.co.uk/blog/2010/07/the-­‐code-­‐review/    !Fisheye/Crucible:  hgp://www.atlassian.com/soZware/crucible/overview  !Gerrit:  hgp://code.google.com/p/gerrit/  !Github:  hgps://github.com/  !Phabricator:  hgp://phabricator.org  !PHPUnit:  hgp://phpunit.de    !PHP  CodeSniffer:  hgp://pear.php.net/PHP_CodeSniffer    !PHP  Depend:  hgp://pdepend.org/    !PHP  Mess  Detector:  hgp://phpmd.org/    !SonarQube:  hgp://www.sonarqube.org/  

Pytania

Pytania?

h"ps://                          /9759