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
320
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
260
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
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
複雑化したリポジトリをなんとかした話 pipenvからuvによるモノレポ構成への移行
satoshi256kbyte
1
680
AccessorySetupKitで実現するシームレスなペアリング体験 / Seamless pairing with AccessorySetupKit
nekowen
0
210
GitHub Actions × AWS OIDC連携の仕組みと経緯を理解する
ota1022
0
220
Platformに“ちょうどいい”責務ってどこ? 関心の熱さにあわせて考える、責務分担のプラクティス
estie
2
510
エンジニアとして高みを目指す、 利益を生み出す設計の考え方 / design-for-profit
minodriven
23
11k
大規模アプリにおけるXcode Previews実用化までの道のり
ikesyo
0
960
AIで開発生産性を上げる個人とチームの取り組み
taniigo
0
130
ИИ-Агенты в каждый дом – Алексей Порядин, PythoNN
sobolevn
0
140
NetworkXとGNNで学ぶグラフデータ分析入門〜複雑な関係性を解き明かすPythonの力〜
mhrtech
3
900
スマホで海難事故は防げるか?年間2000件以上の小型船舶の事故に挑むアプリ開発
atsuki_seo
0
120
あなたの知らない「動画広告」の世界 - iOSDC Japan 2025
ukitaka
0
310
CSC305 Lecture 03
javiergs
PRO
0
230
Featured
See All Featured
Speed Design
sergeychernyshev
32
1.1k
Code Reviewing Like a Champion
maltzj
525
40k
The World Runs on Bad Software
bkeepers
PRO
71
11k
CSS Pre-Processors: Stylus, Less & Sass
bermonpainter
358
30k
How To Stay Up To Date on Web Technology
chriscoyier
791
250k
Evolution of real-time – Irina Nazarova, EuRuKo, 2024
irinanazarova
8
950
Fashionably flexible responsive web design (full day workshop)
malarkey
407
66k
Practical Tips for Bootstrapping Information Extraction Pipelines
honnibal
PRO
23
1.5k
Put a Button on it: Removing Barriers to Going Fast.
kastner
60
4k
Fantastic passwords and where to find them - at NoRuKo
philnash
52
3.4k
How GitHub (no longer) Works
holman
315
140k
BBQ
matthewcrist
89
9.8k
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