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
290
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
240
There's More to Code Reviews than You Might Think ✩
thatdamnqa
0
100
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
210
Other Decks in Programming
See All in Programming
2024年のkintone API振り返りと2025年 / kintone API look back in 2024
tasshi
0
210
『改訂新版 良いコード/悪いコードで学ぶ設計入門』活用方法−爆速でスキルアップする!効果的な学習アプローチ / effective-learning-of-good-code
minodriven
29
5.1k
SwiftUIで単方向アーキテクチャを導入して得られた成果
takuyaosawa
0
260
Amazon Bedrock Multi Agentsを試してきた
tm2
1
280
TokyoR116_BeginnersSession1_環境構築
kotatyamtema
0
110
JavaScriptツール群「UnJS」を5分で一気に駆け巡る!
k1tikurisu
10
1.8k
Pulsar2 を雰囲気で使ってみよう
anoken
0
220
『品質』という言葉が嫌いな理由
korimu
0
150
バックエンドのためのアプリ内課金入門 (サブスク編)
qnighy
8
1.7k
第3回関東Kaggler会_AtCoderはKaggleの役に立つ
chettub
2
700
いりゃあせ、PHPカンファレンス名古屋2025 / Welcome to PHP Conference Nagoya 2025
ttskch
1
260
DROBEの生成AI活用事例 with AWS
ippey
0
130
Featured
See All Featured
Exploring the Power of Turbo Streams & Action Cable | RailsConf2023
kevinliebholz
29
4.6k
How GitHub (no longer) Works
holman
313
140k
I Don’t Have Time: Getting Over the Fear to Launch Your Podcast
jcasabona
31
2.1k
Practical Orchestrator
shlominoach
186
10k
Sharpening the Axe: The Primacy of Toolmaking
bcantrill
40
2k
Testing 201, or: Great Expectations
jmmastey
41
7.2k
Performance Is Good for Brains [We Love Speed 2024]
tammyeverts
7
620
RailsConf 2023
tenderlove
29
990
Fight the Zombie Pattern Library - RWD Summit 2016
marcelosomers
232
17k
Embracing the Ebb and Flow
colly
84
4.6k
Intergalactic Javascript Robots from Outer Space
tanoku
270
27k
Building Adaptive Systems
keathley
39
2.4k
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