What to say in a code review when I don't know how it could be improved?

Question

I've been asked to review some code changes and when I see the code, well... I just don't like it! There must be a better way of doing this, but I can't think of one off the top of my head.

So I ask the developer and they tell me the reason for doing it this way, but I'm not 100% convinced. (I can even suggest another way, but they point out a complication with doing it my way, which I think could possibly be resolved / worked around.)

The problem is now that my choices are either

• I take their word for it and approve the changes,
• I spend hours myself researching the problem, trying to come up with a better way or trying to resolve their complication
• or worse - send them away with the vague, open ended statement to try and find a better way themselves which either might not exist or might be beyond their ability, in which case it would be both a waste of time and demoralising / insulting.

None of these seem like good solutions

What is the best practice for handling this scenario?

Edit (in response to comments)

Thinking about it a bit more - the problems are usually along the lines of introducing complication or maybe poor development practices, e.g.

• the code is now overly complicated (but the dev feels that this complication is required).
• They've introduced new methods, classes, interfaces whereas I think the change could be handled using what we have already. (the dev has identified a snag with this which would take some work to figure out whether this can be rectified).
• it breaks our current style / design patterns (again the dev feels that this is necessary in this case but I'm not so sure).
• it introduces certain "brittleness" - e.g. hard-coding or ineffective use of interfaces. (again the dev feels this is the only way to make it work)
• it may introduce potential for hitherto unknown problems to arise, or problems surrounding scenarios that the dev believes "will never happen" and I can't prove otherwise.

In each case, the point is that off the top of my head I can't give them a better alternative, and the only way of doing so is for me to invest so much time in figuring one out that I might as well have done the work myself, and there's a chance it'll end up that they were right all along.

Answer 1

"There must be a better way of doing this, but I can't think of one off the top of my head."

Can you write this in the code review? I might hedge a little add some uncertainty about whether there is a better way to do it since you can't prove that the better way exists, but if you feel sure you can say so.

A better way existing doesn't necessarily mean someone has to find it though. Not all code has to be perfect.

If your code review process requires you to approve or not approve the code then consider whether the code is good enough to live with for now. Are all the change introduced by the PR good compared to what was there before? If so then consider approving even while leaving a comment to say you think something better is possible.

Answer 2

I've been asked to review some code changes and when I see the code, well... I just don't like it!

Always be honest with how you feel. Don't defend your right to feel the way you do. It's how you feel. Invite the author to care about how you feel. Explore why you feel that way.

I don't want my code to make people feel bad. I care about how it makes them feel. Code that people don't like comes at a cost. I don't want to pay that cost if I don't have to.

There must be a better way of doing this, but I can't think of one off the top of my head.

Oh there's always a better way of doing it. We can debate how to skin cats until our company files for bankruptcy. That there is a better way is not the issue. The issue is that you feel so uncomfortable with this way. The question is, why?

it breaks our current style

I'd wager this is why. Adding complexity, new methods, brittleness, and introducing unknown problems, are all things every solution does. But breaking our current style is jarring. It makes the code hard to read. It violates the Principle of Least Astonishment. And most suspiciously, this is the sort of thing a new coder might do. Yeah, someone who is new to the shop, who doesn't have the current style imprinted on their bones yet, might do exactly this.

But if that's what happened then it's your fault. You know the current style. You didn't step in and impress this style on the new coder, who didn't know any better, way before they finished.

Now they want to be done and aren't willing to listen to vague instructions like, "do it our way".

Nope, sorry, tell me how to do that when I'm planning to code. Not when I'm itching to pick up my next ticket.

But you let that ship sail. So now you have a much harder job. Getting a coder with a working solution, a dead line, and a desire for sleep to care about how you feel.

Well, you don't feel strong enough about it to fix it yourself. You don't want to spend the time researching workarounds that would let the solution stick to the current style. So you're going to have to work with this guy.

It's always good to feel people out. Get a sense of if they care about the cost of breaking the current style. This harms readability. Which is very expensive. If they care, but just don't see how to do it, then you teach! Show them every bag of tricks that you use with your style. Help them master it. If you can get them interested then challenge them to do the best they can with the time they have. Sit with them and pair while you both figure this out. You may learn some new tricks yourself.

But if you can't inspire them to care about that then the only thing you can do at this point is consign them to updating documentation until you rewrite this in the current style. Because if you don't stop this here it's only going to spread.

Answer 3

A code review should be actionable - meaning that the person reading your comment should know what action you are suggesting needs to be taken, unless it's obvious you should also state the reason why the action should be taken (are they going to ask you why, if you don't state it?).

