Every week, we, at Mozaic Works, in the product development team discover 2-3 potential bugs in the product we are developing during our code review sessions. This happens despite a very structured way of work and despite applying ATDD and Test First / TDD.
Yet developers and technical leads complain to us in the community or during coaching sessions and workshops about various aspects of code reviews. Here are some tips to make your Scrum Code Reviews more useful.
There are two main types of guidelines:
Programmers will debate the cosmetic guidelines for hours and hours. The proof is on google, but we wouldn’t search there if we were you and had a lot of work to do. Cosmetic guidelines should be discussed exactly once and encoded in IDE settings. We can live with writing curly braces on the line after the function name; that’s not a debate worth having. However, code consistency is important, and any Scrum team should follow common cosmetic guidelines.
Large guidelines documents are prevalent when working as a junior developer. Typically, the technical leaders would copy the Microsoft guidelines or point the developers to them. While this helps learning how to write better code, it is impossible to remember all of them.
There’s a simpler way. Good architecture results into a thorough risk analysis, code samples and sets of guidelines to follow. These guidelines can relate to security (e.g. “always encrypt data stored by the patient file module”), testing (e.g. “use dependency injection to allow testability”), performance (e.g. “use a profiler to measure the execution time for the queries in the reporting module”) etc. These are the first guidelines; they are specific, contextual and help avoid common mistakes.
And this takes us to…
We remembered the days when the team leader would give us on our first day in a project a 20 pages guidelines document. Reading it took a lot of time, and we quickly forgot its contents. When I became a technical lead, I decided to improve the approach. Here’s how.
Technical guidelines are typically based on “best practices”; the trouble with best practices is that not all best practices apply to all products. I favour the name “rules that help avoid common mistakes”. How do you know which are the common mistakes? Easy, you need to make them first. Some of you will probably frown at this thought, but if you’re honest with yourself you’ll realize that you make a lot of mistakes. I make a lot of mistakes, but my colleagues keep me honest. How about taking advantage of the mistakes you make?
Here’s how this virtuous cycle works:
Write coding guidelines -> Perform code reviews -> Analyze mistakes -> Improve guidelines
There are three prevalent types of code reviews in the teams I worked with: over-the-shoulder, using a tool and pair programming. Each of them has advantages and disadvantages and that’s why it’s worth combining them.
The purpose of code reviews is to avoid mistakes (or, as we typically call them in the industry, bugs). A side effect is learning by reading and discussing someone else’s code. The first difficulty with Scrum code reviews is to build the discipline. If you’re just starting with Scrum code reviews, you need to set up triggers. For example: once a week or when a story is done.
Once you got into the habit of performing Scrum code reviews, the strategies can vary. We have a complicated schedule, and we have to vary the types of reviews:
We don’t use a tool for code reviews. We look at code, discuss, decide on improvements to make and apply them as soon as possible.
How does this scale? We propose the following strategy for Scrum code reviews:
This strategy takes advantage of the brain power of all developers from the team, not only of the technical leads.
People who were technical leads before the Scrum transition typically turn into gatekeepers afterwards. A typical issue is that they want to do all the Scrum code reviews to make sure the quality is at a high level.
While their purpose is good, the implementation is not helping the team. A technical lead that insists on validating the code will become a bottleneck very soon. The developers from the team will have little motivation in taking responsibility for the code they’re writing if they know someone guards it. Also, the technical lead risks to get separated from the team because he’s obviously not equal with the others.
Turn this the other way around and you get a functional, productive team that’s learning together. A former technical lead is coaching and helping everyone else grow by pair programming with them, helping with difficult tasks or delivering small training sessions. Developers take responsibility for their code because they review each other’s code. Everyone has equal roles, but everyone contributes to the team with the best they have to offer: junior developers with their skills and time, senior developers with smart solutions and technical leads with growing everyone else from the team.
Trusting your colleagues will get your team to a more effective work style.
To have more useful Scrum code reviews, follow these rules:
by Ovidiu Mățan
