6

I was reviewing a pull request of a programmer that works under me, and found that in testing fixes to their code, I ended up completely rewriting it.

Should I push my changes and then explain what I changed, or should I scrap my changes and just try to explain to them how to fix their code?

I don't want to offend them or take advantage of my position as code reviewer, but I also don't want to force both of us to spend a lot more time on the task (both them in rewriting their code, and me in explaining how to rewrite it).

EDIT:

  • It's safe to assume I didn't rewrite the code for no reason. It had fundamental issues, which couldn't be fixed in really any other way. I can post the code, but it's probably best just to assume my changes were required.
  • I've been in daily contact with the programmer over the task, all of the changes were previous requested (both over slack, and in person).
  • The code doesn't use any existing code in our code base. It is purely a one-off algorithm, there isn't product knowledge to learn, and I've already given them excellent resources to learn the concepts (MDN links).
  • The programmer has performed adequately on other projects, I'm not sure why they struggled with this project, but I have no reason to consider suggesting termination of the employee.
  • The algorithm in question isn't conducive to unit tests. It is specifically animation code, which is controlled by user input, so the output is visual movement which is somewhat ambiguous (although it has states which a human would clearly identify as incorrect).

OUTCOME:

To provide an update for the outcome of this specific case, I had a discussion with the programmer.

I started by mentioning that I made some changes, just to get that out of the way. Then I asked them about specifics of their code to try to determine why they went the route they did, and have them come to conclusions about specific issues with their code on their own.

After the initial part of the discussion I went over my changes (I felt it was more diplomatic to simply call them 'changes', rather than 'a rewrite'). I tried to go over the larger concepts first, as their coding style wasn't an issue, and then drilled down into more specific lines to show them how I implemented the larger concepts.

Knowing them personally, I assumed they wouldn't be too upset over my changes, but I was surprised at the degree to which they weren't bothered. They didn't have any specific reasons for why they wrote their code the way they did, beside it seeming to work, and after explaining my changes all they really had to say was "so if you've already implemented these changes, I don't need to do anything else, right?".

@Flater's spectrum of options is very useful, and I will likely be revisiting them in the future.

