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
100
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
290
Educating Enfys (lightning talk)
thatdamnqa
0
240
There's more to code review than you might think
thatdamnqa
0
230
There's More To Code Reviews Than You Might Think (lightning talk)
thatdamnqa
0
240
The Less Dull Bits of Testing
thatdamnqa
0
220
Other Decks in Programming
See All in Programming
Google Cloudとo11yで実現するアプリケーション開発者主体のDB改善
nnaka2992
1
160
❄️ NixOS/nixpkgsにSATySFiサポートを実装する
momeemt
1
110
若手バックエンドエンジニアが Elasticsearch を使ってみた話
hott0mott0
1
100
iOSでQRコード生成奮闘記
ktcryomm
2
150
Better Code Design in PHP
afilina
0
190
バッチを作らなきゃとなったときに考えること
irof
2
570
読まないコードリーディング術
hisaju
1
150
運用しながらリアーキテクチャ
nealle
0
240
ABEMA iOS 大規模プロジェクトにおける段階的な技術刷新 / ABEMA iOS Technology Upgrade
akkyie
1
270
CDK開発におけるコーディング規約の運用
yamanashi_ren01
2
270
PHPのバージョンアップ時にも役立ったAST
matsuo_atsushi
0
250
Modern Angular with Signals and Signal StoreNew Rules for Your Architecture @bastacon 2025 in Frankfurt
manfredsteyer
PRO
0
150
Featured
See All Featured
The Success of Rails: Ensuring Growth for the Next 100 Years
eileencodes
44
7.1k
Building a Modern Day E-commerce SEO Strategy
aleyda
38
7.1k
JavaScript: Past, Present, and Future - NDC Porto 2020
reverentgeek
47
5.2k
Fireside Chat
paigeccino
36
3.2k
Bash Introduction
62gerente
611
210k
Building Adaptive Systems
keathley
40
2.4k
Practical Tips for Bootstrapping Information Extraction Pipelines
honnibal
PRO
13
1k
Music & Morning Musume
bryan
46
6.4k
Being A Developer After 40
akosma
89
590k
Learning to Love Humans: Emotional Interface Design
aarron
273
40k
We Have a Design System, Now What?
morganepeng
51
7.4k
Refactoring Trust on Your Teams (GOTO; Chicago 2020)
rmw
33
2.8k
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