If you reverse the roles (imagine you were the one receiving the feedback for a different piece of code) I doubt you would consider the comment "I don't like it" actionable. I will also point out that just Approving without providing any additional feedback, is basically saying - the work/code you are doing is acceptable and you should continue doing things the same way.

Organizations typically use code reviews to achieve several goals (possibility multiple goals at the same time), these may include:

1. To act as a double check to try to catch problems before they are pushed further down the pipeline (to a main development branch or in some cases prod).
2. To provide feedback to a developer about the quality of their approach/code.
3. To ensure the Dev team are on the same page with respect to any boundaries they have collectively agreed to adhere to.

What action you take is going to be dependent on the nature of the problem you suspect.

If you genuinely believe that the the code is introducing a bug / will not function correctly you can (and probably should) take some time to go through the problem and satisfy yourself that the proposed solution is good. However this is a question of batting averages, if you do this once a year no one is going to have a problem with it. If you do it on every single review your manager is probably going to talk with you about your productivity - hence you had better be right a high percentage of the time.

Assuming that you believe that the code is functionally correct, but you "just don't like it" you need to ask what it is you don't like. The root of the problem appears to be that you don't TRUST that the other developer has evaluated enough options and picked the right solution.

Trust - may be a you problem or a them problem, if the other developer has a history of screwing up (not thinking about all options, jumping to conclusions, etc) then it is appropriate to bring up how it is impacting your ability to get your job done, you will need (many) examples and details about exactly how much impact it is having upon your ability to get your job done. It then becomes managements problem to resolve the situation.

If these types of problem occur frequently, but not to specific people (the problem happens to everyone) then you may have a systematic problem:

• Do you have suffient test coverage?
• Are your requirements clear?
• Do you have enough time to complete the tasks?
• Has time been allocated for planning / discussion of designs before coding starts?

It's not appropriate to bring up these types of issue in a code review, however they should absolutely be brought up in a retro.

Finally if:

• There is no systematic issue.
• The code will function correctly.
• The other developer is generally pulling their weight.
• You can't think of any specific action the developer should take.

Then it's a you issue - approve the PR and move on.

Answer 4

I just don't like it!

This is not productive feedback. It's okay for developers to express personal preference for approaches, which is just casual banter; but from the perspective of a code review your subjective like/dislike is irrelevant.

A PR review should contain meaningfully actionable feedback.

There must be a better way of doing this, but I can't think of one off the top of my head.

While on the lower end of the spectrum, this is meaningfully actionable feedback. The current code is problematic. It's not always immediately obvious what the better approach is, but that doesn't prevent you from pointing out problems.

I don't know how to fly a helicopter, let alone fly one well, but if I see a helicopter stuck in a tree I can point out that that's not right. (Credit to Steve Hofstetter for this joke)

I'm not 100% convinced.

This goes back to the initial point. It's an opinion, not meaningfully actionable on it own.

I can even suggest another way, but they point out a complication with doing it my way, which I think could possibly be resolved / worked around.

Sometimes, it's just better to then do it. The issue is that if you only limit yourself to verbal feedback and not code changes, that it can devolve into a game of 20 questions for the author to figure out what it is that you want.

A very common scenario here is when the author is decidedly more junior than the reviewer, and the proposed fix is not something that the more junior dev is experienced with (e.g. further optimization or high-level concepts).

If you know it can be done better but you can't express it with words, then express it in code. Sometimes a 5 minute snippet is enough to convey your feedback, sometimes it's just easier to do it yourself.
Not because the author shouldn't fix their code, but because you're convinced there's a problem and you've not been able to convey that to the author. Doesn't matter whose "fault" that is. It's a matter of there being a problem, and you being more capable of fixing it.

The problem is now that my choices are either [3 options]. None of these seem like good solutions

