January 13, 2015
Every week, me and Claudia 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 me 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.
Tip #1: Discuss Cosmetic Guidelines Exactly Once
There are two main types of guidelines:
- Cosmetic: indentation, spaces vs. tabs, naming policies, curly braces positioning etc.
- Technical: how to write code to avoid common mistakes
Programmers will debate the cosmetic guidelines for hours and hours. The proof is on google, but I wouldn’t click this link if I were you and had a lot of work to do. Cosmetic guidelines should be discussed exactly once and encoded in IDE settings. I 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.
Tip #2: Start with Minimal Guidelines Derived from Architecture
Large guidelines documents were prevalent when I was working as a junior developer. Typically, the technical leaders would copy the Microsoft guidelines or point the developers to them. While this helped me learn how to write better code, it was 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…
Tip #3: Improve the Guidelines based on Your Past Mistakes
I remember the days when the team leader would give me on my first day in a project a 20 pages guidelines document. Reading it took a lot of time, and I 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
Tip #4: Use Different Types of Code Reviews
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. I have a complicated schedule, and I have to vary the types of reviews:
- Over the shoulder – I sometimes go near Claudia and look at the code she’s writing. She does the same; in fact I’m never shy to ask for her help when working on a task.
- Scheduled – every week, we have a one hour code review session
- Pair programming – when there’s a more complicated task or we decide to try a new way of doing things
- Random – about once a month, I take a look at random parts of the code to see if they’re better ways to write them
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? I propose the following strategy for Scrum code reviews:
- Pair program on complex user stories. If the story was completely developed using pair programming, consider it already reviewed.
- When a story is done, review with a colleague. It’s not necessary that the colleague is more experienced. Review against the guidelines and against readability, simplicity, changeability, security. Write down the problems you found and give them to the Scrum Master.
- Once per sprint, schedule a 30′ session with the team where you collectively review some of the code written during the sprint. Give the results of the review to the Scrum Master.
- At least once every 4 sprints, have a skilled developer take a look at random parts of the code for 30-60′. The conclusions should go (no surprise here) to the Scrum Master.
- Discuss the findings at the retrospective. The Scrum Master should decide on a schedule, based on the number of identified issues. The retrospective should result into a guidelines update.
This strategy takes advantage of the brain power of all developers from the team, not only of the technical leads.
Tip #5: Trust Your Colleagues
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:
- Discuss cosmetic guidelines exactly once, and encode them in the IDE settings
- Start with a minimal coding guidelines document that results from architecture risks (e.g. security)
- Perform code reviews and write down the problems you find
- Analyse the problems at retrospectives and improve guidelines based on past mistakes
- Use different strategies for code reviews: over the shoulder, pair programming, scheduled, random, through a tool
- Trust your colleagues
Did you find this useful? Let me know in the comments.