How to handle requests to change code written by my superior

Question

My superior has been working on a project for about 6 months (creating a new robust system to replace an old one we have). Lately, he's having less time to work on it and thus, he handed the task to me to continue his work. The work he had done up to the point where he handed it off to me has not been code-reviewed.

The codebase was quite large, and some corners were cut round; functionalities could be split into multiple classes, coding standards were not all respected, etc. The logic, however, is really good (as expected, he is a lot more experienced than I am).

Anyways, I made my changes, but left some critical parts untouched, since I can break the whole project. We have a code review process where all our changes must be reviewed by our peers. During the pull request, a reviewer commented that critical part and asked why it was written without respecting our standards and why it's so different from the rest.

How do I handle the situation? Should I say that it was my superior (who is also his superior) who wrote it or should I just say that I will change it? I don't want to sound like I'm blaming my superior, but at the same time, I'm not sure how to make the changes correctly.

Answer 1

You seem to agree that the code changes are necessary, but you are afraid to "break" something.

That is a real problem. If you cannot make sure it works after you change it, that means you cannot verify whether it actually works now. How did it ever end up in code review? You don't know whether it works, it doesn't matter if it's neat and maintainable, you don't know whether it works! That should be your top priority.

So as a first step, find a reliable way to make sure it works. In an ideal world, that means automated tests, but maybe you first start with a pen and paper checklist of testcases it has to pass. Write them down, so you can reproduce your tests.

As a second step, test your current program with these cases. Make sure it works.

As a third step, make the changes required in as many small steps as you need. After each step, test, test, test.

When you have verified that it still works, your job is done.

Software development is not fudging it und guessing it should work probably a bit. It's science. You provide proof that it works. Only then your job is done.

Answer 2

I would say something like:

I appreciate your feedback but do bear in mind that I didn't write this code. [insert name here] wrote it as git blame [or git show ...] shows. I opted not to make any changes to it out of concern for breaking stuff. Let me know if you still want me to make the requested changes.

The dev doing the code review may, upon reading that, be like "oh that makes sense. well in that case I'll go ahead and approve it!". And if he isn't and if stuff does break because you were updating the code to be consistent with the companies coding standards then your comments may protect you from possible blowback. ie. if shit hits the fan and you're asked why you would risk making changes to that code you can point back to that comment that you made and the reviewers response.

Answer 3

During the pull request, he commented that critical part and ask why is it written without respecting our standards and why is it so different from the rest.

The whole world runs on code debt!

Every single thing you use every day - your car, your TV, your coffee machine, your bank, and the robots that make every single thing like your underwear and your pencil - is just software. The world's software, with a little bit of plastic thrown in.

And of that software, all but a handful of pieces of said software have code debt.

Your response is trivial,

that's from the original, not attended to yet

really you'd usually just type

not touched yet in this pass

This is the most common thing any programmer types in a lifetime :)

If (for some oddball reason) you felt the need to expand, you might type

that's from the original, not attended to yet. not sure of the priorities, could PrimeBoss suggest on priorities?

Note that,

I don't want to sound like I'm blaming my superior

(1) there's utterly no need to mention "who" wrote the previous pass, it's a non-issue

(2) every single time, ever, that any programmer has written anything, it has been seen as a stinking pile of crap, often in as little as a year, and certainly with three years. Note that all the code you're writing now, you yourself (and everyone else) will see a a joke to be erased from existence in the next pass!

Answer 4

The reply: "Well, that's how [superior] wrote it, and it wasn't a priority for me to change it, plus it seemed too risky. If you like, I can ask [superior] if he thinks that changing this code is worth the effort and risk".

So you are not shifting the blame because the reviewer isn't going to blame your common superior. And to be honest, don't ever accept blame when you're not to blame, and there is a good way out.

Answer 5

"My superior wrote it" is not an excuse.

You are the person submitting it for code review, which means it's now your responsibility. (It does raise the question why someone was working on a project for 6 months, without having it reviewed, but that exceeds to scope of this). You made the decision to leave the code untouched -- which is still a decision you must justify during a review. "Someone else wrote it" is not a good argument.

And remember, during a code review, it's code that's being reviewed. Not people. That works both way: code doesn't get a free pass because it was authored by X, nor should criticism on code be taken personally (yeah, yeah, I know, this is hard). The goal is to deliver a better product, not to assign or shift blame.

