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.

I am in a position where I have been asked to review some code that fixes a problem that I don't believe exists.

The fixer, who is more senior than me, insists his fix is necessary but it appears to be no more than C++ sophistry to me. Part of our deployment process is a code review, and as the 2nd highest engineer in a small company I am expected to review changes.

I believe that reviewers are just as responsible for code changes as the original coder, and I am unwilling to accept responsibility for this change. How would you go about rejecting this review?

share|improve this question

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

110  
Seems obvious to me. You point out in the review that the code is a C++ intellectual masturbation whose purpose is to fixes a problem that doesn't exist. And then, as part of the review process, you reject the code. –  user16764 Apr 24 '13 at 21:17
83  
Don't use the words "intellectual masturbation" when you reject it. If you use that wording you will antagonize him, and he won't see what you have to say as a potentially correct technical criticism, he'll see it as a personal attack. It may well be exactly intellectual masturbation, but you can be absolutely certain that he doesn't see it that way. –  Michael Shaw Apr 24 '13 at 21:33
13  
James - I took the emotion out of your question in order to allow the community to focus on your core question. As others are pointing out, using loaded terminology in your review is only going to aggravate what is clearly a delicate situation. –  GlenH7 Apr 24 '13 at 21:38
7  
C and C++ are full of things that appear to work but are actually undefined behaviour. UB is a real defect, even if you can't write a test that shows it doesn't work as it should. For example many compilers use UB as an optimization opportunity, by assuming that the case that's undefined never happens. For example if you used size > size+1 to check for overflow on a signed integer, the compiler may simply replace this expression with false, since overflow of signed integers is undefined. See blog.llvm.org/2011/05/… –  CodesInChaos Apr 25 '13 at 6:52
4  
@MichaelShaw " he'll see it as a personal attack." which the wording of the question sounds like to me, a personal attack on a more senior dev with whom OP has a difference of opinion he can now "win" because he's been put in a position of power. –  jwenting Apr 25 '13 at 7:03

7 Answers 7

up vote 268 down vote accepted

Ask for a test case that fails without the change that succeeds with the change.

If he can't produce one, you use that as justification.

If he can produce one then you need to explain why the test is invalid.

share|improve this answer
227  
Or maybe if he can produce one, you reexamine your assumptions and consider that he might have been right after all. Presumably, the point of the exercise is to improve the code, not to win the debate. –  William Shakespeare Apr 25 '13 at 3:23
92  
Just because you can't write a test doesn't mean it's not broken. Undefined behaviour which usually happens to work as expected (C and C++ are full of that), race conditions, potential reordering due to a weak memory model... –  CodesInChaos Apr 25 '13 at 6:54
25  
Also, what about standard refactoring? How do you write a failing test for a piece of refactoring work? –  Mike Weller Apr 25 '13 at 7:12
55  
"Ask him to prove why it's necessary..." Maybe ask him to teach you why it's necessary. –  Mike Sherrill 'Cat Recall' Apr 25 '13 at 12:55
8  
@RhysW: Wrong assumption. The existing code, with the race condition, cannot be tested in that respect either. So the new code is theoretically better and empirically equivalent. Your assumption only holds out if the old code is empirically better. –  MSalters Apr 25 '13 at 14:01

Reviewers should be objective.

It's clear that you've formed an opinion about the code in question before you've even reviewed it, and it sounds like you and the fixer have staked out positions. If that's so, then you're going to have a difficult time appearing objective, and an even more difficult time being objective. None of that helps the process, and it may be that the best, most objective thing you can do is to bow out on the grounds that you're too close to the issue.

Consider a team approach.

If it's not possible to remove yourself, perhaps you can have several other engineers review the code at the same time. Either they'll agree with you that the code should be rejected or they won't. If they agree with you, then it will no longer be just you vs. the fixer, and you'll be able to make a stronger case that the team looked at the fix objectively and decided against accepting it. On the other hand, if they decide to accept the fix then that too will be a team decision. It should go without saying that you should participate with as open a mind as you can manage, and that you shouldn't try to influence the other team members' opinions by anything other than rational discussion. Important: if there's a bad outcome later, don't throw the team under the bus by saying "Well I always said it was bad code, but I was outnumbered by the other team members."

Rejections are a natural part of the code review process.

The code review process isn't there to rubber stamp fixes from more senior people; it's there to protect and improve the quality of the code. There's nothing wrong with rejecting a fix provided you do it for the right reason, i.e. that the fix doesn't improve the code. If, after an open-minded review of the code, you still feel that the fix doesn't reduce the risk and/or magnitude of a demonstrable problem, then you should reject it. It's not personal, just your honest opinion. If the fixer disagrees, that's okay too, and at that point it becomes a problem for management to figure out. Just be sure to remain honest, open, and professional.

Responsibility cuts both ways.

You said that you don't want to be responsible for this change, apparently because you don't believe that there's a problem. However, you need to realize that if you're wrong and there is a problem, then you may end up being responsible for rejecting the code that would have avoided the problem.

Take notes.

Keeping a written log of the review process will help you keep your facts straight. Write down your thoughts and concerns while reviewing, description and results of any tests that you might run to measure the alleged problem and the fix, etc. If the issue escalates, you'll have a record of what you've done to support your position. If the matter comes up again in the future (it probably will if the fixer is attached to his own view), you'll have something to jog your memory.

share|improve this answer
23  
+1 this is way more useful than the accepted answer –  Simon Apr 25 '13 at 8:54
1  
Definitely. The accepted answer is specific to one case, clearly not as general and well thought out as this one. –  Florian Margaine Apr 26 '13 at 13:50

Write down your reasons for rejection and defenses for likely counter arguments. Then discuss rationally with the fixer and if necessary escalate to management.

If you have sufficient documentation (including code listings, test results, and objective arguments), you'll be well covered.

You will cause some ill will with the fixer, so be sure the issue is worth it (i.e., does the non-fix cause harm?)

Also, if the fixer is in any way related to the owners, forget this answer.

share|improve this answer
8  
I'd up vote twice for the last portion of your answer alone. Very solid answer. –  GlenH7 Apr 24 '13 at 21:39

Recently in LinkedIn news, there was an article about conflict resolution in the workplace and being humble, rather than appearing arrogant. Sadly, I can't find the link now, but the general gist was to try to form questions in a non-confrontational way. Please don't take this as me implying that you are arrogant, it's just the way in which the article was written.

Anyhow, in telling the senior programmer that he is wrong in his assumption will only lead to him taking a defensive approach and attack you back in his response. However, if you pose the question of why he believes the problem exists, from the point of view of your lack of understanding, then s/he is likely to be more open to discuss the matter.

So, rather than say "The problem doesn't exist, so I shouldn't have to review this", perhaps ask something like "In order to review this properly, I need to understand the problem. Can you please explain it from your point of view?"

As an example, the article described a test given to children where an adult held up a cube whose faces all were one colour, except for the one facing the adult. When the children were asked what colour face the adult was seeing, those under 5 years of age would almost always say the colour they could see, as they couldn't comprehend that the adult could have a different view to their own.

share|improve this answer
8  
Excellent point, but I fail to understand how the example at the bottom is related (I don't claim that it's unrelated, off course, just point out my lack of understanding of the relation). –  ugoren Apr 25 '13 at 14:43
8  
@ugoren Ha ha, I like what you did there! –  Merlin069 Apr 25 '13 at 15:01
1  
@ugoren the relation is 'unless you can see the whole picture, dont form concrete opinions' –  RhysW Apr 25 '13 at 15:01
2  
I always like the humble approach, I would reject the change on the basis that I don't understand and I'm therefore, not qualified to give it the ok. –  PhilDin Apr 25 '13 at 17:06
1  
@PhilDin There's humble and then there's going around saying you're not qualified for the job. If he's in a position to review code, I think the people that put him in that position expect that he's qualified enough to do so. –  Andy Apr 26 '13 at 12:37

