91

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?

Telastyn
  • 108,850
  • 29
  • 239
  • 365
James
  • 4,328
  • 3
  • 21
  • 25
  • 115
    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
  • You should review some code or fix it? – ott-- Apr 24 '13 at 21:26
  • 86
    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
  • 15
    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. –  Apr 24 '13 at 21:38
  • 2
    @user16764 that would be the perfect answer. underlines the **positive nature of reviewing** and the **absurdity of refusing** to review instead of rejecting code. – ZJR Apr 25 '13 at 02:16
  • 1
    I agree with ZJR. By refusing to review code you are agreeing with it, in my view. If I refused to use code that hadn't been sufficiently reviewed nothing would get done. What am I going to do, hold busy people down and MAKE them look at it? That would be popular wouldn't it? So silence is assent. – Zan Lynx Apr 25 '13 at 03:50
  • 8
    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 http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html – CodesInChaos Apr 25 '13 at 06:52
  • 5
    @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 07:03
  • 2
    You're not "refusing to review code" by rejecting bad changes, you've reviewed the code and you found it to be bad, so you "reject" the change. It's not a "code review" if all reviewed code is accepted. – Lie Ryan Apr 25 '13 at 07:18
  • 2
    The way you described the problem, it seems you don't think his change would be harmful - just unnecessary. If that's the case, I would simply approve the change and forget about it. Not every battle is worth fighting. – Nemanja Trifunovic Apr 25 '13 at 12:36
  • 1
    @NemanjaTrifunovic - look back into the revisions of the question and you'll see that the OP used much stronger language in initially asking the question. The terminology used was detracting from the question, which is why I edited the question to tone things down. –  Apr 25 '13 at 14:20

8 Answers8

280

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.

Craig
  • 4,462
  • 3
  • 19
  • 13
  • 252
    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. – Caleb Apr 25 '13 at 03:23
  • 2
    Nice; I must say, though, "code review" is not "testing/QA". This is an appropriate reaction from a tester's perspective. Of course, with a properly structured team, developers don't make code fixes without a pre-existing open "ticket/issue" – Euro Micelli Apr 25 '13 at 03:23
  • @EuroMicelli: Really? You file an issue/ticket for every change? This amount of process leads to large extensions of estimate times. A test case should be sufficient. Or maybe I misunderstand and a ticket is just a quick entry of "bug on line 17: will fix" – Zan Lynx Apr 25 '13 at 03:52
  • 5
    @ZanLynx: Yes, in our team, changes are not made to code without an open ticket that justifies the change; QA is signed-off by a business specialist, and this process is audited on a yearly basis by an outside firm. I know this is not everyone's cup of tea, but it's part of the business when a malicious change or a poorly-coded one can have financial implications for your client. Don't worry; we don't need a 12-person committee's written consent to do some refactoring; we just need to document what we're doing. It's not as bad as it sounds. – Euro Micelli Apr 25 '13 at 04:43
  • 101
    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 06:54
  • 30
    Also, what about standard refactoring? How do you write a failing test for a piece of refactoring work? – Mike Weller Apr 25 '13 at 07:12
  • @EuroMicelli: I'd usually just use version control's commit message to document those minor changes. The bug tracker keeps track of major bugs/feature request and the changesets associated with the bug, while changesets that don't have an associated entry in the tracker are documented by their commit message. Duplicating the documentation for every minor changes into the bug tracker seems like it would create lots of noise in the tracker. – Lie Ryan Apr 25 '13 at 07:28
  • @MikeWeller: refactoring generally reduces the amount of code or at least they should stay roughly the same, significant increase in the number of lines of code that doesn't produce anything testable is a sign of overengineering. – Lie Ryan Apr 25 '13 at 07:31
  • 7
    @CodesInChaos if it cant be reproduced then the code written to 'fix' cant be tested either. And putting untested code into live is a worse crime in my opinion –  Apr 25 '13 at 08:21
  • 3
    What I meant was: if the "fix" the colleague is making is a refactoring operation or just a change in style, he can't write a test for it whether it is legitimate or not. So the "get him to write a test" isn't the best advice IMO. – Mike Weller Apr 25 '13 at 08:47
  • You might not be able to produce tests for dirty code that does the job. Doesn't mean you shouldn't rewrite it, it depends on the context. – Guillaume Apr 25 '13 at 10:13
  • 3
    I agree with this answer. Obviously, the changes are not just refactoring. In the question, it says *"The fixer [...] insists his fix is necessary"*. I would go to him and simply ask him : "Can you please show me why it's necessary, maybe with a failing test ? I don't get why you did this, and I'd like to understand before accepting or rejecting the changes." Talking is the best solution here. Silently rejecting the changes and acting like nothing happens is not a good idea. Ask him to prove why it's necessary, but be kind so he doesn't get angry. – rdurand Apr 25 '13 at 11:54
  • 64
    *"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
  • 1
    @RhysW was originally going to ask as a comment but decided this should be a full question: http://programmers.stackexchange.com/questions/196105/testing-multi-threaded-race-conditions – Dan Is Fiddling By Firelight Apr 25 '13 at 14:25
  • I suppose you could say that it must either pass a test that otherwise fails (and that test needs to be supported by a documented business rule) or it needs to increase readability (as perceived by reviewers) and/or reduce redundancy. Perceived performance is NOT a valid reason for a change unless it meets requirement 1 (is a business rule and fails a tests). – Bill K Apr 25 '13 at 16:35
152

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.

Caleb
  • 38,959
  • 8
  • 94
  • 152
40

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.

Dan Pichelman
  • 13,773
  • 8
  • 42
  • 73
31

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.

TheDarkKnight
  • 486
  • 3
  • 6
9

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.

Pang
  • 313
  • 4
  • 7
Maciej Piechotka
  • 2,465
  • 2
  • 18
  • 19
  • In this specific case, the code would be horrible even if the array size was 17. Probably 30 years ago I’ve seen similar code creating a “segment violation” because the array was right at the end of a segment. – gnasher729 Jan 24 '21 at 10:11
  • @gnasher729 If someone wrote this code today I would complain in review about readability. But point stands that writing a test which trigger the behaviour may be hard or impossible and yet the code has latent bug. – Maciej Piechotka Jan 24 '21 at 19:25
4

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.

djechlin
  • 2,212
  • 1
  • 15
  • 26
  • 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 01: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. – djechlin Apr 25 '13 at 11:15
3

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

SnakeDoc
  • 361
  • 2
  • 12
0

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.

gnasher729
  • 42,090
  • 4
  • 59
  • 119