How to reject a code review that you believe is unnecessary?

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

C/C++ can be full of undefined behaviours. On one hand, it is bad as it can lead to 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 exist 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 a lecture about Transactional Memory, I remember that the presenter tried several times to get wrong value when 2 threads incremented the same value.

However, C/C++, as opposed 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 the standard at hand - even team experts can find something strange there once in a while.

Answer 6

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.

Answer 7

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

Answer 8

Get over yourself, review the code and check if there are any bugs in it. Someone else with more experience decided it’s needed, you have only your opinion against that.

Consider your position in the company, and what it means for reviews of your own code. You are making enemies for no good reason. And in a way that won’t be easily forgotten. You’re basically saying that only one of you two can be on the team.