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
250
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
200
There's More to Code Reviews than You Might Think ✩
thatdamnqa
0
98
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
大規模マルチテナントを解決するYugabyteDBという選択肢
nnaka2992
1
250
The rollercoaster of releasing an Android, iOS, and macOS app with Kotlin Multiplatform | droidcon Berlin
prof18
0
110
Jetpack for KMP
fornewid
1
290
CSC307 Lecture 12
javiergs
PRO
0
220
Rubyのパフォーマンスプロファイリングの改善 / Enhancing performance profiling for Ruby
osyoyu
1
410
君たちはどうコードをレビューする (される) か / 大吉祥寺.pm
utgwkk
15
8.5k
小さな開発会社を作った理由
polidog
0
1.9k
継続的な活動で築く地方エンジニアの道
myamashii
2
360
実用的かつリーズナブルな 「Azure × Gemini × LINE」~キャラクターBot 実装ライブデモ~
tomodo_ysys
1
170
CSC307 Lecture 08
javiergs
PRO
0
330
Activities at Cairo Library
cairolibrary720
0
1.2k
Rust.Nagoya #1
codemountains
0
170
Featured
See All Featured
The Success of Rails: Ensuring Growth for the Next 100 Years
eileencodes
35
6.3k
It's Worth the Effort
3n
181
27k
Practical Orchestrator
shlominoach
185
10k
The MySQL Ecosystem @ GitHub 2015
samlambert
248
12k
Sharpening the Axe: The Primacy of Toolmaking
bcantrill
26
1.6k
Six Lessons from altMBA
skipperchong
24
3.2k
No one is an island. Learnings from fostering a developers community.
thoeni
17
2.8k
Become a Pro
speakerdeck
PRO
15
4.8k
Speed Design
sergeychernyshev
9
270
10 Git Anti Patterns You Should be Aware of
lemiorhan
652
58k
GraphQLとの向き合い方2022年版
quramy
36
13k
Testing 201, or: Great Expectations
jmmastey
33
6.9k
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