In this multipart series, I’m going to document the introduction of code review to my team and how it plays out over the next few weeks. There won’t be any set schedule for this series but I’ll write about my experience as we take big leaps in the “right” direction.
Although I’m not new to managing a team, it’s been two years since I’ve been a technical lead. For a brief moment in time my team consisted of ~30 developers (give or take depending on scheduling) but was quickly scaled back once the project was complete. I was then technical lead for 6 different teams with the size of each team being between four to six developers. I’m just mentioning this to give you some perspective as the team I now manage varies between 3 – 4 developers.
As you can imagine, it’s very difficult to get a good set of consistent practices when you have 30 developers, each with varying moods and biases depending on the day. And also their knowledge of the coding standards outlined the Wiki, which were quite extensive but kept changing slightly every week. I feel that developers just gave up in the end. When it was time to review code, what we really looked out for were obvious bugs, problems with the design or architecture and where code should be located. Variable naming was inconsistent but no one really bothered to enforce them. I don’t blame them, making code review a nit-picky task really puts a damper on the mood.
One good thing was that code reviews were mandatory regardless of how small the change was. Before your code was pushed to the server, you had to call a developer over to your desk and begin walking them through the code. Removing unused “using” statements in your C# code? Code review. Formatting code so it looks nicer? Code review. Your commit message also had to have the name of the person who did the review and could only be done by seniors in the team.
With such a strict culture of code review, it was the norm to have your code criticised (a good thing) but it was also normal to get verbally abused for doing the wrong thing in the reviewers eyes and be screamed at in front of the whole office. It was ugly. As someone new to the company I quickly developed a thick skin and it wasn’t too long before I started to (unfortunately) imitate those around me. Although I never screamed at or put people down personally I did get stuck in to the author rather than the code. I realise now that the most important thing is respect for other people and their work.
The key drivers
- Higher quality code
- Having more than one developer knowledgeable in different areas of the code base
- Questioning design decisions
- Increased communication among team members
- Insight into different solutions
- Finding bugs
Finding bugs is intentionally mentioned last as it’s usually cited as the main reason for code review. Realistically, you might find a missing null check or an array bounds check error but as the complexity of a function or the size of a changeset increases, your chances of finding a bug go down.
Some guidelines for my proposed workflow
- Code reviews can be done by anyone, regardless of their level of experience
- Code reviews should not require you to break someone’s flow. Instead, developers should send a pull request and let the team review at their own convenience
- Everyone involved in the code review (including the author) must agree on an outcome. If not then we have a quick catch up at their desk to make a decision
- Some changes are exempt from code review, such as
- Removing commented out code
- Formatting code (indentation, white-space)
- Removing dead/unreachable code, proven by a static code analysis tool which the entire team are using
- Our code should follow an existing coding style and best-practice guidelines instead of re-inventing it ourselves. We pick something and stick to it.
I think this simple set of rules is a good basis before venturing into a more disciplined approach. I don’t want to introduce to too many things at once.
I’m looking forward to the dynamic this will introduce to the team.