However, it seems for this specific task and programmer, I simply failed to identify their disinterest in the task before their implementation became a mess (that they weren't excited about fixing). They seem to be interested in other tasks, and have done acceptable work in the past (otherwise this would be a different discussion), so ideally I would identify this early and ask if they were interested in other tasks before this becomes an issue again.

Improve this question
1
  • 2
    What does your boss/employer say about this? Ultimately they're the stakeholders paying for everybody's time developing and maintaining the project, and they own the project, Anything that either of you have produced belongs to them, so it's really their decision to make, and they should be made aware of the fact that they've paid twice for two different people to get this bit of work done in two different ways, since they may want to intervene and discuss ways of ensuring it doesn't happen again (better next time would be to talk to them before taking action. After the fact is too late). Commented Jul 14, 2022 at 6:48

5 Answers 5

Reset to default
9

As a baseline, fixes should be implemented by the author, not the reviewer. This is to ensure that the author learns what needs to be fixed, in the hopes that they learn to do it right the first time in the future.

That being said, if this person has a very junior profile and/or is new to the development teams practices or specific patterns being proposed, it can be beneficial to not just rely on letting them figure it out for themselves.

Depending on the complexity of the changes and how junior this person is (relative to the complexity), there are several options here. I consider this a spectrum:

  • Develop the alternate solution yourself, and then explain/demo the changes so that they learn to do it the right way.
  • Pair program, with you performing the changes. This is essentially the live coding version of the above bullet point.
  • Pair program, with them performing the changes and you answering questions and providing immediate review as they make the changes.
  • Have a talk with them to explain your review and let them ask any questions that arise.
  • Sit nearby them (or open a video call) for any questions that may arise, but without you actively being involved until they ask for your assistance.
  • Standard process. You provide annotated review comments, they process the feedback by themselves.

It's okay for a junior profile to need this kind of assistance, but I would hope to see them progress towards the ability to do this independently. I would generally expect that per feature they develop, they would "upgrade" to the next bullet point (or more).

but I also don't want to force both of us to spend a lot more time on the task (both them in rewriting their code, and me in explaining how to rewrite it)

The question is not whether it takes more time to have them do it themselves now, but whether having them do it themselves saves time in the long run, because they'll learn more from having to do it themselves instead of having you do it for them.

Generally speaking, it's better to let the author work through it; but there are exceptions where an author may be too junior to confidently handle it or you're pushing against a very short term deadline. These are exceptions to the rule, however, and should not be frequent.

Improve this answer
7

Your task as a reviewer is to review, not rewrite. Review has two goals, ensuring the quality of code that your organization produces, and advancing the skills of those who write the code. If you just check in your rewritten code, the original author will most likely not learn from it, and you're missing on the second goal.

For now, it would be best to spend more time by getting together so you can explain the issues you found and help the author find a better way (which wouldn't necessarily be exactly your way.)

In the future, it might be helpful to restrain yourself to just noting the problems, and get together with the author soon for one-on-one fixing sessions if the necessary fixes are more than just typo corrections or simple bounds or validity checks.

If it turns out that the coder isn't improving, you'll need to consider other options, such as providing them with additional training, or releasing them from the job if they're unable to fulfill the requirements.

Improve this answer
1
  • Thanks, I added some additional details in my question. I've been in daily communication with them over the past 2 weeks. Both over slack, one-on-one (at their computer, and mine), and we even got a white board involved at one point. Commented Jul 14, 2022 at 4:33
0

If the reviewed code works and isn't harmful, let it pass. Briefly explain what is unsatisfactory. Create a new issue to improve it. Talk to the developer, make your case and assign the new issue to him.

And you may all live happily ever after.

Oh, and don't talk about your rewrite. Ever. To anyone. Keep that in a safe place where no one can see it.

Improve this answer
8
  • Ah, I wish I could briefly explain what is unsatisfactory. In my experience whenever many changes are required, a lengthy explanation is also required. Commented Jul 14, 2022 at 4:30
  • 1
    While there are things that you can let slide and fix later; this is a very slippery slope into technical debt and one that developers and managers alike are really easily attracted to. Because of this, I would strongly urge to not make it common practice to approve and complete a pull request despite of genuine review feedback. If it doesn't pass the smell test, it shouldn't pass. The exceptions to this are so few and far between, and the consequences of wrongly doing so can become so severe, that we should avoid enshrining this as a possible approach in general. Commented Jul 14, 2022 at 7:27
  • You need to do several things in one: Accept the change, merge the change, create a change request, assign it to the developer at highest priority. If your system allows it mark the original change request as “not done” so it stays in the system, until the second set of changes happened. And the assumption is that the change is “not harmful”. Harmful changes are never merged. Commented Jul 14, 2022 at 12:39
  • 1
    @Flater I was trying to make a point about team etiquette. Rewriting a subordinate team member's code behind his back under the pretext of a review is a good reason for the other person to look out for a better team. Commented Jul 14, 2022 at 13:10
  • @MartinMaat: Under the assumption that a subordinate is a more junior profile - or at least lower in the pecking order as to how the codebase should be structured; I disagree. "If the code works" is not the sole criterion on which to pass a code review. Quality of implementation, general style, and consistency with the existing codebase matters. Is it generally better to instruct the author? Yep, as per my own answer. Are there cases where the author may not be able to implement the proposed changes on their own in a reasonable timeframe, without it reflecting badly on them? Also yes. Commented Jul 14, 2022 at 13:14
0

Should I push my changes and then explain what I changed, or should I scrap my changes and just try to explain to them how to fix their code?

Rather than explaining what's changed, why not tell them to add tests that will require them to fix the code? The easier way to specify what the code supposed to do is having tests that verify it works as specified.

The first thing is to make sure the code pass tests. More senior people should be able to see why a piece of code passed at tests now but would create issues down the line, at which point they should suggest what tests to be added.

After their code pass, then you can make suggestions that some piece of code in the patch is already exists in the code base and they should use it instead of adding redundant code.

This is probably slower than pushing your own changes, but it helps people grow into whatever they're expected to be. It also prevent stress because usually juniors are more defensive in code review.

Improve this answer
-2

If you believe that code need to be changed, there is a "gap" between what you know is right and what developer build. What I would do is think about this gap, what exactly developer do not know fundamentally, that my cause this difference.

It should be something simple, i.e. some algorithms, or some basic engineering practice.

Explaining all the reasoning why rewrite is needed, would just take to lot of time (as you correctly pointed out), but explaining "gap" expected to be straightforward.

You can't push rewrite (this is bad practice :( ), otherwise you will need to do this every time in similar situation or take a risk that next reviewer will miss this issue.

Since the only way is to explain, you may consider small talk with team or coffee break as one of good options. But ultimately the "gap" should have small and simple explanation, sometimes this require analysis and a lot preparations but also help you to build your own mentoring skills.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.