In code reviews, should the reviewer always present a solution for issues? [closed]
-
12-01-2021 - |
italiano
english
français
española
中国
日本の
العربية
Deutsch
한국어
Português
RussianQuestion
When reviewing code, I normally try to make specific recommendations on how to resolve the issues. But owing to the limited time one can spend for reviewing, this does not always work well. In these cases I find it more efficient if the developer comes up with a solution himself.
Today I reviewed some code and found that a class was obviously not well-designed. It had a number of optional attributes that were only assigned for certain objects and left blank for others. The standard way to resolve this would be to split the class up and use inheritance. However in this specific case this solution seemed to overcomplicate things. I was not involved in the development of this software myself and am not familiar with all modules. Therefore I did not feel knowledgable enough to make a specific decision.
Another typical case that I experienced many times is that I find an obviously meaningless or even misleading function, class or variable name but am not able to come up with a good name myself.
So generally, as a reviewer, is it fine to say "this code is flawed because..., do it differently" or do you have to come up with a specific solution?
Solution
As a reviewer, your job is to check if a piece of code (or a document) meets certain objectives that have been agreed upon before the review.
Some of these objectives will typically involve a judgement call whether the objective has been fulfilled or not. For example, the objective that code must be maintainable typically requires a judgement call.
As a reviewer, it is your job to point out where the objectives have not been met and it is the job of the author to make sure that his work actually meets the objectives. In this way, it is not your job to tell how the corrections must be made.
On the other hand, just telling the author "this is flawed. Fix it" does usually not lead to a positive atmosphere in the team. For a positive atmosphere, it is good to at least indicate why something is flawed in your eyes and to provide a better alternative if you have one.
Besides that, if you are reviewing something that looks "wrong" but you don't really have a better alternative, then you could also leave a comment along the lines of "This code/design doesn't sit well with me, but I don't have a clear alternative. Can we discuss this?" and then try to get something better together.
OTHER TIPS
Some good answers here, but I think one important point is missing. It makes a big difference whose code you are reviewing, how experienced that person is and how he or she handles such suggestions. If you know your teammate well and you expect a note like "this code is flawed because..., do it differently" to be sufficient for him or her to come up with a better solution, then such a comment can be fine. But there are definitely persons where such a comment is not sufficient, and who need to be told precisely how to improve their code. So IMHO this is a judgement call you can only make for the individual case.
So generally, as a reviewer, is it fine to say "this code is flawed, do it differently" or do you have to come up with a specific solution?
Neither of the two is ideal IMO. The best thing to do is talk to the author and fix the problem collaboratively.
Code reviews don't have to be asynchronous. A lot of issues will unlock if you stop seeing them as a bureaucratic process and take a little time for live communication.
In code reviews, should the reviewer always present a solution for issues?
No. If you're doing that you're not a reviewer, you're the next coder.
In code reviews, should the reviewer never present a solution for issues?
No. Your job is to communicate the issue at hand. If presenting a solution makes the problem clear then do it. Just don't expect me to follow your solution. Only thing you get to do here is make a point. You don't get to dictate implementation.
When should a reviewer present a solution for issues?
When that's the most effective way to communicate. We're code monkeys not English majors. Sometimes the best way to show that code sucks... is less than optimal... is to show them code that sucks less... is more opt... oh hell you know what I mean.
The main issue is if people knew how to write the code better, they usually would have done so in the first place. Whether a criticism is specific enough depends a lot on the experience of the author. Very experienced programmers might be able to take a criticism like "this class is too complicated" and go back to the drawing board and come up with something better, because they just had an off day due to a headache or were being sloppy because they were in a rush.
Usually, though, you have to at least identify the source of the complication. "This class is too complicated because it breaks the Law of Demeter all over the place." "This class mixes up presentation layer and persistence layer responsibilities." Learning to identify those reasons and succinctly explain them is part of becoming a better reviewer. You rarely have to go into a lot of detail about solutions.
There are two types of bad programmers: those that don't follow standard practices and those that "only" follow standard practices.
When I've had limited work contact/providing feedback to someone, I wouldn't say, "This is a bad design." but something like "Can you explain this class to me?" You may discover it is a good solution, the dev sincerely did the best he could or even a recognition that it's a bad solution, but it's good enough.
Depending on the response, you're going to have a better idea how to approach each situation and person. They may quickly recognize the problem and discover the fix on their own. They may ask for help or will just try to go and solve it on their own.
There are suggested practices in our business, but they almost all have exceptions. If you understand the project and how the team is approaching it, that can be the context for determining the purpose of code review and how to go about addressing concerns.
I realize this is more of an approach to the problem than an explicit solution. There's going to be too much variability to cover all situations.
When I review code I only provide a solution for the issues I identify if I can do so with little effort. I do try to be specific about what I think the problem is though, refering back to existing documentation where possible. Expecting a reviewer to provide a solution for every problem identified creates a perverse incentive - it will discourage the reviewer from pointing out problems.
My opinion is going stronger towards not providing code in majority of cases, for a number of reasons:
- If the explanation on its own is not enough, they can always ask for a sample of what you're thinking of.
- You're not wasting your time by trying to get familiar with some code you haven't touched in a long time, just to modify it slightly, while someone else has just spent their time doing just that.
- If they're familiar with the piece of code already and you're not, giving just the feedback may result in better code than you would write. Giving someone a ready solution will often cause them to just use it, without considering improving it further.
- Always providing a solution is bordering on condescending. You're working with someone, hopefully because they were good enough to be hired. If you managed to learn why something is a bad idea, why wouldn't they learn it by listening to feedback and doing it themselves?
- Always providing a solution is just weird. Imagine you're pair programming at their desk: they just finished a couple of lines you think are not great. Do you just tell them what you spotted and why, or do you grab their keyboard and show your version immediately? This is what always providing your solution can feel like to other people.
- You can always say what you'd do instead, without spending the time to actually write it. You did just that when describing the first problem in the question.
- Don't hand out food, teach how to fish ;)
Sure, there are some cases where you're thinking of some specific alternative, and it's worth attaching it. But that's really rare in my experience. (lots of reviews, big projects) The original author can always ask you for a sample if they need to.
Even then, because of the 3rd reason, when giving a sample it may be worth saying for example "using x.foo() would make this simpler" rather than a full solution - and let the author write it. This also saves your time.
I think the key to code reviews is to agree on the rules before the review.
If you have a clear set of rules then there should be no need to offer a solution. You are just checking that the rules have been followed.
The only time the question of an alternate would come up would be if the original dev cant think of a way to implement the feature that fits the rules. Say you have a performance requirement, but the feature takes longer than your threshold after several optimisation attempts.
However! if your rules are subjective "Names must be approved by me!" then, yes you have just appointed yourself namer in chief and should expect requests for lists of acceptable names.
Your inheritance (optional parameters) example is perhaps better, int that i've seen code review rules which forbid long methods and 'too many' function parameters. But normally these can be trivially resolved by splitting. It's the "this solution seemed to overcomplicate things" part where your objectiveness is intruding and perhaps requires justification or an alternate solution.
If a potential solution is quick and easy to type up I try to include it in my peer review comments. If not, I at least list my concern and why I find that particular item problematic. In the case of variable/function names where you can't think of something better, I usually, acknowledge that I don't have a better idea, and end the comment in the form of an open-ended question just in case someone can. That way if nobody comes up with a better option, the review isn't really being held up.
For the example you gave in your question, with the poorly-designed class. I would leave some comments that while it seems like it may be overkill, inheritance would probably be the best way to address the problem the code is trying to solve, and leave it at that. I would try to phrase in a manner that indicates it's not a show-stopper and is up to the developer's discretion whether or not to fix. I would also open with an acknowledgment that you're not particularly familiar with that part of the code, and invite more knowledgeable developers and/or reviewers to clarify if there's a reason it's done the way it is.
Go and talk to the person whose code you're reviewing. Tell them, in a friendly manner, that you've found it a bit difficult to understand, and then discuss with them how it could be made clearer.
Written communication leads to vast amounts of wasted time, as well as to resentment and misunderstandings.
Face-to-face, the bandwidth is much higher, and one has the emotional side-channel to prevent hostility.
If you actually talk to the guy, it's much quicker, and you might make a new friend and find that you both enjoy your job more.
