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

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

Given at Re:Develop conference, October 2016.

D887e6c4ecaca49b7add80a5f841727e?s=128

Daniel Shaw

October 14, 2016
Tweet

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