37

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.

9
  • @Ivella We are using Github, however, his project wasn't source controlled, so there is no backtrace that he did it Apr 22 at 14:24
  • The "he [who] commented" is a 3rd person, yes? Apr 23 at 0:32
  • Has the existing code been running in production? How much has it been tested? If it hasn't been validated much, there's a roughly equal chance changes will break it, fix it or keep it broken or fixed (but hopefully leaning towards fixing it, since there are more pairs of eyes on it), so fear of breaking it shouldn't factor into the decision much. Apr 23 at 7:23
  • 1
    @DanielR.Collins indeed he is another reviewer Apr 23 at 14:02
  • 2
    @RegularNormalDayGuy: It's not tricky, there should be no reason for your reviewer to need to prove it. Just tell him what you just told us. You didn't write it, so you cannot say why the standard were not followed. You can also suggest making the changes yourself if required.
    – musefan
    Apr 23 at 15:13

14 Answers 14

106

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.

6
  • Comments are not for extended discussion; this conversation has been moved to chat.
    – Kilisi
    Apr 22 at 15:17
  • 1
    +1 for “it's science.” And yes, actual science (not the mathsy stuff one learns in high school) is just as messy as debugging software.
    – DomQ
    Apr 23 at 8:45
  • 11
    This is really not a viable answer. If the code already doesn't have tests, it means the company doesn't care and want tests. OP is not going to single handedly change the company culture.
    – Davor
    Apr 23 at 12:35
  • 5
    The coding tasks should simply not be marked "completed" if there are no tests to prove it. I doubt the "superiority" of the superior developer who decided to hand off a large codebase to a new person without showing working tests. Apr 23 at 13:21
  • 3
    I think even the most careless company culture would not forbid the questioner from writing tests, even if they are only for his own use. Apr 23 at 22:50
21

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.

11
  • 4
    I would leave the name of the original developer out of the message. Do not blame the original developer. Just write that you did not wrote the code and that you opted for not making any changes which could break stuff. .
    – some_coder
    Apr 22 at 8:25
  • 33
    "I didn't write this" can be okay, but what purpose does "X wrote it" serve (other than to promote a blame culture that can be quite toxic and counter-productive)? Apr 22 at 8:59
  • 8
    @BernhardBarker - the word "blame" is only applicable when you think the someone did something wrong, which the OP wasn't suggesting. In this case, naming the original author is more of an "appeal to authority" with the subtext being "maybe you don't recognize me as being an authority on this subject but the original author of the code is". It also helps point the code reviewer in the right direction for whatever follow up questions they might have.
    – neubert
    Apr 22 at 11:50
  • 3
    @neubert appeals to authority were ok for Middle Age scholastic discussions, but not for code reviews. Managers are not necessarily good programmers. Good programmers today were not necessarily good programmers ten years ago. The code could be good but the standards have changed. And even good programmers sometimes write bad code. Would you keep buggy code only because who wrote it? Saying the name of the programmer seems pointless at best and defensive at worst.
    – SJuan76
    Apr 22 at 22:26
  • 6
    Get the original and make it as "first commit"; change author to your superior but leave yourself as committer then slap all your changes on top of that - stackoverflow.com/a/18754896/1564365 cheers.
    – Kyslik
    Apr 23 at 0:00
17

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!

5
  • 3
    "The world's software, with a little bit of plastic thrown in." LOL... many a business disaster caused by that attitude ;-)
    – Pete W
    Apr 21 at 15:19
  • 4
    I'd go with "out of scope" as the most modern way to say "not touched yet in this pass"
    – Mohair
    Apr 22 at 14:18
  • good one @Mohair
    – Fattie
    Apr 22 at 14:50
  • 2
    The number of times I've seen something along the lines of comments like: // HACK - This needs to be fixed ASAP! with a date on them over a decade earlier... I generally feel like if you see something wrong and just slap a comment on it for the next guy, it's just never gonna get fixed. Apr 22 at 17:23
  • @DarrelHoffman - my God, when were you reading my code ?! :)
    – Fattie
    Apr 22 at 17:25
6

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.

2
  • To abdicate blame for code, you can't submit it for code review. A prerequisite to that, for any sensible form of development, is to ensure the code is correct and of sufficient quality. Putting forward the review is rather explicitly saying that you have taken on the responsibility for its contents. Apr 22 at 22:56
  • @MatthewRead - that is 100% company dependant. And most companies don't do any sensible form of development. You are awfully confident for someone so blatantly wrong.
    – Davor
    Apr 23 at 12:39
6

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

1
  • "It does raise the question why someone was working on a project for 6 months, without having it reviewed" Amen.
    – Kevin
    Apr 23 at 14:03
2

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.

2

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.

1
  • I never ask for permission to refactor. It's like asking for permission to use flour when your job is baking cakes. Though I almost never refactor code unless it's part of the task at hand.
    – E L
    Apr 22 at 21:21
1

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.

3
  • Earlier in the question, the OP says that the original code had not been reviewed yet, so that is included in the immediate changes they've put forward for this review. So the teammates are doing the right thing here, and not just looking elsewhere for issues. Apr 22 at 22:52
  • 1
    Maybe the first commit message needed to be "Initial commit; code is as inherited". That way the OP can claim "the code is this way because it was like that when I got here." :) All kidding aside, this answer is correct: the code review should be limited to changes, not to a philosophical discussion of every other thing that might be improved. That's a different (or several) ticket...
    – spuck
    Apr 23 at 22:49
  • @MatthewRead: The part that the code hasn't been reviewed before was added later to the question after my answer. spuck's suggestions is a nice way to work with that fact. Thanks. Apr 26 at 5:52
1

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

0

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.

0

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
0

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

0

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.

-1

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.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .