views:

442

answers:

6

So I am trying to introduce code review to my teams. But a considerable number of peers fear that the political aspect might ruin the otherwise good teamwork.

Have you encountered politics problems when you do code review? If so, how do you avoid it?

Or more precisely, how to do a proper code review so there's least politics involved?

+3  A: 

If your development teams indulge in a lot of political infighting, cure the infighting before you attempt to to do code reviews. Also, the review should normally only be conducted by the team who wrote the code - reviews are not intended to allow any Tom, Dick or Harry to come along and make comments.

anon
+1  A: 

Make a Bylaw for coding standards for your team.

NinethSense
Byelaw n. : a rule made by a local authority to regulate its own affairs
chakrit
Yes, if there is strict instruction on what to do and what not, then there is no room for personal attacks (politics!). Let us quantify everything.
NinethSense
And then how do you "quantify everything" without politics?
chakrit
aha :). Well, eg: you can count number of times programmer violated the syntax and semantics of 'if' statement.
NinethSense
+16  A: 

G'day,

Edit: If you approaching the review as an exercise in "I don't trust you so I need to check what you're doing" then you're missing the basic reason for reviews and any review is likely to be less than successful.

An author's assumptions that their own code is perfect every time is, if not optimistic, then at least misguided.

People are human and they can, and will, miss various things in their code. A second or third pair of eyes will quite often pick up things that the author completely missed. Sometimes these things can be blindingly obvious and yet still missed by the author of the code.

There are several proven ways to ensure reviews are successful:

  1. As Neil B. pointed out, only team members involved in the code can be involved in the review. Unless of course there is good reason otherwise, e.g. the team is using a utility or maybe middleware written by another group and you want to verify your usage of the provided utility/middleware/etc.
  2. Emphasise that the project is a team effort and the team should be working together to ensure that the project, i.e. everyone's individual contribution joined together, is the best that they can do. It is not dev A's chunk which was better than dev B's chunk.
  3. Emphasise the learning possibilties available to the team from team members participating in the review. There cumulative years of experience of the typical software team is typically quite large. Getting all that experience pulling together in the same direction to make sure the best product goes out the door is a very powerful tool, especially for teaching the less-experienced of the team.
  4. Always emphasise that it is the code under review. Not the person.
  5. Always use language like "this function fails to test for negative inputs" and not "You forgot to check negative inputs"
  6. Do not allow long discussions/rejections/justifications in the review.
  7. There are only three responses possible during the review, accept the change, reject the change, take offline to discuss later.
  8. The code fragments must be provided before hand.
  9. All participants in the review, apart from the facilitator who is running the review meeting, must read the code beforehand.
  10. Time must be scheduled to allow reviewers to read through the code.

BTW You might like to look at Fagan inspections which takes the above points even further.

Edit: Sorry but I forgot to mention that you must have some coding standards in place. Otherwise your reviews will easily descend into "holy wars" with battles overthings like:

  • brace style,
  • TABs or no TABs in the source,
  • indentation levels 4 spaces or 8 spaces
  • etc.

Having a coding standard in place removes such discussions from the equation. Mind you you'll still have such, er, "exciting" discussions when making the coding standards! ;-)

HTH

cheers,

Rob Wells
Restricting to three possible responses seems to be a good way to go.
chakrit
That was good in principle but you might be shooting yourself in the foot by having a hard limit. Maybe as a suggestion?
Rob Wells
+4  A: 

If you're afraid of politics, don't name them "code reviews" from the beginning, rather introduce publicly and progressively some :

  • naming standards
  • coding conventions
  • development rules ( like always do a certain thing in a certain manner, or never do a certain thing )
  • development guidelines
  • develoment best practices
  • etc.

All this in the name of improving maintainability, readability, respecting standards, reducing bugs, etc.

By enforcing those little by little, you'll be doing your code reviews, but the focus will be on improving existing code ( with a positive connotation ), not blaming someone for writing bad code ( "code review" might be easily perceived as a negative thing - hey, I know you're writing poor code or I don't trust you so I need to check what you're doing )

Billy
@Billy, sorry mate. bad approach. if you're approaching it as an exercise in "I don't trust you so I need to check what you're doing" you're missing the basis reason for reviews. People are human and can , and will, miss various things. A second or third pair of eyes will quite often pick up things that the author completeky missed.
Rob Wells
@Rob - mate, I was saying that *it can be perceived* that way when you announce some guy : "hey, I have to do a code review on your work" ...
Billy
+3  A: 

Be the first to be rewiew. Dont bring the thing like "ok guys, now i will be on your soulder, better have some nice thing to show me".

Antoine Claval
+1  A: 

I found this blog article extremely helpful, when we introduced code reviews:

http://www.basilv.com/psd/blog/2007/strategies-for-effective-code-reviews

Regards, Ovanes

ovanes