Introducing code reviews to a team – Part 1

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.

My approach

Taking the best parts from the code review process at my last job and combining it with some ideas from a few good blog posts and some modern tools, should give me a good starting point.

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.

  • danielkun

    We are having a 100% code review coverage for nearly two years now. What helped us split the non-creative “grunt” work from the more creative “design, architecture and code-accessibility” kind of work is having a checklist to fill out and post the results with the pull request by the code author and upon closing the pr by the reviewer. Because the initial spreadsheet version was clumsy to use, I’ve built a web app that integrates nice into the workflow. Perhaps your team can make use of it, too. See http://www.guess-what.io/c/

    • I’m cautious about how many processes to introduce at once. If I make the process complex or introduce too many steps it’s almost certain to fail. But I guess it depends on the team and their current work load.

      A checklist is definitely on my list of to-do’s for code review, but only when code review is second nature. In the beginning we’ll all need to remind each other and make sure we follow through on feedback.

      I’ll be sure to check out your app and suggest it to the team.

      • danielkun

        You’re absolutely right not to introduce too much at once. You could let the team gather some ideas in the first phase about stuff that can be mechanical checked to prevent bugs, etc. and decide whether it would make sense to use a checklist in the following phases.

        In my experience, the most difficult task is to get the team to understand that code review does not only consist of checking formatting, naming conventions and trivial matters like that, but the main reason for code review is, as you said, to transfer knowledge and keep the code-base fit for future requirements.
        Maybe you can make those two goals very explicit and make sure everyone values it most.

  • Pingback: 1 – Introducing code reviews to a team()

  • Pingback: Introducing code reviews to a team – Part 2 | Depth Infinity()