How to make reviewer interested in high quality code review?

Question

I experienced to work with the code which was reviewed, but still of low quality. Actually reviewer looked through the code and could think that this is more formal procedure and didn't pay enough attention to the process of review and said: "I have reviewed the code!". Some small fixes can be made but significant flaws are missed.

The problem is to explain that it is important to make the thorough review, because in future this code is easier to support, extend, fix, easy to reassign to other person etc...

But all these reasons are not essential for reviewer. Responsible for code is another person. Oh, author of code can leave company? Reviewer didn't think about this. Company spend more time to fix bugs when they reproduced by tester? it doesn't mater for reviewer. Some things are not documented? Author explain this to the reviewer. And reviewer even can support effectively such code. others... others...

Answer 1

The only purpose for a code review is to improve the quality of the code and the quality of the processes that produce the code. With that in mind:

• A review must never be undertaken by a single person and should always include all of the programmers involved in writing the code. If someone who wrote some code you intend to review is not available, postpone the review.

• There must be no artificial distinction between "reviewer" and "author" - both are part of the same team and must have the same goals.

• The results of the review must never be a general statement like "this code is of poor/good/excellent quality". Instead, it should spell out in detail, with specific examples why and where the code fits into these categories.

If I sound a bit dogmnatic here, it's because I was once subject to a hostile review of legacy code in an app that I was managerially responsible for, but which nobody on my team had written. I was explicitly forbidden to rewrite the code and was not allowed to take part in the code review. The review basically said "this code is crap" - which we already knew, and did nothing to raise the quality of the product and everything to lower the morale of the developers.

Answer 2

The reviewer of the code must be held accountable for the quality of the code. The sign off on the review should be formalized. If sufficient fault is found in the code a second review should be mandatory and a secondary signature required.

Answer 3

Accountability encourages quality. The reviewer's stamp of "reviewed" approval should be a statement that the reviewer would have checked that code in, himself.

Consider this scenario: A bad late-cycle bug is found and tracked back to its original checkin. Instead of only asking the author why it happened, ask both the author and the reviewer. Both the author and the reviewer should learn from the mistake.

If a reviewer who lets crummy code pass as "reviewed" is held accountable for the poor workmanship (of the review), then the reviews will improve: either the person doing the reviews will start doing better reviews, or the next person filling that position will. I don't mean to sound harsh, but people need to actually do their jobs. (This of course assumes that reviewing code is a part of the job, not just a friendly favour.)

Answer 4

Bribes. Things like cookies or doughnuts for however many bugs found. If you show that you actually appreciate the reviews that the reviewer is giving, it will encourage them to try harder for you. Obviously, this isn't a cure-all, nor will it necessarily work on most of the reviews (chances are they are already busy as hell with their own stuff.) But if you actively try to engage the reviewers with positive feedback that shows you are taking them very seriously, and they'll take you seriously in return.

If you complain, you'll only breed resentment.

EDIT

In case it wasn't clear from the rest of my post. I wasn't talking about bribery as being the actual solution. The real solution is engaging the reviewer, rather than just critiquing the review you got. In the same vein as reading my whole response rather than just disliking the first word of it.

Answer 5

Start with the Macadamian Code Review Checklist. Edit and adapt it to suit your specific needs. This way you will have specific guidelines for reviewers. If possible, reviews should follow The Ten Commandments of Egoless Programming. Other than that - invest in team building. Peopleware is a good source of ideas. The Psychology of Computer Programming is another.

Answer 6

I know it is blasphemy, but in general, code reviews provide little value to begin with. Typically, there are few real errors found (ok, almost never if you have competent developers). Most code reviews end with trivial action items and don't fix any problems, unless you count grammar as a big issue.

The more important review is "How did you test the code?". What tests were performed? Did the developer do a sufficient level of testing? That is where you will find out if the code works or not.

It is not that having peer reviews is a bad idea, but the realities end up getting in the way. The main reality is that software is usually hard to understand and when it isn't hard to understand then the problem it is solving is hard to understand. While some companies have easier applications, in the places I have worked it would frequently take a day or more just to understand the problem and requirements the code is solving before even being able to begin to dive into the code to understand if the code works correctly. If you don't understand the problem, how can you find real errors in the code? Most people don't have the luxury of 1 1/2 days to properly review someone else's code.

With that said, there are some reasons/situations where code reviews are productive even at a rudimentary level:

• The single most important reason for code reviews is that it forces any developer with pride in their work to make sure their code is of good enough quality that they would be willing to show it to other developers.
• The original developer is...lacking in skills.
• A lead developer (who knows what they want) is having someone do the coding for them.
• The code is either simple to grasp or a variation on a theme that is common at the company.

So in answer to your question. I would say don't bother.