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
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920070/twitter-xxl.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920092/5328e4296f63cd592700018f_globe-icon.png)
itcraftsman.pl
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035686/DSC_0339.jpg)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035688/zce-php5-3-logo.gif)
Zend Certified Architect
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035692/zf-zce-logo.gif)
- 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 ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920098/code-review.png)
Co to jest Code Review ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1980516/Henry_Oldenburg.jpg)
Henry Oldenburg, sekretarz Royal Society, 1665
Co to jest Code Review ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035722/fagan.jpg)
Fagan Inspection Process
Michael Fagan
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035725/fagan-inspections.jpg)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035726/dscf1368-1024x768.jpg)
Co to jest Code Review ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035730/agile-manifesto.jpg)
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 ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2035760/pull-request.png)
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
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036379/tests-fails.png)
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
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036375/tests-success.png)
Pull/merge request
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036316/push-to-branch.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036327/git-pull-request-buttons.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036335/open-pull-request.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036342/pull-request-changes.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036385/pull-request-list.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036388/comments.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042020/comments-zoom1.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042022/comments-zoom2.png)
Pull/merge request
public function mostRepeatedValue(array $array)
{
$counts = array_count_values($array);
arsort($counts);
return key($counts);
}
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036375/tests-success.png)
Pull/merge request
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036396/pull-request-after-refactor.png)
Pull/merge request
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2043527/split-view.png)
Pull/merge request
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036402/accept-pull-request.png)
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
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042034/bad-example.png)
Na co zwracać uwagę ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036761/code.jpg)
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 ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036893/req_trees.png)
Na co zwracać uwagę ?
Czy istnieją testy automatyczne pokrywające przeglądany kod ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2036912/cowboy.png)
Na co zwracać uwagę ?
Czy przeglądany kod stosuje się do ustalonego stylu ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042039/mandlebroat.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042040/1280px-Mandel_zoom_00_mandelbrot_set.jpg)
Na co zwracać uwagę ?
Czy sam zrobiłbym coś inaczej ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037010/knowledge-sharing-image-cropped_10.jpg)
Co na tym zyskam ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1980459/benef2.jpg)
Co na tym zyskam ?
Lepsza komunikacja z zespołem
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037479/Team-Communication-bigstock-25539350.jpg)
Co na tym zyskam ?
Polepszanie jakości kodu
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037059/56547624.jpg)
Co na tym zyskam ?
Łatwiejsze wdrożenie nowego programisty
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037068/3418243489_2696f5fdb4.jpg)
Co na tym zyskam ?
Wcześniejsze wykrywanie błędów
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037074/its_not_a_bug_its_a_feature_by_forester_joe.jpg)
Co na tym zyskam ?
Dotrzymywanie terminów
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037085/vdc4g4.jpg)
Jakie są zagrożenia ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1980637/dangerous.jpg)
Jakie są zagrożenia ?
Brak widocznego kontekstu zmian
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037103/context.png)
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
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037138/knowledge.gif)
Jakie są zagrożenia ?
Mniejsze poczucie odpowiedzialności
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2037158/exchange-2013-500-error.png)
"przecież mi zaakceptowałeś ten kodzik"
Opór przed wdrożęniem
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/2042095/kurve.jpg)
Pytania ?
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920108/57840152.jpg)
Dziękuję za uwagę
@ ArkadiuszKondas
itcraftsman.pl
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920070/twitter-xxl.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920092/5328e4296f63cd592700018f_globe-icon.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/341527/images/1920105/dziekuje-za-uwage.png)
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,515