Cyufjy

In the company I work for, change requests go through various steps, including a development step, a peer review step and a testing step. On this particular change request, I have been assigned as a developer and a colleague developer has been assigned as a reviewer.

This colleague is the one who trained me originally. He has been with the company for over 10 years, while I joined a year ago, just after university. He's got more years of experience as a developer than I am old. Despite that, he is still my peer as we are both "mid level" (no longer junior but not senior) developers.

This particular change request involves an old, complicated function. It is badly written according to any code standard, but it works. It took me a few hours to even find what I needed to change. The client wanted a different order of evaluation for determining the initial value of a field (e.g. check client.address before debtor.address instead of the other way around).

My colleague noticed I was taking long and looked at the source code with me. He pointed out some code and said, "You have to delete that". I had my doubts about that, but I tried it anyway. Trying it made me realise where the problem was, though. In the end, I had to switch two statements around.

I documented the change, tested it and it worked as intended. I forwarded the change request to my manager, who verified it worked and forwarded it to the reviewer. I received it back a few minutes later with the comments "that won't work, you changed the wrong code" and "you didn't do what I said".

I said that I tried what he said, but it didn't work. The code he referred to was unrelated to the change request, except that they were doing something with this same field. His response was that what he said was just a hint, because I need to be able to figure out things on my own.

Now he refuses to pass the change request to the testers until I fix my mistake. I asked him for more details, and he keeps saying I need to figure it out on my own, because I need to learn. I need to start with deleting the code he mentions, according to him. Since that code has nothing to do with the change request in question, I am very reluctant. I asked our manager, and he said to present my argument to the reviewer, since the manager doesn't know about programming. I tried that, but the reviewer is adamant in his point and dismisses me with "just do what I said".

Partially due to the age of this code, there are no unit tests and they cannot be written for them without rewriting half the codebase.

How do I fix this situation?

He might be correct that the code needs fixing, but he's going about it the wrong way for any code review I've ever seen. Code reviews should fail for very specific, addressable points, eg:

1. Inconsistent indentation in ScoreHandler.cpp, lines 45-50. Violates style guidline #8.


2. 
   getConsistentReview() in Foo.java is not thread safe as required, synchronizing on the wrong lock.


3. 
   getFoo() in Foo.java is iterating over a list to filter out results. Since our source level is now 8, this should use the stream API to filter instead, which is much more concise.

It sounds like you've got someone who's (badly) trying to mentor you by getting you to "fix" code in a way that he wants, without telling you exactly why it's broken or what's wrong with it. It's not even clear if the reason he wants you to change it is stylistic, or functional.

You could just refuse to play ball and push it back to him. Whether you think that will work overall, or whether it'll cause more problems depends on the culture.

If you want a softer approach, try writing back with more detail of what you tried, why that didn't work, and state exactly what you need to proceed:

Hi x,

I've previously tried deleting y code as you suggested, but that instead causes z to happen, resulting in the function failing (these) tests in (these) cases. I'm very unclear on the requirement here - is the change that you're asking for a stylistic one or a functional one? If it's stylistic, could you point out the specific style guidelines the code is violating, and if it's functional could you provide me with a test case that fails?

None of the above is unreasonable to ask for in a code review - it's more unreasonable that it wasn't provided to start with.

If he still says "just do what I tried" after that, you can then just push back with "As above, I'm afraid that didn't work and I'll need much more detail in order to solve the problem." If anyone comes down on you for taking a long time, show them the paper trail; it should become clear pretty quickly that he's the one dragging his feet and refusing to play ball.

In the workflow at my company, that would be easy.

Mark the change request as “won’t fix” with a comment “correct fix rejected by reviewer”, then assign the change request to him. Obviously he knows how to fix it, so that should be a five minute job for him.

And actually, if he was correct, then that would be the right thing to do. No point in wasting your time. So there is nothing he can complain about.

With ancient and obscure code, two things can happen: I leave it unchanged, or I improve it and take responsibility if it breaks. The one thing that is not going to happen is that I order someone else to make those changes. And most definitely not during a code review.

This might not apply to you for technical or organizational reasons, but the general answer is:

Make a unit test

A unit test is proof a bug has been fixed. Every time you fix a bug, or every time you need to demonstrate a bug, unit testing saves a lot of arguments. See if there's a QA person you can talk to and get this one sorted out.

Remember he didn't reject your change because it didn't work, he rejected it because it wasn't his change. Having a unit test back you up strengthens your position.

Specifically for your situation: Remember that if he wants you to do what he said, he should do it himself. It's your task and you get to do what you say. That's your mentality, now for the dialog.

Call the developer over and talk with him. Demonstrate that your fix works, and if you're asked why you didn't do what he said you can say something like this:

This is not exactly what you said but it works and fixes the problem. I understand this solution in my head, the other way would probably work as well but would've taken me a lot more time to figure out. So I think we can mark this one as Passed in the code review?

I agree with berry120. However your review notes were

"that won't work, you changed the wrong code" and "you didn't do what I said"

My suggestion:

(Spend no more than half a day or so on this. Concisely document relevant code behavior, errors / warnings, your actions taken and reasons for doing so.)

• make sure your new code works in Version X (no errors or warnings) according to client request


• create a fork or a version of the code prior to your changes


• do exactly as the notes state (attempt to fix errors if any), this is now Version Y


• if these changes won't address the client request make special note of this and attempt to include Version X into Version Y


• inform (email) your reviewer and manager:

I believe I have finished implementing the client requested changes in Version X.

To address the review notes I took the following additional steps for Version Y.

List here your progress and findings, include your documentation of this process (keep it short,5-10 lines max.).

If it helps readability, you also can have this list at the end of the email and refer to it accordingly.

If the review notes didn't address the client request:

(the following will explain your delay and initial non compliance with the notes)

As far as I could tell (see my report above/below) the review notes alone wouldn't address the clients request and I additionally implemented Version X.

I wrote Version X first to assure the clients request was addressed before adding other code changes.

Should Version Y still have errors you couldn't fix:
(It probably will be the review notes or the addition of Version X to the review notes - adjust wording below accordingly!)

I'm happy to continue working on Version Y.

Unfortunately integrating the review notes with Version X (the working client requested changes) is proving more complex than submitting Version X alone and I'll need more time allotted.

I'm on the fence about including the following to appease your reviewer somewhat while at the same time pointing out to management that his changes are causing the errors. It might also cause management to interpret it that you still need the reviewers supervision.

insert reviewer name, if you have a moment I'd greatly appreciate your guidance regarding the motivation behind (or possibly less inquisitive:"the nature of") your code changes and the resulting errors I'm getting for Version Y.

Obviously use the actual version numbers instead of X / Y (;

EDIT:

To make my answer more readable and concise I changed it.
Please leave comments if this is better. thanks.

Something similar happened to me - my first job after college, a senior developer had to make a change to support something I was working on, but the change he made did not work. What ended up working for me, and what I suggest you do is the following:

Get another senior developer involved, schedule a meeting with both of them and explain your view point. If at all possible demonstrate the tests you ran. Then let him explain his approach. Figure out what's the best fix is and do that.

Make sure you don't come across as adversarial, something along the lines of:

Hi Mike. It seems we keep talking past each other about bug #123. Do
you have 30 min on Monday to go over it? I'll have Jim joint us since
he's been working on that lately