93

Recently during code review, my direct supervisor wrote a comment suggesting a change on some code I submitted. Normally, that is not a problem (and I understand that is the very point of code reviews), but it turns out that his concern is based on incorrect information.

More specifically, he raised concerns over a certain language feature, and explained why that could be an issue - except that, in fact, the language does not work as he explained. This is supported by language documentation, and is not a formatting/standards issue. (The language issue is about as complex to understand, simple to fix, and makes as much of a difference as a trailing comma in a C array. That is to say, this behavior has never been changed, it is not hard to grasp that the comma is optional, it is not an obscure concept, and it does absolutely nothing. I'm keeping the exact issue a bit vague on purpose.)

My question, then, is how do I explain to him (if at all) that his idea of how the language works is incorrect? Being at least one or two decades junior to him, I don't want to come off as arrogant. I would normally have no problem with sucking it up and making the change (it's as easy as deleting a comma) - but I also feel like that goes against the spirit of a code review.

Not a duplicate of How do I tell a coworker he's wrong?, since this is specifically about telling your seniors they are wrong..

  • 44
    If you have a process for rejecting comments made during code review, just follow that. – Laconic Droid Apr 29 at 2:51
  • 1
    @LaconicDroid We do not - as I understand it, code reviews go through a process but are somewhat informal at the office. – osuka_ Apr 29 at 2:55
  • 3
    @rkeet +1 for the "missed a changelog" point - this may be a recurring issue from an earlier version of the language that the supervisor recalls fighting with in the past, but has been patched/updated/fixed since he moved into management. Not "What an idiot, you don't need to do that", but rather "don't worry, you no longer need to do that" – Chronocidal Apr 29 at 9:44
  • 45
    Maybe this should have a country tag? OP's username makes me think this might be Japan, where A) hierarchies are a lot more important and B) age is part of that and "one or two decades" make a huge difference. The gist of the answers will probably be the same, but we might have to put in an extraordinary amount of effort (from western perspective) into staying respectful to the senior. I myself think "being honest is my sign of respect", but that definitely comes from my culture instead of being universal. – R. Schmitz Apr 29 at 10:06
  • 13
    The manager's comment may be intended to help the code conform to the code conventions. Many companies expect all their code to have spaces instead of tabs (or vice-versa), uncuddled else blocks, or semicolons in Javascript: changes that don't affect whether the code runs, but which are standardized for readability. – Greg Edelston Apr 29 at 20:47

11 Answers 11

312

Be humble.

I'm a bit confused here. My understanding was that feature X has property Y. Is there something I'm missing here which means this doesn't work in this case?

Much better to start from the assumption that you are the one that is wrong, rather than the other way round. If you're wrong, you'll learn something; if you were in fact right, you've now taught your manager something in a non-confrontational way.

As an important aside, while you say this is different from our other question, it isn't really: you should be humble with your peers and with your juniors as well.

  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. – Snow May 2 at 8:52
  • I find being obtuse to be annoying and passive aggressive. If you think it is wrong just say "I think it works this way, as stated in document X". – user May 2 at 8:53
  • +1. Always a good approach to ask questions during review no matter the position of developer/reviewer. – sepehr May 2 at 19:27
24

You can simply "ask for more clarity" and show willingness to change but subtly indicating that his concerns are not valid. An email to this effect may help:

You have requested following changes in the code because of so and so reasons. However, my understanding of this language is that this problem will not occur because it instead works this way. I suppose I am missing something but could you please clarify or confirm that my understanding is wrong? If you have few minutes, I can show a demo of this and may be you can point out more specifically what your concerns are.

This way you will at least have a documented proof that you tried to correct him back. If he still insist, you can do whatever he requested and just hope it wont be a problem in the long run.

  • 10
    And when asking for clarity, provide a unit test demonstrating correctness of the feature you implemented. Ask for the clarifying comment to be provide in a way that you can make the unit test more complete. – Paul Apr 29 at 19:30
21

In one of your comment you say:

"it is trivial to conclude that the requested change has no effect"

Based on this, the easiest way forwards would be to keep the change and spend your energy on something more important.

Obviously this is only applicable if the change is indeed harmless.

  • 33
    @Mike That's virtually the worst solution imaginable: Passive aggressive and cluttering the code base with commented out code (that's against most sane coding styles). – Voo Apr 29 at 11:17
  • 4
    Your boss may take this as an insult. Can you do this without using the word "trivial"? – Dawood ibn Kareem Apr 29 at 19:45
  • 12
    Obviously I'm not suggesting that the OP should say "it is trivial to conclude that the requested change has no effect" to their boss. I'm repeating a statement from one of their comments here on stack exchange and suggesting that it points to a simpler way forward if they fear confrontation. – P. Hopkinson Apr 29 at 21:43
  • 9
    If the change has "no effect" then the request is a stylistic one. That is an entirely valid request, especially if it is consistent with much the rest of the project's or company's code. The last thing you want is a codebase with a lot of stylistic variations for how the same thing is accomplished. – Chris Stratton Apr 29 at 22:57
  • 6
    @CooperBuckingham sorry to be blunt but did you read the proposed solution before voting it down? Your comment makes me think not. – P. Hopkinson Apr 30 at 8:42
