Code Review vs Pair Programming

03 April 2016 craftmanship

This week, while working from home, we started a conversation on Slack on how to improve the quality control in our development process. Our current process only relies on occasional code review (abbreviated CR) from the boss.

This is a very light process compared to what I was used to. Nevertheless, the global quality of the product (number of bugs, performance, maintainability…) is superior to what I’ve seen so far. Therefore, I was OK with this process and even came to the conclusion that systematic CR was not essential to the quality of a software product.

And guess what… my boss proposed to do systematic CR and, of course, that he should not be the only one making the reviews.

I suggested that, instead, we should use pair programming (abbreviated PP), which is a technique that consists of having two developers on one computer working on a single problem. I believe that this practice is superior to CR and I demonstrated that the few significant problems we had during the past year (mainly a performance issue and a disagreement on a design) would not have existed, had we used PP.

Why I don’t like CR

I’ve been a professional programmer for 13 years and I think most CR process I’ve encountered were: useless, inefficient and unfair.

More importantly, I always thought that systematic CRs were a symptom of a lack of confidence in the author of the code. By the way, if it’s the reason you do CR, either learn to trust your collaborators, or improve your hiring process. CR is not an insurance against bad programmers.

I’ve always hated being the reviewer, especially when I was a team leader, mostly because I never knew how to show a problem without the author feeling offended. So my attitude has been to quickly skim the code hoping that I would not have to say anything. But unfortunately, sometimes, the code is just completely wrong and one day or another have to ask to restart from scratch. And here comes the worst part: you have to say to your coworker that spent days or weeks on this piece of code that he/she has to throw it away! A few years ago, one of my coworkers got so frustrated that he told me: “If what I’ve done is not good enough, then DO IT YOURSELF!”. I really felt bad that day, but it really needed to be said.

I have fewer problems with being the reviewee. My only fear is that the CR turns to a passionate debate on topics that nobody ever very agrees on. The classic examples, coming from my own experience are: the endless debate on the positions of the braces, the usage of auto in C++ or var in C#, the usage of virtual in C++, the usage of std algorithm instead of for loop in C++ etc. From what I saw, the reviewer tends to focus on problems of very little importance, and by that, I mean things that would neither reduce the number of bugs, nor simplify the code, nor improve the performance.

As a reviewee, what I hate the most is being asked to do something that I’m strongly against because I know that the original solution was better. This is the most frustrating part! You carefully crafted your program, made it clean and elegant, you’re sure that the reviewer will have a good time discovering the quality of your work, and BAM! like a slap in the face he/she doesn’t really dare looking at the solution just because he/she originally thought it should be different, like there can be only one solution to a problem.

Lastly, I don’t believe someone can understand in 5 minutes all the design decisions that have been made during a month of development. It’s simply not humanly possible to understand so quickly what unexpected issues appeared during the development and what compromises had to be made. I know one month is long, and that stories should be shorter, but that happens in many companies. Who never worked on a 4-week refactoring on its own?

Why I like PP

I’ve done very few PP sessions in my career, probably only a dozen. Each time, I found it extraordinary! I think that there is something magical with PP, something very hard to explain that makes 1 + 1 > 2.

The obvious advantage of PP is that each participant learns from the other. Indeed, while you share a keyboard with someone, you learn a lot of his/her working process:

does he/she draw a sketch?
does he/she write the tests first?
what are the solutions that he/she favors?
does he/she favor top-down or bottom-up approach?
how long is the design process before starting the code?
how often does he/she stop to take a step back?

Then, of course, you learn a lot about technical stuff:

hidden/forgotten library features
algorithms that you never heard about
preferred way of writing equivalent code

Ultimately, you understand every design decisions and every detail of the code. The result is significantly different from what you would have written yourself, yet, you’re OK with it because you followed the thinking process and the compromises that had to be made.

No more debate on the wrong details, no more blaming, no more reboot from scratch etc. It seems to me that everything I hate with CR is de facto eliminated with PP. Or so I though…

The Software Craftmanship Meetup

Yesterday, the Software Craftmanship Paris Meetup had an open session with no predefined topics. I took this opportunity to open a debate entitled “Code Review vs Pair Programming”. I was especially looking for the feedback of people who are actually using PP and the opposite, people who tried and deliberately decided not to do PP anymore.

