Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

On my team, we carry out code reviews (without the use of external tools). The reviewer produces a list of items which the developer should address or consider and then essentially leave them to it (after discussing with them the relevant points) without follow up on their subsequent changes. Occasional spot checks suggest that for the most part all the comments are being addressed.

However, in a lot of literature on the subject of code-reviews there is a strong emphasis on verifying that the code review comments are addressed. I'm wondering what people's thoughts are on this. Does your team have (or had) issues with people not addressing comments? Does your team behave like mine and not do the verification step? If your team does do the verification step, how often is it just a rubber stamp exercise?

share|improve this question

6 Answers 6

The key to success (if there is one) is to do it like the team likes to do it. I've been in teams where nothing gets committed to VCS prior code review, followed by another review if there was something to fix. This suited team fine as everyone had agreed to do it, it was considered valuable and not too rigorous.

In some other teams, this definitely would not have worked. For example, in another team we had members from different organizations, with different skills. Even with much lighter review style we had issues with people being scared of doing reviews or 'being' reviewed. There are obviously more than one explanation for this 'failure', but IMO, the biggest is the lack of 'buy in': people had too different backgrounds and expectations, perhaps lack of trust, to be able to effectively utilize code reviews. In these teams, code reviews perhaps were not beneficial at all.

Team needs to decide whether code reviews are done at all, and if so, how. Start with something that everyone in the team can agree on. Over time, the habits and tastes will change. Then it's time to adapt again.

share|improve this answer
    
Yes, I like the do what works best for the team concept. –  Chris Knight Jul 21 '11 at 9:37

Our code review process requires a sign off from the reviewer. If the changes they recommend following discussion are not made, they don't sign off.

share|improve this answer
    
How often do the reviewers find that changes weren't made? And how often do the reviewers rubber stamp the review without close inspection once the developer says 'Yep, I done all those changes you recommended'? –  Chris Knight Jul 21 '11 at 9:39
    
It's never happened on code I review and any code I write that gets reviewed, the changes get reviewed - and in fact, I show them to the reviewer in the source file. –  temptar Jul 21 '11 at 10:14

Your team hopefully consists of professionals who can and will do the job that they are paid to do. I go to great lengths not to treat them like children by checking up on them and making sure that they are doing what they are supposed to.

Code reviews are done at our desk and everybody involved takes notes to bring to the meeting. The meeting shouldn't last longer than half an hour. Somebody takes minutes and will send an email out to everybody what the action items are for that meeting. Those code review suggestions that were agreed upon are now tasks on the sprint plan.

If these tasks aren't met that sprint then the developer needs to explain why. There is no need at that point to "double-check" him to make sure he did that work.

IMHO if you need to "double-check" your developers completed tasks then your team has a serious skill deficiency, ethical deficiency or both and would need to be corrected at a higher level.

share|improve this answer

I've seen both practices, and in general it reflects the quality demands on the different organizations.

In the environment where quality was essential, issues brought up were assigned a severity. The senior developer in charge of the review would check the changes made, and approve the result (or recommend another review, if the changes were too complex).

In another environment we could just trust developers. Issues brought up during code review were generally formulated as advice anyway, and often it was reasonable to ignore them. When deadlines are tight, some improvements just don't pay from a business perspective.

share|improve this answer
    
excellent points. I like the severity idea. Thanks –  Chris Knight Jul 21 '11 at 10:39

In my team we do live, pair-programming style code reviews. Fixes are often made on the fly while discussing possible enhancements that can be made. We found that direct communication happens to create a kind of contract between reviewer and reviewee so that fixes are more likely to be made and there are a lot less problems than if we used a tool or document as a buffer between the two.

Only written trace we have is that we mention reviewer in source control when committing, which makes it easier to trace which code commits were or weren't reviewed.

share|improve this answer

Code Reviews are built into our SDLC.

  1. A developer is assigned a Task.
  2. Code is checked into Source Control, associated to that Task.
  3. Once all code is ready, the Task is marked for Review.
  4. If Review fails, the developers fixes the code under the same Task. GOTO 4.
  5. If Review passes, the code is promoted to the Integration (Alpha) Environment.

We check the code against our Coding Standards and look for anything that look like it could have a negative impact of performance.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.