Upgrade to Pro — share decks privately, control downloads, hide ads and more …

There's more to code review than you might think

There's more to code review than you might think

Given at DrupalCamp Dublin 2017

Daniel Shaw

October 21, 2017
Tweet

More Decks by Daniel Shaw

Other Decks in Programming

Transcript

  1. THERE’S MORE TO CODE REVIEW THAN YOU MIGHT THINK Daniel

    Shaw @thatdamnqa · http://thatdamnqa.com
  2. @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
  3. @thatdamnqa Check code style Automate if you can. Bad news

    can be better received by
 a cruel and lifeless script
  4. @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]
  5. @thatdamnqa Double check the code $permitted = ['2', '4', '5',

    'cake']; $input = 0; if (in_array($input, $permitted)) { echo "I'm in!"; }
  6. @thatdamnqa Double check the code function is_allowed_access($input, $expected) { if

    (in_array($input, $expected), true) { return true; } else { return false; } }
  7. @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 ); }
  8. @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 ); }
  9. @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 ); }
  10. @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