You've not really revealed a concrete example, and there's more than one answer here.

• Sometimes, the current code is unacceptable and should never be allowed to get merged. For example, SQL injection or other security issues.
• Sometimes, the current code is acceptable but there's interest in seeing if there's an even better way of doing it. But this effort should be timeboxed. Since the current code is already acceptable, there's no point holding it up for an extended period of time. In this scenario, unless an improvement is readily available (relative to the urgency of merging the PR), you can just accept the change and backlog further improvements to it.
• Sometimes, the current code is already good, and your feedback is more a matter of a different personal preference than it actually improves things.

At the end of the day, the purpose of a PR is to prevent bad code from going out. Further improving good code can be a meaningful activity, but there is a cap on how much effort is too much for the PR. On the other hand, there is no time cap on how long you should spend on fixing bad code, because bad code is just not acceptable.

Answer 5

• the code is now overly complicated (but the dev feels that this complication is required).
• They've introduced new methods, classes, interfaces whereas I think the change could be handled using what we have already. (the dev has identified a snag with this which would take some work to figure out whether this can be rectified).
• it breaks our current style / design patterns (again the dev feels that this is necessary in this case but I'm not so sure).
• it introduces certain "brittleness" - e.g. hard-coding or ineffective use of interfaces. (again the dev feels this is the only way to make it work)
• it may introduce potential for hitherto unknown problems to arise, or problems surrounding scenarios that the dev believes "will never happen" and I can't prove otherwise.

These considerations from your edit come under the general heading of "code smells and accruing technical debt".

The best way to handle them depends on how your team tracks and manages technical debt, how receptive they are to clean-ups and other non-functional changes, and the individuals involved. Very often, that best way will not be within a code review.

Sometimes I just make a mental note of something that looks brittle, and keep track of whether we ever see any real variation in the relevant dimension. Maybe we don't, and removing the brittleness would be would be extra work to no real benefit. If we do see variation in that, I can quickly point out where we need to clean something up to support the new case.

Sometimes I'll raise a backlog ticket to simplify, standardize, refactor or otherwise polish something that technically works but is causing some friction. If you never get around to low-urgency backlog tickets this might be ineffective, although it can always be promoted if the friction starts to directly affect a higher-priority ticket.

Sometimes I'll just add a comment saying

// if we ever want this to support X, we'll need to do Y

to make it clear to anyone who ends up trying to add support for X, that it's a known limitation.

What I generally try to avoid in code reviews is anything that can degenerate into bike-shedding, as these points could, unless the risk is going to be realized soon. Even then, it might be simpler to do the refactoring in in whatever ticket realizes the problem, rather than adding future-proofing to the code under review, when the future-proofing isn't logically related.

Answer 6

What is the best practice for handling this scenario?

One solution not explicitly mentioned is to send the work to be re-analysed by someone else.

This could be sent to someone of the same grade as the original developer - a fresh perspective on a problem sometimes helps find a new solution, as can a leg-up from the first developer who has found all the hard edges already but has ran out of steam before pulling a proper solution together.

Or it could be sent to a higher grade of developer - perhaps but not necessarily yourself - if the work so far has revealed unexpected difficulties that make it no longer appropriate for the original developer.

Obviously, when an existing solution isn't great but there's no clear way to improve, then there is no avoiding the essential dilemma of whether to accept what's in hand, or whether to have someone work on searching for better.

Answer 7

Really, you need to be asking yourself:-

• Does the code work correctly?
• Is the performance acceptable (think of the famous Donald Knuth quote "Premature Optimization Is the Root of All Evil")?
• Is the code quality (layout, naming, etc.) good enough to maintain in the future?

If you can answer "yes" to all three, then nothing needs to be done. Accept the code and move on.

Answer 8

It was someone else’s job to find a good solution. If you think it’s not a good solution, and you have good reasons to believe this, you put it in the code review. Then the person writing the code should think about this. Possibly talk to you, or to other experts. But not being happy with the code doesn’t mean you have to know how to fix it.

On the other hand, if this is just an opinion of yours which you pulled from thin air, then you are just wasting people’s time.

So add it to your review if you have good reason to think the code is bad.