93

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?

closed as primarily opinion-based by gnat, user22815, maple_shaft May 25 '17 at 17:57

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
  • 24
    @gnat: No the code is not too complicated. And it is just an example. I am generally asking if the reviewer is responsible for presenting a solution. – Frank Puffer May 23 '17 at 11:54
  • 5
    no, I'd say that as a reviewer you're not obliged to tell how to improve it. If you can describe what feels wrong in there, do it; if not - just provide general comment. (One of most useful review comments I recall receiving was literally like "this class is all total garbage") – gnat May 23 '17 at 12:48
  • 5
    "The standard way to resolve this would be to split the class up and use inheritance. ". I sure hope you're not reviewing my code! – gardenhead May 23 '17 at 14:04
  • 7
    Pinpointing potential problems could be enough. The reviewer looks at the code as a user, maintainer or designer. Providing a different angle view or spotting issues the coder may not have noticed yet can help the coder to improve on his work. And if you spot something you like, it would not hurt to report that too. It should not be a corrective exercise but rather an enlightening one. That is why it is called "peer review". – Martin Maat May 23 '17 at 15:26

11 Answers 11

139

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.

  • 23
    +1 for discussion to come to a solution together - this is the way I learn the most from the senior programmers reviewing my code – dj18 May 23 '17 at 15:04
  • 19
    +1. When giving feedback, it's best to give constructive criticism whenever possible. – FrustratedWithFormsDesigner May 23 '17 at 17:00
  • 7
    I agree with the last bit especially. It's perfectly okay to say, "this solution feels wrong because...," or "I'm worried that this part might be problematic because..." without giving a solution. – Daniel T. May 23 '17 at 20:45
  • 1
    @dotancohen: The "can we discuss this" was intended to be a question. Personally, I would have the discussion anyway, even if it is only to learn something myself. – Bart van Ingen Schenau May 24 '17 at 12:24
  • 1
    The subtle danger is, with enough discussion and implementation communication this stops being a review and becomes pair programming. Pair programming is good but once it's done you need a 3rd person to review because independence has been lost. – candied_orange May 24 '17 at 15:55
35

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.

29

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.

  • "Bureaucratic process" is such a good way of putting it! – frnhr May 25 '17 at 18:27
17

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.

  • 8
    Don't code in a vacuum, because it sucks. – user251748 May 23 '17 at 16:27
  • Hm. When I suggest a solution to a problem, it often has benefits that I'm aware of but would simply take too long to give an exhaustive list of them all. (These are often related to stability and having confidence that the thing will keep working while we change other stuff around it.) So if you do something else that doesn't solve as many problems, I wouldn't be exactly happy (at least not unless you could tell me a good reason why what I suggested didn't work out). How would you handle that? – jpmc26 May 24 '17 at 22:34
  • 1
    P.S.: "Code monkey" is often used to describe an unskilled, unthinking programmer who simply does what they're told even if it's a bad idea and doesn't have good design sensibilities. See Urban dictionary. Even Wikipedia notes that it's sometimes derogatory. – jpmc26 May 24 '17 at 22:37
  • @jpmc26 if you're going to use code to communicate with me then I hope you'd use code that shows how the problem might be solved better. Also, Code Monkey can be used with affection. Certainly more affection than English majors get – candied_orange May 25 '17 at 0:20
  • "Code monkey get up get coffee. Code monkey go to job. Code monkey have boring meeting, with boring manager Rob. Rob say code monkey very diligent, but his output stink..." – Baldrickk May 25 '17 at 8:34
13

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.

  • 4
    +100 my most common frustration with Code Reviews is that if I had known a better way, I probably would have written that way. – RubberDuck May 23 '17 at 16:24
  • I love your first sentence. It made me think of asking myself: "is this code good enough?" then flipping a coin to decide whether to bother improving it or not! (Normally I just keep at it until I run out of time, but maybe I could stop when it is good enough instead?) – user251748 May 23 '17 at 16:26
  • IMO "This code is complicated because it breaks the Law of Demeter" is a bad comment. "This code is complicated because X is too coupled to Y and Z" is better. – immibis May 25 '17 at 1:46
  • "If people knew how to write the code better, they usually would have done so in the first place". There are exceptions. I developed this code that kind of works but will bite us in the arse some time in the future. Non-technical manager doesn't understand "I don't like this code and want three days to improve it". Non-technical manager understands "Joe reviewed and rejected this code and I need three days to improve it". – gnasher729 May 25 '17 at 19:31
4

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.

  • 1
    But, if it requires explanation to be recognizably good design, there are inline comments missing. – Wildcard May 23 '17 at 16:10
  • 1
    Sometimes rules have no exceptions, but usually they do not. – user251748 May 23 '17 at 16:20
  • @Wildcard - that depends on the ability and preferences/opinions of the people looking at it. – JeffO May 23 '17 at 18:29
  • 1
    @Wildcard I take the approach that the feedback should be phrased as a question, but the answer will (eventually) take the form of a comment, or perhaps a code change (e.g. better naming). That leaves the door open for the developer to explain their thinking, and discuss the options, rather than feeling like a demand, or accidentally doing their work over for them. – IMSoP May 24 '17 at 16:52
3

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.

3

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.

  • Your 5th point made me smile, I was imagining "dueling keyboards" to see who could get a great solution first. Who knew that Pair Programming could be like those two racecar arcade games, or a full-contact sport? "Steve just brutally checked Ron in to the BSOD, 2 minutes in the penalty box..." – user251748 May 25 '17 at 13:33
2

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.

  • 2
    "I think the key to code reviews is to agree on the rules before the review." This would be the ideal case. In practice we cannot assume that every developer knows all of the rules. Code reviews are useful to spread this knowledge and explain the rules with practical examples. Maybe that's one of the greatest benefits of doing code reviews.. – Frank Puffer May 24 '17 at 6:47
  • write the rules down in the coding standards document and give to new devs – Ewan May 24 '17 at 6:57
  • 1
    We have written down coding standards and these are given to new developers. This works most of the time but there are sometimes misinterpretations. In addition to the written down coding standards there are general principles like DRY or SOLID that are also addressed in code reviews. We expect a basic knowledge about these from our developers and also do some internal trainigs to improve it. This is an ongoing process and code reviews are part of it. – Frank Puffer May 24 '17 at 11:45
0

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.

0

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.

  • this doesn't seem to offer anything substantial over points made and explained in prior 11 answers – gnat May 25 '17 at 16:13

Not the answer you're looking for? Browse other questions tagged or ask your own question.