ImperialViolet

I write this slightly out of anger/pain - I've had two patches get screwed up by code reviews today, but there are two sides to every code review...

The aim of code reviews is fairly obvious: if your code can't stand up to being reviewed it probably doesn't belong in the code base. There are some obvious downsides too; the amount of time that they take is the most common one that I hear.

However, there are some other downsides too. The code review is the most error probe part of the patch writing process. When you're actually writing the patch you are fully engaged with the structure of the code - what you're writing is probably correct and testing mostly catches the rest.

However, in the code review you're constantly in interrupt mode. A reply comes in and you make changes/answer the points in the reply and send it back. Every iteration is dangerous because you're context switching in and out.

This can be made worse is the reviewer is picking out stupid things: like changing an unsigned to an int (it was the length of a list, unsigned was correct). However, if it's someone else's code, or even if it's just been a long day you might give in and not bother arguing.

If the two parties know each other, the probability of a dangerous, picky review is reduced for the same reason that communication of all forms generally works better when people actually know each other. Bad reviews also stem when theres a big organisational difference between the two (because no-one is going to tell a senior engineer that they are a crap reviewer)

Of course, testing often saves the day here, but this is C++ and not everything tests easily. (In fact, very little tests easily unless test friendliness was a major facet of the original design). Inevitably, people don't run all the (probably pretty manual) tests after every little change and they just make that one last requested change and submit to get the damm thing done before lunch. (That would have been me today - the error wasn't a big deal at all, but it won't have happened without the code review.)

Code reviews also kill all minor changes. No one fixes typos or slightly bad comments because the effort of getting the review is too much. The barrier is such that a whole class of patches never happen.

One patch of mine today (I vaguely know the reviewer) got slightly better in one part and slightly worse in another because I could deny the requests which were wrong or silly, but did do that with enough vigor. The other (don't know the reviewer and they are very senior) got worse in almost every respect.

This is not to say that I don't think that code reviews are a good idea. I think some form is needed in a large code base. But they can easily be not just costly, but dangerous, unless done right.