Introducing code reviews to a team – Part 2

It has been almost two months since the introduction of code reviews in the team and there have been some positive and not-so positive outcomes. At this point I wouldn’t call it a complete success but we are definitely on the right track. We are communicating more and code quality has improved as a whole.

I’d like to highlight some of the mistakes I made when introducing this new process and how we’ve managed to pull ourselves out of it, to the detriment of progress and confidence.

As you read this, be patient. I talk about Git quite a bit more than code reviews, but you’ll understand why as you continue reading.

Our current workflow

Although I wrote about it in my initial blog post, I’ll briefly go over our current workflow.

  1. Before starting a new piece of work, a developer creates a branch named after the ticket ID so it’s easy to track
  2. Once work is complete the branch is committed and pushed up to GitHub
  3. The developer creates a pull request and asks the team for a code review. The code review process also involves testing.
  4. If code review is given the OK by at least two developers, the author merges the code into our main deployment branch for a further test by a stakeholder external to our team.
  5. If the code review fails, then, then things slow down.

The workflow is a mish-mash of techniques I’ve learnt over time from working in very large teams (~30 developers) to very small ones (2 – 6). This coupled with some blog posts I’ve read online about modern development methods, mainly like the workflow above. You have tiny feature or bug branches and nothing gets through the brick walled pull request unless people have reviewed it.

Relying on tools to fix social issues

Before taking on this new role as manager the team were quite comfortable using SVN, checking into a single branch and creating tags for deployments. Tried and tested practices. Work was done, checked in (without review) and tested only by the developer who wrote the code. They then moved on to the next piece of work. There was nothing standing in their way.

And here lies the problem where I didn’t take the team’s background into consideration when enforcing this workflow. When people are committing code without roadblocks then code reviews become an impediment to getting stuff done. Pull requests were not the solution here, it was communication. My mistake was thinking that introducing the team to pull requests would solve that problem.

In reality what this did was slow people down to the point where almost nothing was making it’s way into the core codebase for deployment. Developers forgot to update their local repository before creating new branches causing a huge amount of merge conflicts. Pull requests turned into forums of discussion where there were so many comments, the mere thought of reviewing the code and the comments was off-putting. Pull requests would in-turn remain there for days or weeks. The team became skeptical of our workflow, and rightly so. I didn’t introduce it to the team correctly.

When a code review “failed”, and by failed I mean there wasn’t a “this looks good to me” in the comments, rather than address the issue the author would either not make corrections to their code or more importantly, actually talk about the review.

My main goal in all of this mess was code reviews and although we are there now, for the first two months we were battling our tools and not focusing on communication.

If I could do it again

If I could do it from the start again, I would introduce code review as a task you do then and there before checking in code. You deal with it right there, discuss the change, test together and then commit code. Pull requests can come later.

Git is amazing, and complex, and simple. It can be anything you want it to be and dwarfs SVN in the number of workflows it allows a team and each developer. It can however be daunting if you’re coming from SVN cold. To a team coming from SVN the best way to introduce Git is to set it up like SVN. Have one branch (called master) which everyone pushes to and pulls from. Introducing feature branches can be introduced a week or two after everyone has learnt the basic commands, mainly, git pull, git checkout, git push, git add -p.

Further to my previous point, pull requests became a place where the conversation would simply go stale. Developers couldn’t agree on the “right” way forward, so their grievances were left in the pull request comments. If I could do it again I would get the team to agree on an architecture and general code design first so that reviews go along a lot smoother. It’s easier when everyone agrees upfront.

While this might seem obvious what frustrates me is that we had the process sorted at my last job. Everyone agreed on design and coding standards and code reviews were done before commit.  I thought I could speed this process up with pull requests and it would all be rainbows and unicorns.

Final words

So what are things like today? Well, we catch up once a day for 10 – 20 minutes usually in the morning and talk about the day ahead and what was completed yesterday. We also flesh out the details and requirements of new features and proposed implementation as a team. Then we make a decision and move forward. We know what each other are working on at all times and I’m really happy about this.

Git is no longer a problem and everyone is comfortable with the branching and pull request system and we are starting to hum.