There's More to Code Reviews than You Might Think ✩

Given at Re:Develop conference, October 2016.

Transcript

1. THERE’S MORE TO CODE REVIEW THAN YOU MIGHT THINK Clair Shaw @clairs · clairshaw.co.uk
2. What are
 Code reviews for?
3. Code reviews are not an indication
 of anybody's abilities
4. ☐ 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
5. Check code style
6. Check code style Pick a house style, use it, communicate it
7. Check code style Pick a house style, use it, communicate it
8. Check code style Don't get too distracted by checking code style
9. Check code style Automate if you can. Bad news can be better received by
 a cruel and lifeless script
10. Review the configuration
11. Review the configuration { "require": { "usefultool/tool": "1.1" } }
12. Review the configuration { "require": { "usefultool/tool": "1.2.1" } }
13. Review the configuration { "require": { "usefultool/tool": "^1.2" } }
14. Check forwards compatibility
15. Check forwards compatibility PHP 5.6.26 PHP 7.0.11
16. Check forwards compatibility jQuery 1
 jQuery 2
17. Check forwards compatibility jQuery 1
 jQuery 2 jQuery 3
20. Review the documentation
21. Review the documentation Ensure changes make sense, and are correct
22. Review the documentation Make sure code changes are documented
23. Review the commit message
24. Review the commit message Update usefultool.
25. 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]
26. Double check the code
27. Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets(); } else { $this->accessDenied(); }
28. Double check the code $permitted = ['2', '4', '5', 'cake']; $input = 0; if (in_array($input, $permitted)) { echo "I'm in!"; }
29. Double check the code Is the code testable?
30. Double check the code if (in_array($user_id, $permitted_users)) { $this->showRestrictedSecrets(); } else { $this->accessDenied(); }
31. Double check the code function is_allowed_access($input, $expected) { if (in_array($input, $expected), true) { return true; } else { return false; } }
32. 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 ); }
33. Double check the tests
34. Double check the tests Test for failures too
35. 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 ); }
36. 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 ); }
37. Final thoughts…
38. Final thoughts… All output should be reviewed.
 It’s not personal
39. Final thoughts… Often, a peer review can teach
 something to two people
40. Introducing Code Reviews
41. Introducing Code Reviews Step 1: Keep watching
42. Introducing Code Reviews Step 1: Keep watching Step 2: Ask questions
43. Introducing Code Reviews Step 1: Keep watching Step 2: Ask questions Step 3: Go to step 1
45. ☑ 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
46. Thanks. 
 clair@clairshaw.co.uk
 @clairs