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

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

18. None
19. None

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
44. None

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