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 ?

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

<?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

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

public function mostRepeatedValue(array $array)
{
    $counts = array_count_values($array);
    arsort($counts);

    return key($counts);
}

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ę ?

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 ?

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 ?

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