Upgrade to Pro
— share decks privately, control downloads, hide ads and more …
Speaker Deck
Features
Speaker Deck
PRO
Sign in
Sign up for free
Search
Search
There's more to code review than you might think
Search
Daniel Shaw
October 21, 2017
Programming
1
230
There's more to code review than you might think
Given at DrupalCamp Dublin 2017
Daniel Shaw
October 21, 2017
Tweet
Share
More Decks by Daniel Shaw
See All by Daniel Shaw
Educating Enfys (lightning talk)
thatdamnqa
0
180
There's More to Code Reviews than You Might Think ✩
thatdamnqa
0
93
There's more to code review than you might think
thatdamnqa
0
150
There's More To Code Reviews Than You Might Think (lightning talk)
thatdamnqa
0
170
The Less Dull Bits of Testing
thatdamnqa
0
150
Other Decks in Programming
See All in Programming
incrementalモデルの理解を深める
ikkimiyazaki
2
640
WasmOS: Wasmを実行する自作Microkernel
riru
0
370
自動テスト実行結果の目的を整理する / Organizing objectives of automated test results
twada
PRO
10
2.1k
脱・初心者!脱・マネコン!AWS CDKを使ってみませんか!?
har1101
0
160
OpenTelemetry のサービスという概念について
azukiazusa1
1
410
CircleCIを活用して AWSへの継続的デリバリーを 実践する
coconala_engineer
1
110
受託開発でGitLab CI を活用していく
xiombatsg
1
130
生成 AI の中身を覗いてみよう〜基礎から医療現場での応用まで〜
soh9834
2
770
品質が高いコードって何?Rev2.1
ickx
1
490
両面どころかインフラもTSでできるよ ~ 全方位TypeScriptによるプロダクト開発 ~
myfinder
9
2.4k
実践!RDRAを活用した既存システムの仕様変更 / Specification Changes in Existing Systems Utilizing RDRA
imamotohikaru
0
2.7k
htmx is fun!
codehex
2
190
Featured
See All Featured
It's Worth the Effort
3n
180
27k
Building an army of robots
kneath
300
41k
Ruby is Unlike a Banana
tanoku
95
10k
Fashionably flexible responsive web design (full day workshop)
malarkey
397
65k
Code Review Best Practice
trishagee
54
15k
What the flash - Photography Introduction
edds
64
11k
JazzCon 2018 Closing Keynote - Leadership for the Reluctant Leader
reverentgeek
178
11k
Building Better People: How to give real-time feedback that sticks.
wjessup
350
18k
Building Adaptive Systems
keathley
29
1.8k
Writing Fast Ruby
sferik
619
59k
How STYLIGHT went responsive
nonsquared
92
4.7k
Easily Structure & Communicate Ideas using Wireframe
afnizarnur
185
15k
Transcript
THERE’S MORE TO CODE REVIEW THAN YOU MIGHT THINK Daniel
Shaw @thatdamnqa · http://thatdamnqa.com
@thatdamnqa What are Code reviews for?
@thatdamnqa Code reviews are not an indication of anybody's abilities
@thatdamnqa ☐ Check code style ☐ Review the configuration ☐
Check forwards compatibility ☐ Review the documentation ☐ Review the commit message ☐ Double check the code ☐ Double check the tests Code review checklist
@thatdamnqa Check code style
@thatdamnqa Check code style Pick a house style, use it,
communicate it
@thatdamnqa Check code style Pick a house style, use it,
communicate it
@thatdamnqa Check code style Don't get too distracted by checking
code style
@thatdamnqa Check code style Automate if you can. Bad news
can be better received by a cruel and lifeless script
@thatdamnqa Review the configuration
@thatdamnqa Review the configuration { "require": { "usefultool/tool": "1.1" }
}
@thatdamnqa Review the configuration { "require": { "usefultool/tool": "1.2.1" }
}
@thatdamnqa Review the configuration { "require": { "usefultool/tool": "^1.2" }
}
@thatdamnqa Check forwards compatibility
@thatdamnqa Check forwards compatibility PHP 5.6 PHP 7.0 PHP 7.1
@thatdamnqa Check forwards compatibility jQuery 1 jQuery 2
@thatdamnqa Check forwards compatibility jQuery 1 jQuery 2 jQuery 3
@thatdamnqa
@thatdamnqa Review the documentation
@thatdamnqa Review the documentation Ensure changes make sense, and are
correct
@thatdamnqa Review the documentation Make sure code changes are documented
@thatdamnqa Review the commit message
@thatdamnqa Review the commit message Update usefultool.
@thatdamnqa Review the commit message Update UsefulTool lib Wider company
policy dictates that the latest UsefulTool v1 should be used. Update Composer config to reflect this. [MYCOOLAPP-1234]
@thatdamnqa Double check the code
@thatdamnqa Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets();
} else { $this->accessDenied(); }
@thatdamnqa Double check the code $permitted = ['2', '4', '5',
'cake']; $input = 0; if (in_array($input, $permitted)) { echo "I'm in!"; }
@thatdamnqa Double check the code Is the code testable?
@thatdamnqa Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets();
} else { $this->accessDenied(); }
@thatdamnqa Double check the code function is_allowed_access($input, $expected) { if
(in_array($input, $expected), true) { return true; } else { return false; } }
@thatdamnqa Double check the code function is_allowed_access($input, $expected) { if
(in_array($input, $expected), true) { return true; } else { return false; } } function testCheckAccess() { $input = 2; $expected = ['2', '4', '5']; assertTrue( $class->is_allowed_access( $input, $expected ); }
@thatdamnqa Double check the tests
@thatdamnqa Double check the tests Test for failures too
@thatdamnqa Double check the tests function is_allowed_access($input, $expected) { if
(in_array($input, $expected), true) { return true; } else { return false; } } function testCheckAccess() { $input = 2; $expected = ['2', '4', '5']; assertTrue( $class->is_allowed_access( $input, $expected ); }
@thatdamnqa Double check the tests function testCheckAccess() { $input =
2; $expected = ['2', '4', '5']; assertTrue( $class->is_allowed_access( $input, $expected ); } function testCheckAccess_isStrict() { $input = 0; $expected = ['2', '4', '5', 'cake']; assertFalse( $class->is_allowed_access( $input, $expected ); }
@thatdamnqa Final thoughts…
@thatdamnqa Final thoughts… All output should be reviewed. It’s not
personal
@thatdamnqa Final thoughts… Often, a peer review can teach something
to two people
@thatdamnqa Introducing code reviews
@thatdamnqa Introducing Code Reviews Step 1: Keep watching
@thatdamnqa Introducing Code Reviews Step 1: Keep watching Step 2:
Ask questions
@thatdamnqa Introducing Code Reviews Step 1: Keep watching Step 2:
Ask questions Step 3: Go to step 1
@thatdamnqa
@thatdamnqa ☑ Check code style ☑ Review the configuration ☑
Check forwards compatibility ☑ Review the documentation ☑ Review the commit message ☑ Double check the code ☑ Double check the tests Code review checklist
Thanks.
[email protected]
@thatdamnqa