15

If your manager gets this part of the language wrong, is it possible that other developers will also be confused by it? Bear in mind that maintainability by other people should be a goal. So: if this feature is confusing, there's an argument to be made for not using it.

With that in mind, you can go to your manager and -- being humble -- state that you were surprised by his comment, and so you double-checked the documentation. It turns out that this feature is as you described, but you accept that it could be confusing, so you agree that you'll adopt his suggested clearer way to write the code.

  • 6
    I think all this overblown humility is unnecessary, but this answer mentions the comment that I was looking for. Just show the guy the documentation, ask if he interprets it the same way and if the code needs a comment so others won't wonder about the same thing. This situation comes up often when doing code reviews. – JollyJoker Apr 29 at 11:35
  • This is the point: it's something that someone could be tripped over. It could also merit to be documented (e.g note that we are leveraging feature X and so Y is not needed, like it would if we had gone for the Z approach). So next time you would have clear intent and rationale... – Francesco Apr 30 at 18:18
  • 1
    From the example given in the question, the boss is not actually "wrong" - they are requesting something that is functionally unnecessary but also not harmful and will make no difference in the compiled output. Likely as not, they are requesting it because it makes the operation of the code clearer, or brings it into line with company written or traditional standards for the company's code. Code that unnecessarily requires being an expert in language nuances to understand is not code that is good for real world use. – Chris Stratton Apr 30 at 18:21
10

I feel that Philip Kendall answer is incomplete and lacking some backup so that is why I decided to write my own.

I often struggle to make people understand that they said something false but here are some techniques that often works for me:

Technique One

If someone makes an unsupported claim that I'm afraid is false, I ask them for backup. Like:

Oh, this is interesting/disturbing/curious, do you have any source for that? I want to know more about it.

Then, I let them search and they will, hopefully, realize their mistake on their own. Since they are the one who realizes their mistake, they won't feel the shame that often comes with the fact of being corrected.

If this doesn't work, I use technique two.

Technique Two

If the previous technique didn't work or isn't possible, I act confused and ask for clarification. This way, I'm showing that I'm open to the possibility of being wrong myself and that being wrong isn't such a big deal.

I usually start with some basic questions and then ask the more complex ones, like:

Me: So, <some basic question>, right?

Other: Yes

Me: And, <some other basic question>, right?

Other: Yes

Me: So, knowing that, how could <your real question>?

Using this series of question allows you to be sure that you are on the same ground knowledge and that you don't have some misunderstanding somewhere.

When they answer your last question, if you are still not satisfied with their answer, you can ask follow up questions, like:

I still don't understand. When I read X, I understand Y. Isn't that right?

You notice that, during the conversation, I keep blaming myself for the misunderstanding. By doing so, you are not pressuring the other to be right. Since you can acknowledge being wrong with no shame, they will be more at ease to do the same.


I use this technique often and, so far, it's the best I have. Note that, in my experience, heated arguments are really bad and should be avoided at all cost.

  • Technique one is gold. It kind of imposes an "I know that I know nothing" posture to the other person (and often, to ourselves). – Marc.2377 Apr 30 at 0:22
  • 1
    Just be aware that "why" is a challenge. If you can rephrase any of your questions to use "what" or "how" instead of "why", you may get even better results with people. – CodeSeeker Apr 30 at 20:32
5

I don't see the need to be so indirect and "humble" about this.

You can make it pretty polite and simple and clear without coming across as arrogant:

Manager:

If you overflow an integer in C, it's undefined behavior, so add a check for that.

You:

I think unsigned overflow is guaranteed to wrap around in C? Only signed overflow is left undefined. (I see it here.)

This seems just fine as a first response to me. The only way I could see this coming off as arrogant is if you have a back-and-forth argument.

  • 2
    I think this response would be fine to a peer, and maybe a manager. – Mister Positive Apr 30 at 11:17
  • 1
    This is either a bad example, or an example of unwise thinking. Unless you can demonstrate that the operational result of wrapping is still useful to the business or else will be ignored anyway, the boss has a point - you need to handle the overflow. That they mis-stated the nature of the problem is far less important than that they noticed that there is a problem. – Chris Stratton Apr 30 at 18:24
  • @ChrisStratton: Yeah don't focus too much on the example... – Mehrdad Apr 30 at 18:29
  • @Mehrdad I think this is right, but I agree that as a coder, my first reaction to your example is "Yeah, you should handle that". If you added a simple change like "... guaranteed to wrap around in C, which our intended use case XYZ won't be affected by because ABC", my programmer mental alarms wouldn't be on high alert reading your question. – aidan.plenert.macdonald May 1 at 16:52
1

I suggest a matter-of-fact Socratic approach that avoids the need to be either humble or accusatory:

Hi NameOfBoss, about line X in function Y; I believe it might benefit from some additional clarification. Mind if we take a quick look?