If the outcome of the review is that it's better to make changes, and you're too afraid to make them on your own, ask help. Don't be afraid to ask help from the original author, even if he is a superior. Noone who has been working for half a year on a new project expects to have written only trivial code. Make a plan how you are going to address what comes out of the code review, formulate what your concerns are ("If I do X, this may break Y, how can I assure it didn't break Y?") and ask for some time to go over it.

One final thing, being afraid to break something isn't bad in itself. But that should never paralyze you. Ask yourself, how can I minimize the risk it is breaking something? (Unit tests have already been mentioned as a tool to minimize breaking things (but realize that unit tests alone may not be enough)). But in your case, it's brand new code -- intended to replace a complete system. Odds are, when it's going to be rolled out, it will break things, whether or not you modify the critical section of the code which is subject to this question. You will want to slowly and carefully roll out the new system, and be ready to roll back on a moments notice.

Answer 6

The first thing I would do here is write some tests to work out whether the code is functioning correctly and to ensure you understand it properly. If it all works as intended I would not touch the code; but if it doesn't then I would do the 'clean-up' work while I was fixing the real code problems. I would personally avoid thinking about the "rank" of the original author as much as possible, "superiority" is very often a poor proxy for quality, especially if they completed the work under time-pressure.

There are 3 basic principles at play here:

1. The code must work as intended
2. Just because a 'superior' wrote it doesn't mean it is better code
3. There is a cost to any code change

Point 1 "The code must work as intended".
It seems like you broadly understand the code written, but is it tested? Can you actually prove it works?
If not, then I would suggest your first step is writing some acceptance tests for it. They will show whether the code work as intended or not. Then you can either point to these in the code review and say "I know this is not according to the standards, but it passes all the acceptance tests so I am happy to leave this in it's current state"; alternatively it fails the acceptance tests and you will need to fix it in any case.

Point 2, "just because a superior/senior wrote it, doesn't mean it's better code".
Nobody is perfect and anyone can write bad code; many very senior people have written much very bad code! If your reason for not touching it is "a senior person wrote it" then that's not a good reason. First thing you should do is make sure you understand it (maybe write some tests?), if you are confident you understand what it's doing and why then you should be able to better explain in a code-review why you chose to leave the code as-is.

Point 3. "All code changes have a cost".
The majority of "cost" in any software project is maintenance and any changes make that maintenance job incrementally harder. It may seem small to reformat some code to a standard, but without tests (again) what if you introduce an unexpected change. You still need to understand the code, but perhaps you remain unaware of a case where a line-break is functional in JyC#++ that the code relied on. Further your name is now on the git history and any questions about it will come to you. If you understand it all and have tested it, this should be no problem; but if not ,then you've taken on a responsibility to forward queries to the original author to explain the purpose, which adds work over those queries going directly to them.

In summary, my inclination would be to ensure I understood the code and tested that the functionality worked correctly. After that I would be personally inclined to leave it as-is unless changes are required, with the intention to come back and make any improvements at a future time as and when they are needed.

Answer 7

I am fortunate enough to be a "superior" in this situation. But make no mistake, I fully expect a developer reviewing my work to call me out on things that are wrong with it. Perhaps reviewing it initially should have been the thing to do when taking over the project and highlight lacklustre areas up front.

In regards to the review after the fact, if your answer was something along the lines of "You wrote it like this, I've picked it up. I've improved a number of areas to be inline with our standards but didn't feel comfortable changing this critical area because there aren't any tests. Do you want me to write the tests and continue the refactor?". That sounds like a totally reasonable response and is completely understandable. I would say yes to the tests and give you time to clean it up. It also gives the chance to reconsider why corners were even cut there to begin with.

How do we all learn if not for contribution and making the things we write better? You should absolutely be challenging everyone, superior or not. That's what makes you a good developer. Hopefully this helps.

Answer 8

Anyways I make my changes, but left some critical parts untouched, since I can break the whole project. We have a code review process where all our changes must be reviewed by our peers.

According to that, he should review only your changes. It is commandable that your team looks beyond the immediate code, on the other hand there is no bound to what else can be improved in other parts of the code.

I assume this this not your final one-time commit before launch, but rather one (larger) step towards that? Do you track further development stories? If so, I'd recommend creating a story for refactoring specifically the critical part, for adding unit tests (the answers suggesting unit tests are absolutely accurate, that is an important part) and then telling

This is old code that I haven't worked on yet. It will be reworked in [link story]. This PR is for [describe feature/link current story] only.

So don't mention anyone personally. You have rightly focused on just one thing at a time. Tracking future work is still important and having some input or suggestions form your collegue beforehand may help.