C/C++ can be full of the undefined behaviours. One one hand it is bad as it can lead to the unexpected behaviours. On the other hand it allows aggressive optimization and usually when you are using C/C++ you are interested in speed.

Writing test case which breaks may be hard - it might involve strange architecture or compiler which does not exists anymore. Also it might seem like on any sensible architecture "it shouldn't cause any problems".

However at one point or another you will change platform (say - you want to go mobile so you port to ARM or you want to speed things up and use GPU). At this point things may start to break and you will need to debug it. It might be as trivial as updating the compiler to newer version (and you might want it/need it).

The problematic code was:

int d[16];

int SATD (void)
{
   int satd = 0, dd, k;
   for (dd=d[k=0]; k<16; dd=d[++k]) {
     satd += (dd < 0 ? -dd : dd);
   }
   return satd;
 }

During last iteration d[++k] => d[++15] => d[16] is accessed. Since usually the next element was legal memory (in terms of paging not C memory model) even the trivial compilers did not produce any executable with strange behaviour. The only way of writing the test case was finding platform with exactly the same memory model as C (probably VM).

However gcc 4.8 prerelase seen that d[++k] is executed in every loop. So k < 16 otherwise the access would be illegal and the legality of program fed to compiler is part of contract. So the loop condition is always true given the assumptions so it is infinite loop. This might sound strange but it was perfectly legal optimization - emitting system("dd if=/dev/zero of=/dev/sda"); system("format c:") would also be correct replacement for loop. There are more subtle ways you can choose to exhibit behaviours. For example during lecture about Transactional Memory I remember that presenter tried several times to get wrong value when 2 threads incremented the same value.

However C/C++, as oppose to some languages, is standardised so such dispute can be done with reference to objective source:

  • If you think that this is not a problem try to prove it using C/C++ standard (i.e. that it leads to defined value and behaviour)
  • If he thinks that this is a problem ask him to prove it using C/C++ standard (i.e. that it leads to undefined value or behaviour)

In general if your team is writing in C/C++ it's useful to have standard at hand - even team experts can find something strange there once in a while.

share|improve this answer

This sounds more like a problem with your team policy than with this specific review. When I worked at a large software shop on a team of 9, a reviewer flat-out had veto power over code, and this was a standard we all respected. We were a talented and savvy enough - viz. "mature", but more in the sense of "not childish" than "seasoned" - that our team lead could reasonably expect us to always be able to come to an agreement without devolving into arguments in a prudent time period.

Simply based on your language in your post, I might warn you that it sounds like you're going to approach this situation in a way that might lead to a devolved argument. Perhaps it is "intellectual masturbation," but there is probably a reason he or she did it, and the burden on you will be to come up with a simpler way to solve that problem - and if no such way exists, document in comment why the masturbatory code was, in fact, necessary.

share|improve this answer
1  
"... but there is probably a reason he or she did it ..." - That's a big assumption you are making. And you are also missing the point that the Question states that (in the OP's opinion) no problem exists. Surely, the onus should be on the person who is proposing a "fix" to demonstrate that there is a real problem to be fixed. It is meaningless to propose a "simpler solution" to a problem that doesn't exist. –  Stephen C Apr 25 '13 at 1:57
4  
@StephenC yes, and I stand by the opinion that the OP should give the benefit of the doubt in this case. Or else it's going to be a really confrontational review. –  AAA Apr 25 '13 at 11:15

Make the submitter show proof that his code fixes his problem. If he can test, and show you proof, then you are the one who is wrong and should just quit your complaining and deal with it. If he cannot show proof to support his claim there is a problem, and show proof that his patch fixes the problem, then I'd send him back to the drawling board.

Of course there could be sensitive internal political-ness around this. But this is the way i'd go (delectably).

share|improve this answer

protected by MichaelT Jan 11 at 19:21

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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