External Code Reviews

  • why?

  • internal vs external

  • do's and don'ts 

Agenda

Why?

To measure wtf/minute

  • Prevent the echo-chamber

And also...

Devs

Reviewer

  • Ensure best practices and guidelines are being followed
  • To identify new best practices/good ideas 
  • To help find defects
  • To improve quality of code
  • To provide guidance and feedback
  • To share knowledge 
  • To help Rangle.io improve it's projects
  • To help developers grow

But really.....

The differences

Internal

  • per-pr
  • before merge 
  • on-going
  • frequent
  • potentially blocking
  • generally smaller scope

External

  • can be per-pr,
  • a few prs,
  • a few commits,
  • a general area of concern
  • on-going
  • less frequent
  • shouldn't be blocking
  • can be done after merging

the condiments

the commandments

*reviewer suggests* .... did you mean

Do - limit scope

  • ~200-400 lines of code
  • More likely to find a bug in 10 lines of code, than 600
  • Ask for an area to focus on
    • Specific PR
    • Specific commit
    • Specific module/area of concern

Do - limit time

  • Limit it to 60 mins of prep
  • Limit the meeting to 60 mins
  • Less than 60 on either is ok

Do - take your time

  • It's better to review 50 lines of code carefully, than 200 carelessly
  • Try to walk through the logic, not jut skim
  • Look for possible errors
  • Look for things that they might have missed, but should be considered
  • Consider the context around the code you are reviewing

Do - consider the guidelines

  • Each project is different, and not every guideline may be fully applicable.
  • Deviations should have reasons

Do - use the metrics

  • Might take extra time on the first time
  • Should hopefully not slow down subsequent reviews
    • If they do - let me know
  • If key things don't improve - escalate them

Don't - let them replace the code review

  • Meant as a baseline 
  • Should not replace in-depth code reviews

Don't - build a bikeshed

to spend time focusing on easy to grasp, but relatively unimportant things
angular.module('myModule',[])
.controller('MyCtrl', function myCtrl($rootScope)
{
    $rootScope.firstName = 'Evan'
    $rootScope.lastName = 'Schultz'
    $rootScope.fullName = $scope.firstName + ' ' + $scope.lastName;
    $("#fullName").text($rootScope.fullName)
 
});

myCtrl vs MyCtrl vs MyController shouldn't be the focus here when there are bigger issues at hand

Do - be constructive

  • Be honest - most people want feedback
  • The meeting should be a discussion, not a lecture
var person = {
  firstName: 'Evan',
  lastName: 'Schultz'
}
var makeUpperCase = R.curry(R.mapObj(function (i) {
  return i.toUpperCase()
}));

makeUpperCase(person)

Bad

var person = {
  firstName: 'Evan',
  lastName: 'Schultz'
}
var makeUpperCase = R.curry(R.mapObj(function (i) {
  return i.toUpperCase()
}));

makeUpperCase(person)

Bad

Why the hell are you currying R.mapObj?

It's totally not needed - you give Ramda a bad name, moron.

var person = {
  firstName: 'Evan',
  lastName: 'Schultz'
}
var makeUpperCase = R.curry(R.mapObj(function (i) {
  return i.toUpperCase()
}));

makeUpperCase(person)

Better

var person = {
  firstName: 'Evan',
  lastName: 'Schultz'
}
var makeUpperCase = R.curry(R.mapObj(function (i) {
  return i.toUpperCase()
}));

makeUpperCase(person)

Better

You don't need R.curry here, as most of Ramdas functions are curried by default, and it doesn't provide any value here. You could also use the R.toUpper and simplify things down to:

var makeUpperCase = R.mapObj(R.toUpper);

Don't - be overly defensive

...but do defend

  • Be open to feedback
  • Often explaining your idea or reasoning out-loud can provide extra insight to yourself, and the reviewer

Do - ask questions

  • Did you think about...
  • Did you consider...
  • Why is it this way?
  • Why didn't you .... 

Don't - focus on style

  • Let linters and beautifiers take care of this
  • ... do check up that this is setup and being used

Do - focus on substance

  • Clarity
  • Cohesion
  • Coupling
  • Encapsulation

Do - call out the crazy

  • Things can make perfect sense to those who build it, but an outsider look might see that they have gone off into the deep-end.

Do - keep an eye out for patterns

  • Are similar things being done in slightly different ways?
  • Are there areas of repeated code that could be refactored
  • Is there a pattern emerging that could be abstracted?
  • Are there spots where using a design pattern could be beneficial

Do - get actionable items

  • Not every review will have them
  • Should be reviewed at the next review
  • Suggestions should also provide reasoning behind it

Don't - forget the tests

  • Are there test/spec files?
  • Are the tests meaningful?
  • Are the tests testing what the person thinks they are
it('should find the given product by id', function() {
  var prodId = 1;
  someSvc.findById(prodId)
  .then(function(result) {
    expect(prodId).to.be.equal(prodId) // did they really mean result.id? 
  });
});

// is this test even going to hit the expect when you expect it to?

Don't - forget the tests

  • Are there test/spec files?
  • Are the tests meaningful?
  • Are the tests testing what the person thinks they are
  • Do the tests do what they say
it('should convert the provided time to minutes', function() {
  var input = "3 hours";
  var result = someSvc.convertToSeconds(input);
  expect(result).to.equal(10800)
});

Do - make notes available

  • Submit a PR day/few hours before the review
  • Give the team a chance to read the notes
  • Use the PR as a discussion - comment on items/etc

and finally .....

celebrate the wins

  • Identify what is done well
  • Suggest where it could be applied elsewhere
  • If you think it could be useful for other projects - let others know
Made with Slides.com