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 Reviews than You Might Thi...
Search
Daniel Shaw
October 14, 2016
Programming
0
110
There's More to Code Reviews than You Might Think ✩
Given at Re:Develop conference, October 2016.
Daniel Shaw
October 14, 2016
Tweet
Share
More Decks by Daniel Shaw
See All by Daniel Shaw
There's more to code review than you might think
thatdamnqa
1
320
Educating Enfys (lightning talk)
thatdamnqa
0
260
There's more to code review than you might think
thatdamnqa
0
270
There's More To Code Reviews Than You Might Think (lightning talk)
thatdamnqa
0
270
The Less Dull Bits of Testing
thatdamnqa
0
250
Other Decks in Programming
See All in Programming
テストコードはもう書かない:JetBrains AI Assistantに委ねる非同期処理のテスト自動設計・生成
makun
0
320
The Past, Present, and Future of Enterprise Java with ASF in the Middle
ivargrimstad
0
130
為你自己學 Python - 冷知識篇
eddie
1
350
個人軟體時代
ethanhuang13
0
320
はじめてのMaterial3 Expressive
ym223
2
750
詳解!defer panic recover のしくみ / Understanding defer, panic, and recover
convto
0
240
250830 IaCの選定~AWS SAMのLambdaをECSに乗り換えたときの備忘録~
east_takumi
0
390
The Past, Present, and Future of Enterprise Java
ivargrimstad
0
390
Amazon RDS 向けに提供されている MCP Server と仕組みを調べてみた/jawsug-okayama-2025-aurora-mcp
takahashiikki
1
110
CJK and Unicode From a PHP Committer
youkidearitai
PRO
0
110
Compose Multiplatform × AI で作る、次世代アプリ開発支援ツールの設計と実装
thagikura
0
160
「手軽で便利」に潜む罠。 Popover API を WCAG 2.2の視点で安全に使うには
taitotnk
0
860
Featured
See All Featured
Practical Orchestrator
shlominoach
190
11k
Building a Modern Day E-commerce SEO Strategy
aleyda
43
7.6k
YesSQL, Process and Tooling at Scale
rocio
173
14k
The Pragmatic Product Professional
lauravandoore
36
6.9k
Rails Girls Zürich Keynote
gr2m
95
14k
Art, The Web, and Tiny UX
lynnandtonic
303
21k
VelocityConf: Rendering Performance Case Studies
addyosmani
332
24k
The Psychology of Web Performance [Beyond Tellerrand 2023]
tammyeverts
49
3k
Reflections from 52 weeks, 52 projects
jeffersonlam
352
21k
Sharpening the Axe: The Primacy of Toolmaking
bcantrill
44
2.5k
Designing Dashboards & Data Visualisations in Web Apps
destraynor
231
53k
"I'm Feeling Lucky" - Building Great Search Experiences for Today's Users (#IAC19)
danielanewman
229
22k
Transcript
THERE’S MORE TO CODE REVIEW THAN YOU MIGHT THINK Clair
Shaw @clairs · clairshaw.co.uk
What are Code reviews for?
Code reviews are not an indication of anybody's abilities
☐ 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
Check code style
Check code style Pick a house style, use it, communicate
it
Check code style Pick a house style, use it, communicate
it
Check code style Don't get too distracted by checking code
style
Check code style Automate if you can. Bad news can
be better received by a cruel and lifeless script
Review the configuration
Review the configuration { "require": { "usefultool/tool": "1.1" } }
Review the configuration { "require": { "usefultool/tool": "1.2.1" } }
Review the configuration { "require": { "usefultool/tool": "^1.2" } }
Check forwards compatibility
Check forwards compatibility PHP 5.6.26 PHP 7.0.11
Check forwards compatibility jQuery 1 jQuery 2
Check forwards compatibility jQuery 1 jQuery 2 jQuery 3
None
None
Review the documentation
Review the documentation Ensure changes make sense, and are correct
Review the documentation Make sure code changes are documented
Review the commit message
Review the commit message Update usefultool.
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]
Double check the code
Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets(); }
else { $this->accessDenied(); }
Double check the code $permitted = ['2', '4', '5', 'cake'];
$input = 0; if (in_array($input, $permitted)) { echo "I'm in!"; }
Double check the code Is the code testable?
Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets(); }
else { $this->accessDenied(); }
Double check the code function is_allowed_access($input, $expected) { if (in_array($input,
$expected), true) { return true; } else { return false; } }
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 ); }
Double check the tests
Double check the tests Test for failures too
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 ); }
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 ); }
Final thoughts…
Final thoughts… All output should be reviewed. It’s not personal
Final thoughts… Often, a peer review can teach something to
two people
Introducing Code Reviews
Introducing Code Reviews Step 1: Keep watching
Introducing Code Reviews Step 1: Keep watching Step 2: Ask
questions
Introducing Code Reviews Step 1: Keep watching Step 2: Ask
questions Step 3: Go to step 1
None
☑ 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]
@clairs