Pairing Is Not Code Review
Published on January 30, 2012 by Jesse Storimer
I previously posted about the importance of formal code review and its benefits. Though I shared why the code review process is important I didn't make it clear how that fits into a software development process alongside TDD, pairing, etc.
I replied thusly to @brynary's tweet:
I want to expand on that.
In my experience it is most useful at the beginning of the software development process. When you're looking to the horizon for a solution, unsure of which direction to take. Once a course has been chosen then a single developer can push the code in that direction.
Code review doesn't even relate orthogonally to pairing. Code review is the last step in the software development process.
Code review is one developer saying to another:
"Hey, I've found a problem, wrote some tests, implemented a solution, tested the solution locally and in the staging environment and now I think it's ready to go. Do you see any reason why it shouldn't be deployed?"
If there is anything that indicates the change shouldn't go online then this is the time to bring it up. This is not the time for sweeping design changes.
What Code Review is For
The biggest benefit of code review is fresh eyes on the code. Ideally, eyes that weren't involved in the design and implementation of the code. Fresh eyes are the best bet to find edge cases that the original author may have missed.
You should have at least one reviewer on each review who is familiar with the (official or unofficial) style guide of the project. This is the stage where your style guide is enforced. Did the author forget to use an idiom common to the project? Was some code re-implemented that could have been included from a module elsewhere?
When reviewing code it can be tempting to point out all the ways that the author is 'wrong'. In other words, all the things that you would have done differently. Remove ego from the equation as much as possible, your way isn't necessarily 'right'. After that err on the side of being verbose, keeping in mind that not every comment needs to be addressed.
Not all code review comments need to be answered with modifications to the code. Not every suggestion needs to be implemented. Anything that prevents the code from working as advertised needs to be addressed. Everything else is at the discretion of the author.
The point of this kind of comment is basically to say 'gotcha!'. If the author decided to do a database query from a view helper then you should remind them that we put database queries in the controller and reserve the view helpers for extracted view logic. This doesn't mean that the entire changeset should be delayed to ensure that this convention be followed. The most important thing is that code gets shipped.
The author can always follow up later to fix the regression. The subtle point of this kind of comment is to make the author think twice the next time they break convention. It's to remind them that they have to be accountable to their peers. Their peers are watching, and caring!
Waiting On Others
It seems at first glance like code review will slow down a development cycle. We have one developer responsible for implementing a feature from start to finish. Before deploying the code he hands off responsibility to a reviewer to '+1' the whole thing. What if the reviewer is busy, takes days to respond? Talk about buzzkill.
If code review is going to be part of your development process then developers need to understand its gravity. Developers should be aware that when a code review is assigned to them they are the last thing holding up a changeset from being deployed. There's no excuse for letting this go on longer than it has to.
Unless something prevents it: code reviews should be performed the same day they're received. If you can't do it then forward it on to someone else. The point isn't to enforce rigid structure into your process where none is needed. The point is to respect your co-workers. The way to do that is to help them get their stuff deployed without delay.
As I mentioned, code review has had a huge impact on my development as a programmer. Having my code critiqued by other developers always sheds new light and I've learned a ton from the suggestions of others.
My biggest takeaway from code review was a different perspective on code. Code review helped me understand that I'm not just implementing features for end users, I'm implementing APIs for my peers. They may very well be the ones who have to understand and maintain my code in the future.
No matter how important it seems to ship something quick to make an end user happy the guy performing maintenance must be considered. Code review ensures that that guy gets a voice before your code ever sees the light of day.