This comment states A. Meanwhile, this documentation here states B. On face value, these two statements seem to contradict each other. If this is intentional, I believe it would be prudent to document why A is correct in spite of B. As the author of comment A, can you help me with that?

  • 2
    On the plus side, this leaves open the possibility that the reviewer's change might actually be appropriate. – Chris Stratton Apr 30 at 18:28
0

I would handle it in two ways, depending on the supervisor. If the supervisor is a tech person that works in the language you code in - I would contact them through an email with something along the lines of:

During the last code review, you have commented part xyz of my code as invalid and it got me wondering if my knowledge of how it works was correct. I have checked against the documentation and made a small demo application and it seems to work correctly - should I still make the change or keep it in the current version?

Do include links to the documentation about that code part and a small demo - most developers, both junior and senior, are eager to learn new things in the field they work in and there is always a chance that it was just an honest mistake of your supervisor. It also shows that you make sure the knowledge you use at work is correct, which is usually well received by supervisors, while not attacking the supervisor in any way.

If your supervisor does not work with the language you use (as some teams often use many different technologies and the supervisor has to learn them a little for the sake of code review), and as you mentioned it is not a big change, I would change the code and not address it further - trying to persuade that your version is correct and their knowledge is too small would probably create more tension and negative feelings between you and your supervisor, than it would create good language practices.

  • 5
    "It works in my demo application" is not a good sign that something is correct. You should always go from an actual specification. – Philip Kendall Apr 29 at 13:04
  • 2
    @PhilipKendall Oh how I wish that a specification would always be fully correct (looking at you, MSDN). It's also why i mentioned both documentation/specification and a demo should be included - it lets the supervisor quickly check/debug the code without the need of them writing a demo themselves. – Rachey Apr 29 at 13:11
0

"Thanks for the feedback. After additional review, I see conflict X in your recommendation, please help me understand if this conflict is not the case or I have misunderstood your feedback."

0

As you are saying that do the change is much easier, First you do the change and get it reviewed once again. At time of submitting the changed code for review once again you can simply ask that

Hello Jim, I checked the documentation provided for language and my understanding of the feature X working in language is as like this.

You can then provide a link to the documentation and ask him if what you understood from document is correct or not.

  • Have the discussion before re-submitting, otherwise you're just creating extra work for everyone. – Chris Stratton Apr 30 at 18:29
-10

In reading some of these workplace problems, it brings back old memories where I worked in a SLC, UTAH company, in a manufacturing environment as a programmer. In looking back on it, I burned a bridge which I should of NEVER let happen. It was a bridge that I had to cross many times before. My supervisor was known as the company drunk, but he knew his stuff. He had many problems, but that side of him, was none of my business in the first place. But NEVER tell your immdiate supervisor that he/she is wrong. NEVER! And never in front of his peers or any of his/her supvervisors. That a strike or nail in your coffin. But before you runoff at the mouth on him/she being wrong, first take some extra time plus some extra effort(s) on your part and research it all the problems out where he/she 'might' be incorrect in full and provide alternate answers or solutions to solve the problem(s). Notice the careful wordings in the above statement??? Use words like "'might' be incorrect" NEVER 'Wrong'. Have supporting documents and answers to support your theories. After you've done your extra homework, set up a time to meet with your supervisor in a meeting form so that it fits his/hers' schedule, not yours! And go into that meeting with an attitude showing that you are workable, friendly, and happy and SHOW IT! One thing I learned with this past supervisor, he could read my attitude and body language and facial features and voice - know everything about me. And with all this effort and a meeting, you could either loose or win. As my father would say, your supervisor holds the future of your job within the company you work in. In a sense, you are at his/hers' mercy. Oh, one thing I forgot, DON'T raise for voice for anything! Again, as I look back on this job, I could see places where the bridge was starting to burn. My job as a manufacturing programmer, is just like yours. All my work was reviewed by my supervisor period! And I recieved good marks and bad marks on my work and sometimes I was told to redo my programming work, and it was humbling! So my advice to you as a from manufacturing environment programmer, It's up to you if you want the job, if you like the job and want to stay - it's up to you and your attitude plays a big part in your job. Remember, you are to make your supervisor look GOOD to everyone!! Enough of my preaching... Now enjoy your job.

  • 37
    You really should use newlines and avoid YELLING. – Eric Duminil Apr 29 at 6:33
  • 5
    Also, and more importantly: It is not clear how this answers the question - it looks more like a related anecdote. Please edit and clarify how this answers the question. – sleske Apr 29 at 11:21
  • 7
    And more importantly, I believe most people agree that code reviews are a place of conversation - which means the comments and concerns go both ways. Having code/changes/comments criticized is not, and should not, taken personally by most programmers. – osuka_ Apr 29 at 14:36
  • I think this answer is the perfect example of written "style" and "format" of communication between any two people, but especially engineers...can sometimes matter as much as the content of the communication. – Cooper Buckingham Apr 30 at 15:27

protected by mcknz Apr 30 at 17:07

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

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.