views:

440

answers:

11

Everyone that has ever had to perform any form of code review has probably been here. You're a member of a code review team, and during the code review you come to an issue where a member of the team is overly aggressive about their viewpoint of a solution, whether it is right or wrong.

How would you handle such a situation, especially if the solution presented is wrong?

+8  A: 

If a coworker feels that strongly about an implementation, offer to work with them to refactor the code and then compare the maintainability, performance, and robustness of the new solution over the old one. It totally deflates the argument if you say, "Maybe you're right. Let's do extreme programming tomorrow and see if we can figure out how to make this better."

hoyhoy
For the most part I would agree here. Generally my approach is to take that specific issue offline, where we can work out the specifics. Then that allows the discussion to continue outside of the highly stressful / personal context of the review itself.
Jason Mock
+3  A: 

Criticizing code is difficult. You tend to hurt feelings. That's what makes a code review difficult on the human level. To answer your question it would be helpful to know the circumstances. If the code fails, who will be held responsible in the end? Is the code review team considered just a help for the programmers, or has it been installed to make up for mistakes which the programmers are known not to fix?

Thorsten79
+1  A: 

If they are getting aggressive then perhaps they should not be taking part in the code review from that point on. Aggression does not help the process in any way.

Garry Shutler
+2  A: 

Often times when someone aggressively protects their design, it's because they are operating within their comfort zone (golden hammer), don't know of any other design alternatives or patterns, or are simply trying to be difficult with or resent the reviewer or review process. In these cases, I wouldn't tell the designer that "they are wrong" but rather attempt positive reinforcement and constructive criticism.

Ask them to go over how they came to their current solution. What alternatives they may have considered, and why they arrived at the one that they did. If there's obvious flaws in the design, politely point them out, and make positive recommendations for alternative strategies they may have not considered.

The software design review process should be approached like a two-way street. Don't dictate, attempt to resolve differences of opinion with respect and professionalism.

Kris
+1  A: 

Make an in-house coding convention guideline. You can table hot potatoes during the code review and discuss it later with everyone else in the team and put that in the convention.

eed3si9n
+2  A: 

In the scenario that the solution presented is wrong, I would suggest the following:

1) Ask for clarification as to why they believe their solution is correct. You may discover some cases where your solution isn't a fully fleshed out as possible.

2) Then ask the person how his solution handles the likely issue of X. If they have an answer that accounts for the risk in a reasonable manner, then consider their scenario. If it doesn't, goto step 1.

3) If you have a chief architect or sr developer, and the person is unyielding after trying steps 1 and 2, appeal to that person. Make your case to him/her and ask for guidance. In that scenario, just follow the guidance.

Unfortunatly, you'll also have to gauge the political environment as well. If the person saying it is a major stakeholder, you're going to need to step more carefully, as political fallout could make it harder to do your job. That doesn't mean to stay quiet, just to be wise in your wording :-)

torial
+1  A: 

I find it difficult to handle the situation where I really want to fail a code review, but don't want to unduly hold up a release.

Failing a code review would necessitate a code change, and a code change would mean repeating (some) testing; repeating testing may be time consuming and we do actually want to ship code, after all.

Normally how I work is:

  • Is a problem found during a code review liable to cause a significant bug? If yes, then fail (If there is an obvious bug, it should be raised against the release).
  • Can the problem be fixed without creating unnecessarily large amounts of extra work? If yes, then fail. This is useful if the review is done before system testing has already begun.
  • Otherwise pass anyway, but ensure that the issue is noted so that it can be corrected in future code, in maintenance to the existing code, or if the code is changed anyway later in the process (for example because a bug is discovered during further testing)

I've found this is a good compromise between failing anything and never failing anything.

We don't yet have good guidelines for code reviews; I generally play it by ear.

MarkR
"between failing anything" - do you mean, "between failing everything"?
Flubba
+2  A: 

Giving and recieving critique is in some circumstances a bit difficult. If someone comes off as aggressive during a review, wether they are on the receiving or the sending end, it means they have some kind of ownership over the code. It will hurt like hell when their code gets smashed to pieces.

This is very common in not only programming, but also in other creative workmanship. I have witnessed myself graphical artists or illustrators who overzealously put pride on their work tend to get rather pissy if someone comes with anykind of constructive criticism.

Getting angry over a review is all a fallacy because YOU ARE NOT YOUR WORK. And people tend to forget that because we easily get attached to stuff.

Depeding on the kind of agressivity shown by any member, if I was the moderator of a code review, I would have taken some kind of a time out and let the member talk it out in a seperate room. In a code review we help people with their code so they can achieve their goal, not to banter them.

Spoike
+1  A: 

Though it might be too late in your situation, it helps immensely if you have predefined the purpose, specific goals, and non-goals of the CR. Also outline the process, and define an arbitration procedure, if necessary.

Then, during the CR, allow the interviewee to defend his code according to the predefined goals. If he disagrees with your evaluation (e.g. purpose is security, goal is preventing SQL Injection), reality can be the final arbiter... just penetrate his code. Or, if it's more subjective (or not as directly provable), apply the defined arbitration procedure - for instance, the senior developer/architect/pm/etc.

Worst case, just agree to disagree. This might be relevant depending on the status of the CR - is it required before checkin, is it a consultant audit, is it just a formality, etc.

AviD
A: 

Self control. Explain your solution for several times, using different arguments and if that doesn't settle things down, try to get a third person's opinion. If that doesn't work either, make sure that you really have a better solution and get more people to know about it. Then wait for the code to fail.

radu_c
A: 

The best way to attempt to stop this behavior is at the beginning of a project, before it starts: make it clear that the code base is "a communist state" (in a purely theoretical sense). By that I mean either:

  • Everyone owns every part of the code, so that people need to speak up when they see something odd; or
  • No one owns anything, so that people should not get upset when their code changes (either through review or maintenance).

How to deal with it once it's happened? I think hoyhoy's answer is the best: engage the criticizer in an effort to make the code better.

Peter K.