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
External Code Reviews
By Evan Schultz
