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