Answer 9

You say the original Supervisor who worked on the code has less time, but you don't say they are totally unavailable. Therefore you should go back to them and ask.

The other answers are correct that you are now tasked with making everything work, including the tricky bits. But if the Supervisor is a more experienced programmer than you and the code reviewer, they may have had a valid reason why it was done that way or it may need some tidying up that they didn't get to yet.

To the code reviewer you should truthfully say that that part was amended by the Supervisor and that you will discuss it with them and see what their reasoning was. This is not blaming, it is a statement of fact and leaves the possibility that the Supervisor was correct.

When you talk to the Supervisor, you can also ask what other changes hare needed in that area and get clear direction on what to fix and explanations for anything you don't understand

Answer 10

It's okay to change the code. Requirements change. Context changes. People gain more domain knowledge. Sometimes code that was meant as a proof of concept makes its way into production. Even senior engineers make mistakes.

I have had my ego bruised a few times, but I can recall only one situation where I got angry about someone changing my code. I had mostly fixed a bug, but there was a subtle boundary condition that wasn't correct. I wrote a failing test for that condition, then went on a two-week vacation.

When I returned, someone had deleted my test and merged the partially broken code. They didn't understand the purpose of the test, so they just deleted it.

So before you change the code of someone more experienced, you at least need to understand what it was trying to achieve, and if that purpose is still applicable. If you're afraid you will break something if you change it, it isn't tested well enough. Do that first.

Answer 11

Seems there are quite several ideas on the abstract question how to manage (or accept) the code quality as a single output dimension. I'm more economist than coder so I'd add the decision about effort. Short, talk. Set the goals together with your superior. Without that you may be smart or lazy or good or evil depending on your choice about the coding, but I'm afraid you have bad chances to be successful in your current job.

I think your boss, unless you have an immediate reason for job search, needs to be able to handle the situation when you make decisions on code quality and thereby shift the position he created. However, you need to involve him for a discussion on priorities. Those decisions are much more important to meet at higher level - the technical part can be delegated better than the resolution of conflicting goals for the team. You need to spell out how much it takes you to get the code in par with company standards (and, as the responses hint, common sense about reliability). You need to share your thoughts about the minimum to get across code review formalities and the extra you need to also get comfortable with the content. You two need to plan where the target is. Don't get yourself either doing something your manager does not want to spend time on or not doing something (s)he expects to be done. The more senior you are, the better you can estimate your managers' expectations, but you can only attain that seniority by gathering input. Don't fear to ask, don't fear to present your concerns (or if you have good reasons to do so, coding style proposals won't help you out).

Answer 12

The PR is done on code changes. So your peer's question about the code that wasn't touched shouldn't be a change request, but just that - a question why the code was written that way. If they do want to bring the code up to the coding standard, that should be a separate PBI and a separate PR.

Answer 13

So basically Alfred is your boss and Bob is boss of both, and Bob wrote the code.

Since VCS can blame the author of a code fragment, you are not blaming Bob for non-standard code, but you can just truthfully say that the code being reviewed was not made by you, and is old inherited code.

Your superior (Alfred) will have responsibility of decision. If he decides that you have to refactor the code, then it's just your direct boss's task. I understand that the code was written cutting corners because there was not enough budget/time to write clean code. Alfred should take this into consideration too.

Answer 14

Similar to what neubert said, I'd remind the reviewer, in this case your boss, know who wrote it and that you are still working through the code to update it to the correct standards. You can go on to say that you simply haven't gotten to that specific piece of code yet, but will in future commits.

I'd also suggest doing this is an informal way, like a brief (5 minute) talk in the hall or as they pass your desk. This is not to throw anyone under the bus, but to get you out from under it. Then simply put in the code review notes something about the future commits and leave it at that for now.

You also shouldn't mention the "fear of breaking things" in the notes. You can make a side comment about it to your boss, but they will probably just remind you that the code needs to be up to standards and not to worry about breaking it, since it's also your responsibility to fix it. :-)

I've had reservations of rewriting code of a boss, but I also took a few breaths and just did it. It didn't always feel great, but it was needed. Eventually, I got over those nerves and, given time, I'm sure you will, too. Believe it or not, there's nothing really special about your boss or your bosses' boss. They are humans, too, and make mistakes. They just get a bigger paycheck while making the mistakes.

Recap:

• Informally remind the reviewer that it's not all your code
• Add neutral (non-blamey) notes about future changes
• Keep pluggin' on