Better code reviews
This weekend I’ve been thinking about the most productive code reviews in some of the teams I’ve worked with. This post lays down some (very) opinionated ground rules.
TLDR; Awesome code reviews are small in size, contained in scope and short lived.
The premise
Just one! Developer productivity.
A productive developer is a happy developer, she spends most of her time in making things better, crafting beautiful experiences or fixing the not so great ones. She’s a great advocate of the product. She participates and stays engaged in processes and product improvements.
A non productive developer spends most of the time in the who part of a problem. She keeps complaining about deadlocks with other parts of the system. She is kind of frustrated with day to day stuff. A prolonged exposure to harmful activities/notions might have led to a learned helplessness. She may suffer from burn outs and eventually lose faith on goodness :D
Inefficient code review processes slows us down. Let’s talk about making code reviews better in three roles.
- A Developer creates the change (or pull request)
- A Reviewer is a fellow developer who reviews the change
- The Team = Developer + Reviewers
A developer should…
-
Meet the minimum requirements of a code review. It could be adding unit tests for each change, explaining the issue precisely, providing information on validations.
-
Define the scope of the change, and outline if there are further changes expected as a follow up. Some teams may expect every change to be complete in some sense, in that case it probably makes sense to define the stages.
-
Contain the changes to the scope mentioned above. I believe scattered changes add to the risk of the change, and will significantly increase the testing matrix. Now if you’re on a 15 year old code base with almost zero automated tests, please add a special prayer to the validation matrix ;)
A reviewer should…
-
Update her status of a code review: one of
waiting/request changes,approvedorapproved with suggestions. This is essential to notify the developer on next action. If we don’t indicate the status, the developer might jump in fixing comments, and then reviwer provides more comments, all of it leads to an never ending vicious cycle. -
Differentiate between suggestions and must-fix. Suggestions are not blocking, they may or may not be fixed in the current change scope. Must fix should be addressed. A differentiation allows constructive discussions, quite often future workitems, designs emerge from code reviews; marking them as suggestions allows a future change to take it up while letting the current change get to production!
-
Uses code reviews as a way to reinforce ubiquitous language. Discussions in the comments are guided by the design of the entire code in mind, they educate the team and ensure the same gaps in thinking don’t occur in future!
-
Explains or cites the sources for philosophical comments. Prefer the entire team follow a similar philosophy; if there are multiple opinions, talk; and then either convince or get convinced. Multiple philosophies in a single code base makes reading (reasoning, and debugging) of the code base extremely frustrating! My favorite go-to is the stylecop and fxcop citations, they are very well written and often help in such discussions.
Side note: too many philosophical comments is a distraction. Please push your team to use tools like .editorconfig, or linting (stylecop, resharper etc.)
A team should…
-
Establish a clear SLA for code reviews. In one of my past projects, we used to follow this:
- All comments on a change are published within a day of the pull request
- A pull request must be merged within a working week
-
Have a pull request template. This should call out the minimum requirements for any PR. It may include mandatory tests, validations etc..
-
Agree on the size of a code reviews. Some teams allow touching 100 files in a single change, others restrict to 5. Small and short lived pull requests are always better. Small changes minimizes risk, allows for frequently integrations and are easy to pull out if things go wrong!
-
Agree on a philosophy for coding. Probably the most important one here is Coherence: follow whatever convention is already in the source code. If you’re breaking, create a change only for the breaks. E.g. if a file uses
m_varNamefor variable naming, please keep it. If you’re too inclined to change it, first create a change which only updates all them_varNametovarName. -
Automate the philosophy. This ensures code review comments are NOT about naming variables, rather they focus on real business logic.
What are your favorite code review ground rules?