Open Code Review

Łukasz Lenart at Warszawa Design Patterns Study Group gave a very interesting presentation about Open Code Review in his company (you can read the details here). I’ve failed to notice whether it’s his own idea, or somebody’s else, but that’s not as important as the fact how well it works.

Open Code Review, in short, is when a small group of programmers (up to 6) meet once in a while for a brainstorming session, take some printed out code, and try to find out, for an hour or so, what could be done better.

It’s basically about sharing knowledge and insight with others. It can be a very beneficial experience, especially for teams that do not use Pair Programming, or to pairs that do not switch very often, but I think anyone can find it useful.

The power of this technique is reflection. When we code, when we create software, we often find ourselves thinking in a specific way. We tend to take things for granted. We start to look at the code with our mind set on a particular solution, and as a result, we can’t see the forest for the trees. This is how human mind works, it’s nothing new. After a few weeks of walking the same route, you stop noticing the buildings along the way.

It’s also one of the reasons why Pair Programming is so efficient. “You cannot solve a problem with the same mind-set that got you into the problem in the first place”, as some believe Einstein said. Having another mind next to you, looking at the same code, gives a chance to spot the problem as it is being created, but more importantly, it is very helpful in the process of discovering the best design. Open Code Review works in a similar way. It allows you to learn different mindsets and different styles of solving problems. Nothing is as refreshing as being able to see from other points of view.

Apart from this, OCR meetings also help in spreading knowledge (people read different books) and experience (people work on different projects with different technologies and problems).

This idea inspired me to organize similar meetings at TouK and I’ve got very good feedback so far. Our formula crystallized by looking at the the past and finding out what was cool and what was not. Our take on Open Code Review is quite different to what Łukasz proposed.

But first, things that we do the same way: we take maximum 6 people (plus the moderator), we gather them up in a conference room, we hand out code printed on paper.

The code we take is not Open Source, as Łukasz suggested. His reasoning was, that if you take somebody’s code, it could turn personal, we all put some heart into our work, after all. This is where we disagree: we have found it very valuable to take internal code from our projects. We are not afraid of our code being criticized. As Robert C. Martin wrote: every good programmer should get used to professional code review. Bad programmers can benefit from it even better. And if you have some Prima Donnas in your team, unwilling to hear constructive critique to their code… screw them. Nobody’s able to work with them anyways.

Second thing we do differently, is that we do not comment the code on paper. We actually have a working environment (IDE and stuff) with three keyboards/mice, thanks to USB hubs. We either comment the code directly, writing TODOs, for bigger changes that would take more time than it’s left, or we do the refactoring right away. This allows us to make a commit to the repository after the session and improve some project a bit. It’s also much easier to talk about the code, when you have a keyboard at hand. Sometimes it’s just way simpler to show what you mean than to explain it with words.

We also take a bit longer for each meeting: usually an hour and a half, or two hours. This is the optimum time, to learn and do something meaningful, and not get tired. And we do not split attendees into teams, we want people to talk a lot, and we want people to talk with each other.

So what is our recipe for an Open Code Review? It’s like this:

Ingridients

1. Invite anyone who wants to participate, from all over company, but take no more than 4 to 6 programmers plus a moderator
2. Take a conference room with a laptop, three or four keyboards/mice, and a projector
3. Set up the meeting for 1.5 or 2 hours
4. Moderator prints out a class (or a few classes) that
   1. has between 150 and 300 lines of code
   2. has unit/integration tests for all non trivial methods
   3. is from a project that works with our environment (IDE/Maven etc.)
   4. is not trivial and not completely shitty (that would be too easy and nobody will learn anything)

Instructions

1. Give everyone printouts and 10 to 15 minutes to read the code and make notes
2. Ask whether you should start top-down or from some other point in the code (some classes are broken conceptually and it’s better to fix them by talking about their responsibilities first). If in doubt start from the top.
3. For every block/method/property ask whether anyone thinks that it could be done better. The person that has the idea, has the power (keyboard) to do it.
   1. If it’s a small change, fix the code
   2. If it’s a big change that would take longer than the meeting, write a TODO
   3. If someone disagrees, talk, talk, talk until consensus is found or you agree that you disagree and move on
   4. Run (write if needed) tests, to make sure the code works. This is a real refactoring, you cannot break the code. Broken code is not considered better.
4. If you cannot find anything more that you could get better, use the principles from “Clean Code” (or some other book) to guide you.
5. At the end of the meeting, commit the code.

Things to remember

If you are the moderator, remember to shut up and keep quiet. You probably had a bit more time to read the code so it’s natural you’ll have noticed more flaws, but let the people do the exploration and discovery. When they don’t have anything else to say, then it’s the time for you to speak.

If you are the author, no matter what, DO NOT EXPLAIN the code. Either the code speaks for itself, or is badly written.

Enjoy.

PS: this practice has been unfortunately stopped, because I’ve started to go to conferences and could not moderate any more. Do not make my mistake and communicate clearly, that ANYONE can call for an Open Code Review, and that the moderator should be changed every two weeks or so. I’ve failed at doing that, and now I have to revive the whole thing.