views:

374

answers:

6

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...

+5  A: 

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.

anon
I agree that the code author should be involved in the review, whenever possible, and I also agree that the results of a review need to be actionable. However, I think having a single reviewer can be entirely reasonable, for some situations.
Mattias Andersson
Which situations?
anon
Software development situations can vary from "throwaway script" to "nuclear reactor control system" (and the like), so the level of verification required will similarly vary: the former needs no code review, at all, and the latter had better be rigorously reviewed by several very sharp devs.
Mattias Andersson
To give a specific example, though: I think one reviewer could well be enough for an early-cycle change to some mid-importance feature authored and reviewed by competent developers. Note that I'm not talking about a Fagan-style review process; I'm talking about a less-formal one.
Mattias Andersson
+4  A: 

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.

ojblass
+10  A: 

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.)

Mattias Andersson
Rgere should not be a dichotomy of "reviewer" and "author" - both want the same result (improved quality) and should be part of the same team. Yours sounds like a recommebndation for a culture of blame.
anon
"a statement that the reviewer would have checked that code in, himself". That would mean that no code ever got checked in. Everyone has different ideas on what makes quality code. I would be embarassed to check in much of the code that other developers create. So how could they get past my review?
Dunk
I agree with Neil, you do not want to create an atmosphere of blame. Ultimately, the goal of code reviews should be growth and quality. The reviewee should WANT to find out where their code could be improved, and the reviewer should WANT to help them do that. If you lay out consequences,
then you are encouraging them to protect their asses more than try to help each other.
@Neil: I'm not suggesting "a culture of blame"; I'm suggesting that people should take pride in and responsibility for the work they do. Since reviewing code is one aspect of my job, I do it conscientiously. If it's not done properly, what's the point of the review? Checking some box, somewhere?
Mattias Andersson
@Dunk: If I were embarrassed to check it in, I'd be embarrassed to put my name on it as the reviewer. I realize that people can have different styles that fall within whatever guidelines exist, but I think having some agreement on what "quality" means is a pretty important prerequisite for reviews.
Mattias Andersson
@devinb: I agree that growth and quality are two important goals of code reviews, and each one helps both me and the company for which I work.
Mattias Andersson
@All: I get the feeling that the key disagreement is our POVs on what it means to do a code review. I tried to clarify that I'm talking about situations where reviewing code is a fundamental part of the job.
Mattias Andersson
@Mattias: All but one of my jobs were at locations that code reviews were considered very important. I have witnessed first hand for many years the results. By and large, they are a huge investment of time for little relative payback. Unless one of the conditions exist as I stated in another answer.
Dunk
@Dunk: I'm sorry to hear that you (and apparently Neil) have had some bad experiences with code reviews; my experiences have been rather the opposite, even when the author was a very senior developer.
Mattias Andersson
A: 

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.

+7  A: 

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.

Yuval F
+2  A: 

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.

Dunk
+1 for pointing out that asking "How did you test the code?" is vital.
Robert Gowland