Praktyczne Code Review
ARKADIUSZ KONDAS
Backend Team Leader
@ Da Vinci Studio
Zend Certified Engineer
Code Craftsman
Blogger
Ultra Runner
WeBB MeetUp Autumn
09.12.2015
@ ArkadiuszKondas


itcraftsman.pl


Zend Certified Architect

- Co to jest Code Review ?
- Jak to wygląda w praktyce ?
- Na co zwracać uwagę ?
- Zyski i zagrożenia ?
Agenda:
Co to jest Code Review ?

Co to jest Code Review ?

Henry Oldenburg, sekretarz Royal Society, 1665
Co to jest Code Review ?

Fagan Inspection Process
Michael Fagan


Co to jest Code Review ?

Agile Manifesto
http://agilemanifesto.org
Przeglądanie lekkie:
- do 200-400 wierszy kodu
- do 300-500 wierszy na godzinę
- do 60-90 minut
- nie ma za małych zmian
Jak to wygląda w praktyce ?

Pull/merge request
Pull/merge request
<?php
class ArrayUtils
{
    /**
     * @param array $array
     *
     * @return mixed
     */
    public function mostRepeatedValue(array $array)
    {
    }
}
Pull/merge request
class MostRepeatedValueTest extends \PHPUnit_Framework_TestCase
{
    public function testIfItReturnMostRepeatedValueInIntegersArray()
    {
        $array = [1, 3, 4, 5, 1, 3, 1, 7];
        $utils = new ArrayUtils();
        $mostRepeatedValue = $utils->mostRepeatedValue($array);
        $this->assertEquals(1, $mostRepeatedValue);
    }
    public function testIfItReturnMostRepeatedValueInStringArray()
    {
        $array = ['Apple', 'Orange', 'Plum', 'Apple', 'Grape', 'Apple'];
        $utils = new ArrayUtils();
        $mostRepeatedValue = $utils->mostRepeatedValue($array);
        $this->assertEquals('Apple', $mostRepeatedValue);
    }
}Pull/merge request

Pull/merge request
public function mostRepeatedValue(array $array)
{
    $counts = [];
    foreach ($array as $item) {
        $item = (string) $item;
        if (!isset($counts[$item])) {
            $counts[$item] = 1;
        } else {
            ++$counts[$item];
        }
    }
    $mostRepeatedCount = 0;
    $mostRepeated = null;
    foreach ($counts as $key => $count) {
        if ($count > $mostRepeatedCount) {
            $mostRepeatedCount = $count;
            $mostRepeated = $key;
        }
    }
    return $mostRepeated;
}
Pull/merge request

Pull/merge request








Pull/merge request
public function mostRepeatedValue(array $array)
{
    $counts = array_count_values($array);
    arsort($counts);
    return key($counts);
}
Pull/merge request

Pull/merge request

Pull/merge request

Przydatne wskazówki
- Sprawdź swój pull request
- Nie wywyższaj się
- Nie formułuj bezsensownych żądań
- Szanuj partnerów
- Ucz innych
- Ucz się od innych
Przydatne wskazówki

Na co zwracać uwagę ?

Na co zwracać uwagę ?
Czy rozumiem co przeglądany kod
ma robić ?
function same(aArr, bArr)
{
  return [].concat.apply([],aArr).sort().toString() 
    === [].concat.apply([],bArr).sort().toString();
}Na co zwracać uwagę ?
 
Czy istnieją jakieś oczywiste błędy logiczne w kodzie ?
public function hasCorrectAge($user)
{
    return $user->getAge() = 18;
}// Yoda notation
public function hasCorrectAge($user)
{
    return 18 == $user->getAge();
}Na co zwracać uwagę ?
Czy spełnione zostały wymagania ?

Na co zwracać uwagę ?
Czy istnieją testy automatyczne pokrywające przeglądany kod ?

Na co zwracać uwagę ?
Czy przeglądany kod stosuje się do ustalonego stylu ?


Na co zwracać uwagę ?
Czy sam zrobiłbym coś inaczej ?

Co na tym zyskam ?

Co na tym zyskam ?
Lepsza komunikacja z zespołem

Co na tym zyskam ?
Polepszanie jakości kodu

Co na tym zyskam ?
Łatwiejsze wdrożenie nowego programisty

Co na tym zyskam ?
Wcześniejsze wykrywanie błędów

Co na tym zyskam ?
Dotrzymywanie terminów

Jakie są zagrożenia ?

Jakie są zagrożenia ?
Brak widocznego kontekstu zmian

Jakie są zagrożenia ?
Osoba sprawdzająca z poza projektu, brak wiedzy na temat domeny
/**
 * @throws EmptyVesselException
 */
public function emptyTheContent()
{
    if ($this->isEmpty()) {
        throw new EmptyVesselException();
    }
    
    $this->filledWith = null;
    $this->ingredients = [];
    $this->currentCapacity = new Capacity(0);
}Jakie są zagrożenia ?
Różny poziom osób sprawdzających kod

Jakie są zagrożenia ?
Mniejsze poczucie odpowiedzialności

"przecież mi zaakceptowałeś ten kodzik"
Opór przed wdrożęniem

Pytania ?

Dziękuję za uwagę
@ ArkadiuszKondas
itcraftsman.pl



Praktyczne Code Review
By Arkadiusz Kondas
Praktyczne Code Review
Code Review czyli inspekcja kodu. Chcę odpowiedzieć na pytania: co to takiego code review ? dlaczego warto robić code review ? oraz jak zrobić to dobrze ?
- 1,833
 
   
   
  