Review of our code review process

August 19th 2016, twelve days before the deadline, the release of another expansion of the game world of Kweetet. I read this article online that absolutely convinced me that we had to start with code reviews, and we had to start now. The stats spoke for themselves. 80% of the bugs are found during code review? Sign me up!

Did you read the above line? “80% of the bugs are found during code review”. I’m still baffled by that.

When you’re rushing for a deadline that’s when the most mistakes are introduced, so I wanted to start with code review immediately, to capture as much of them as we can. It turned out this was very successful, we did find bugs that would have been very embarrassing in a release.

There are a lot of options in setting up a code review process, for example as described in this article, which is a pre-merge review process with pull requests and is by far the most common model, certainly when using git.

At die Keure we use perforce for the development of Kweetet, which does not know the concept of pull requests. This forced us to use a post-check-in process. More specifically, we used a MOB code review process (how cool a name is that?).

Now, a good year later, we’re still doing code reviews and didn’t change much to our initial process.

Context

Maybe I should start with a little context, because although our process works well in our team, there’s no guarantee it would work in another. Actually, it turned out it didn’t work for another team - more on that later.

Kweetet is played at school and at home during the six years that a child attends primary school. It is a story driven adventure game, where children complete quests, interact with numerous NPC’s, collect stuff, play puzzles and minigames and a lot more. A child starts playing the game in the first grade and only finishes it in the sixth. Every school year they start a new chapter in the story. And every chapter is played in another world, which means a lot of content. And a lot of code.

We have apps for Windows, Mac, iOS and Android plus a “lite” version for Android and iOS. Kweetet can't be called a small project; I didn’t count the number of lines, but we have a good 7000+ files with code, not counting the server code. This code has been written by a team of 4-5 people (seniors, juniors and interns) over the course of four years. In total, 10 programmers worked on the code.

We could have done this faster, but we faced a major technical hiccup in the middle of our development: the game used to be played directly in the browser via the Unity webplayer plugin, but alas, browsers decided they didn’t want any plugins anymore forcing the webplayer to retire. We tried to convert the game to WebGL, but we learned the hard way that this technology, promising as it is, has no way near the performance (yet) as the webplayer had. This forced us to convert the game to installed versions on several platforms. We lost a lot of time on that.

So in short the context is: lots of code, a small team with interns coming and going, over four years. Only the last 1.5 years we had code reviews. By now it’s safe to say: we should’ve done this by the start, we would’ve been ready perhaps a year earlier.

As said before we don’t use git, which is a common tool in code review processes, but Perforce. Completely unlike git-flow, the whole team works on the trunk (and for good reasons, but not in the scope of this post to elaborate on). So we’re actually reviewing commits rather than pull requests, if put in a git context. There is collective code ownership.

Our process for MOB code reviews

• Right after lunch, we sit down with some team members for maximum 45 minutes (it’s often shorter) and review all commits since the last review session. Right after lunch, because then we don’t interrupt anyone’s “zone”.
• Anyone who wants and/or has time can join, but you’re not obliged. In general we’re doing these reviews with 3-4 people; with more, the discussions can become lengthy.
• We do this reviews on a projector, with someone behind the keyboard going through the files. There’s no designated driver, anyone who wants can take the wheel.
• We don’t exceed the 45 minutes limit, anything that isn’t reviewed we review later. Programmers note the bugs that are found and fix them afterwards before continuing on what they were doing.

These are not hard rules, if no one is free, or there haven’t been many commits, the review is postponed. Sometimes we skip to important parts first and then review the other things later.

We try to keep to a daily review, but at least once a week it happens that we skip one.

The good

Less bugs

Obviously, we have less bugs. We don’t measure anything, but we do find mistakes regularly, so these are all bugs that are squashed. This is actually the least important advantage (but an advantage nonetheless).

Better code

The more important advantage is not so much that there are less bugs, but that the code is just better. It becomes more readable, it is more performant, it is better structured.

The above situation is one that could occur in the past, but doesn’t anymore.

Because we talk with each other on a daily basis about our code, we discuss the chosen solutions on problems. We share a lot of views and opinions with each other, we discuss why we prefer certain approaches, we share ideas, we tell each other what we perceive as readable code, we have many good laughs.

