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
310
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
250
There's More to Code Reviews than You Might Think ✩
thatdamnqa
0
110
There's more to code review than you might think
thatdamnqa
0
260
There's More To Code Reviews Than You Might Think (lightning talk)
thatdamnqa
0
260
The Less Dull Bits of Testing
thatdamnqa
0
240
Other Decks in Programming
See All in Programming
「App Intent」よくわからんけどすごい!
rinngo0302
1
120
Git Sync を超える!OSS で実現する CDK Pull 型デプロイ / Deploying CDK with PipeCD in Pull-style
tkikuc
4
450
Gemini CLIの"強み"を知る! Gemini CLIとClaude Codeを比較してみた!
kotahisafuru
2
150
MCPを使ってイベントソーシングのAIコーディングを効率化する / Streamlining Event Sourcing AI Coding with MCP
tomohisa
0
180
Claude Code派?Gemini CLI派? みんなで比較LT会!_20250716
junholee
1
690
Jakarta EE Meets AI
ivargrimstad
0
280
What's new in AppKit on macOS 26
1024jp
0
170
CDK引数設計道場100本ノック
badmintoncryer
2
560
Android 16KBページサイズ対応をはじめからていねいに
mine2424
0
650
202507_ADKで始めるエージェント開発の基本 〜デモを通じて紹介〜(奥田りさ)
risatube
PRO
3
550
プロダクトという一杯を作る - プロダクトチームが味の責任を持つまでの煮込み奮闘記
hiliteeternal
0
140
Advanced Micro Frontends: Multi Version/ Framework Scenarios @WAD 2025, Berlin
manfredsteyer
PRO
0
440
Featured
See All Featured
What’s in a name? Adding method to the madness
productmarketing
PRO
23
3.6k
Fantastic passwords and where to find them - at NoRuKo
philnash
51
3.3k
Large-scale JavaScript Application Architecture
addyosmani
512
110k
A better future with KSS
kneath
238
17k
Intergalactic Javascript Robots from Outer Space
tanoku
271
27k
Adopting Sorbet at Scale
ufuk
77
9.5k
Designing Experiences People Love
moore
142
24k
Faster Mobile Websites
deanohume
308
31k
Unsuck your backbone
ammeep
671
58k
Speed Design
sergeychernyshev
32
1k
Design and Strategy: How to Deal with People Who Don’t "Get" Design
morganepeng
130
19k
Optimizing for Happiness
mojombo
379
70k
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