A paper with "Code Review vs Pair Programming" and sticks for each votes

I was not disappointed. Ten gentlemen join the conversation…

What I learned about CR

When I explained my issue with CR, people immediately saw that they were some problems with the format of my CRs.

First, many people ticked when I took the example of a review of a one-month development. I immediately understood that a major part of my issues with CR comes from the fact that they came too late. CR must be frequent. There seems to be an agreement that a daily review is to be preferred.

Secondly, the same people reacted when I said that a CR takes 5 minutes. Indeed, even if reviewing trivial code can take just 5 minutes, usually a CR takes a lot more than 5 minutes. Also, I learned that reviewing code properly is a very intensive task, so you have to limit yourself to a one-hour run and then do something else.

As a consequence, the CR must be included in the estimations and in the plannings. Most teams have a dedicated column in its Kanban board. For all of them, the CR is blocking: the code will not go to staging (and therefore to production) if the review hasn’t been made.

As a reviewer, the way you talk to the reviewee is crucial. Communication is the key of CR. There are several techniques to prevent the reviewee for being offended.

First, never criticize the person, but always talk about the program itself. For instance, one should not say “your code is wrong”, but instead “the code is wrong.” This nuance is much appreciated by the reviewee because it feels less like the reviewer is blaming him/her.

In the same idea, never modify the code, but let the author make the changes. Doing so would infantilize the reviewee. I hope nobody does that.

Second, oral communication seems to be better than a software tool. Indeed, I too believe that it’s easier to accept criticism when it comes from a human being, compared to a cold heartless message in the mailbox.

To implement this, the reviewer takes the time he/she needs to make the review alone and writes all the things he/she wants to say on a piece of paper. Then he/she talks directly to the reviewee, checking every item one by one, and optionally giving the paper at the end of the conversation.

To avoid the endless chapel fight and religion arguments, you need to be very specific about the goal of the review. The goal is usually not to verify the conformance with the local coding conventions, this particular topic should be agreed upon before coding, or better solved by an automated tool (Checkstyle, Resharper, Clangformat…). Here are examples of good goals: finding bugs, finding holes in the test coverage, finding performance bottlenecks.

The last point that we addressed was that, like my boss suggested, everyone should make the reviews. Meaning that, not only juniors being reviewed by seniors, but also seniors being reviewed by juniors. Also, nobody should be exempted from CR.

During the conversation, I realize that every reviewer has its own level of detail in the analysis:

some review every single line
some only look at the functions names and signatures
some only look at the public interfaces or contracts
some only look at the tests

There seems to be no common practice or recommendation for the level of detail.

What I learned about PP

I was very surprised to see that PP is more employed than I though. Within the audience of 10 people, which admittedly is a very small sample, 25% were practicing PP on a regular basis.

Among the team regularly practicing PP, all of them also make CR. PP and CR are not mutually exclusive. They are complimentary tools.

Even if PP is a form of CR with instant feedback, it does not replace the CR. They are two different tools that you use on different occasions. PP is really good to start a new task of high complexity, whereas CR is used to close a task.

The personalities in the team must be taken into account. PP is not for everyone. Some people hate it because, for them, programming is an individual act, or because they don’t like having company when then work.

Finally, everybody agreed that PP is exhausting and you cannot reasonably make more that 2 hours of PP per day.

Conclusion

I was very pleased to see such interest, especially for an old topic. Talking about this issue was very helpful and changed the way I see CR.

When you listen to people talking about this, you realize that your practices say a lot about your team. For instance, I was quite shocked by the negative vibe that exists in one team, and amazed by the positive energy of another.

Unfortunately, we didn’t have enough time to talk more in detail about PP. As I said, I’m convinced that PP brings a significant boost in productivity. So I’m still looking for pieces of evidence, possibly studies, demonstrating this.

Still reading? Good, because I kept the best for the end. To my great surprise, some people love to be reviewed. Apparently they enjoy the challenging part of it. I was not expecting this. But now, I think this is a very good attitude. Hopefully, it’s going to be mine someday ;-)

Good Code Smell