Thus the code became very consistent, all members of the team write code in pretty much the same way now, and we all changed our ways to get to that point. Voluntarily.

Better developers

This made us all better developers, we learned solutions to problems we would’ve never thought of, we write better and more performant code, we all know and understand almost the entire code base.

Better team

It made us a better team. We have our share of heated discussions, certainly at the start, but it all happens in mutual respect and it made a stronger team. There was also a lot of humor. A lot of it :)

Respect and team spirit among team members can really improve the quality of the code, since everyone is open for improvement and feedback. These code reviews introduce that.

More resilient

We are more resilient to changes, by which I mean members of the team entering or leaving. When a team member joined us (often interns) they quickly learned the codebase and also learned our approaches. On the other hand we were challenged by these new team members; we had to defend to them why we did the things the way we did them.This brought new insights to the team and to the new member.

When we weren’t doing these code reviews, we often ended up with very alien pieces of code in our codebase written by interns who didn’t know any better. We still suffer from that.

And when a team member left, everyone knew the code he/she worked on so there is little to no need of knowledge transfer sessions.

Collective code ownership

Because everyone has seen the code grow, has discussed the changes, had the chance to contribute (even without writing any code), there is a stronger sense of code ownership among the team over the complete codebase.

We talk to each other

Instead of writing our remarks behind a computer, we sit together and talk to each other (Of course, this is only possible when your team is not spread over different locations in the world.). Differences in opinion are voiced immediately and can be discussed in an open manner. Misunderstandings are detected early. It creates a stronger team spirit.

It’s fast

Typing a sentence is not as fast as saying it. And when you’re typing, you really need to weigh your words, lest they are not misinterpreted. Discussions in text can generate a lot of back and forth messages, costing more time.

Less delays

The review of pull request can take time before someone had time to do them, and by the time you get the feedback you’re already on working on something else, as happened with the author in this article. By reviewing commits rather than pull requests, we review code very early, even while it’s still in development. This can seem obsolete, since the programmer might detect his own errors before he’s done, but more often the programmer is just helped by this. Even better: if we notice that a programmer is implementing a system fundamentally wrong, we detect it before much work has been wasted. This is especially the case when interns join the fray.

The not so good

It didn’t work for the webdev team

We also have a team of four web developers who tried to implement the same process, but they stopped after a few months. They considered it a waste of time and instead started to review pull requests in git from time to time via the computer. Discussions happen via slack.

Perhaps important to note is that they used to work via a weak code ownership model (each programmer is responsible for a separate part of the code or a separate project), this makes the open code review process not such a good fit.

We mostly review code

Since we review the commits one by one, file by file, we’re mostly considering small local changes to files and new code files. We’re not looking at the big picture in a structural manner. However this is not such a bad downside; although we don’t review systems in a consequent process, we often discuss the architecture of the game when a related issue pops up

We can get behind

Sometimes we get behind with our reviews, when most of us can’t attend the review, we skip it. We try to keep this to a minimum, but sometimes we skip a few days, and then it’s possible we have a lot on our plate. Nothing to do about that, we just do the work and sometimes extend the duration of the review session a bit. The problem is that this can become a drag and that causes us to be less attentive. We try to avoid this problem as much as possible, by having review sessions nearly every day.

No history

An advantage of written code reviews is that you have a history of them. You can refer to previous reviews for motivation of arguments. You can cross check how you dealt with similar issues in the past. This is something we don’t have in the process. Can’t say I miss it though.

On the spot

In the proposed process, we need to review and spot defects on the spot. This can be hard sometimes. It’s a big advantage of reviewing pull requests on the computer: you can take all the time you need to review a file/codebase and you can even take a complete checklist and go over the code at length.

Conclusion

The team and myself all feel that this MOB code review process improved our code, our product and ourselves. For a small co-located team, working on the same project, I think this is a good approach.

Feel free to share your own code review experiences below! I'm starting as a lector in game programming shortly and I'm playing with the idea to incorporate code reviews in the lessons, so any feedback is more than welcome!