There's more to code review than you might think

Given at DrupalCamp Dublin 2017

Transcript

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