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 Think ✩
Search
Daniel Shaw
October 14, 2016
Programming
0
98
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
250
Educating Enfys (lightning talk)
thatdamnqa
0
200
There's more to code review than you might think
thatdamnqa
0
180
There's More To Code Reviews Than You Might Think (lightning talk)
thatdamnqa
0
200
The Less Dull Bits of Testing
thatdamnqa
0
170
Other Decks in Programming
See All in Programming
AWSでゲームサーバーを運用! Amazon GameLiftのお話
iriikeita
0
200
From Spring Boot 2 to Spring Boot 3 with Java 22 and Jakarta EE
ivargrimstad
0
1.9k
I/O Extended Android in Korea 2024 ~ Whats new in Android development tools
pluu
0
250
Play Billing Library 7.0.0 変更点まとめ@potatotips#88
kako351
0
160
Advanced App Shrinking Techniques
cbeyls
2
150
Namespace on read
tagomoris
2
370
How to use Macrobenchmark
veronikapj
0
160
DDDを志して3年経ったら「DDDの皮を被ったクリーンアーキテクチャ」になった話【デブサミ2024夏】
texmeijin
1
620
さきがけから振り返るアーキテクチャ刷新 / Reflecting on the Architectural Renewal from the Vanguard
nrslib
2
770
継続的な活動で築く地方エンジニアの道
myamashii
2
350
Rustのweb開発を助ける 便利なツール紹介
yuki0418
1
190
TiDB Serverless ~理想のServerless DBを考える~
soso_15315
1
160
Featured
See All Featured
Leading Effective Engineering Teams 2024
addyosmani
3
300
Understanding Cognitive Biases in Performance Measurement
bluesmoon
18
1.2k
A Modern Web Designer's Workflow
chriscoyier
689
190k
How to name files
jennybc
67
96k
A Philosophy of Restraint
colly
200
16k
XXLCSS - How to scale CSS and keep your sanity
sugarenia
245
1.2M
Build your cross-platform service in a week with App Engine
jlugia
227
17k
Debugging Ruby Performance
tmm1
71
11k
How STYLIGHT went responsive
nonsquared
93
5k
Thoughts on Productivity
jonyablonski
64
4.1k
実際に使うSQLの書き方 徹底解説 / pgcon21j-tutorial
soudai
149
45k
Six Lessons from altMBA
skipperchong
24
3.2k
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