7 ms·

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide fee

by IncRnd 4y ago

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go. * While I don't have feedback yet on this function, it's too long for me to follow. It would be easier to read and for future maintenance if you refactor this part into a function. * Since we're in closedown we can only take certain types of changes. If you refactor this into a separate function in the library, this change can be accepted. * Or, I hate to be the process person, but the internal guidelines for this team call for all code to be structured the same way. Refactor this part into a separate function and I'll approve the PR. There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge." How would you speak when sitting next to the person face-to-face? What tone do you want your boss to use when providing a performance review during a 1on1?

broast 4y ago
I agree with this. Don't give demands, give reasoning and examples.
wayne-li2 4y ago
Great post! One quibble with your last point — your guide is probably most helpful to the devs that are too direct and blunt, and expect others to be that way to them.
- IncRnd 4y ago
  Haha! That's an excellent way to state the issue.
jszymborski 4y ago
Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people). These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.
- IncRnd 4y ago
  Thanks!
- glouwbug 4y ago
  It's your standard Socratic Method[1] that's devised to help you (the learner) arrive at solutions on your own terms. Just don't push it too deep - others may become a little too annoyed, and, well, read up on Socrates to learn what happens next [1] https://en.wikipedia.org/wiki/Socratic_method https://en.wikipedia.org/wiki/Socratic_method
sebastien_b 4y ago
Agree with adding additional reasoning, regardless of the actual tone. I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).
- dhzhzjsbevs 4y ago
  Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal. My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.
- redwolf22 4y ago
  I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.
mmazing 4y ago
.. and sometimes after going through that exercise you might find you don't have a good reason for what you're asking for. :)
- deleted 4y ago
  [deleted]
gonehome 4y ago
+1 the reasoning is the entire point and all of these examples are good. Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).
- convolvatron 4y ago
  my rule is to weigh whether or not its worth making a comment very carefully. nothing without a clear justification. never say 'I would have done it this other way' - unless its a good friend who you often discuss style issues with. comment on things that you feel would make a substantial difference on schedule, maintainability, or function - and provide a clear reasoning with which to argue against. wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.
somethoughts 4y ago
Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better. And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful. Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused. And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.
- drewcoo 4y ago
  This! Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this. If discussing actual code problems in code reviews rankles people, just escalate to your manager. If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.
- dhzhzjsbevs 4y ago
  I ignore style check nitpick comments. Fix your linter and autoformatter. You don't pay me to lint code.
BeetleB 4y ago
I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews. Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]: "If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess." The worst comment I see in code reviews (and sadly, all too often) is: "Why did you do it this way?" I have no idea why I'm being asked this. The answer is "Because it solved the problem." Even this is problematic: "Why did you do it this way instead of X?" Possible (unhelpful) answers: "Because I didn't think of X." (I still don't know if you want me to change the code and why). "Because it solved the problem." Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of." Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion. [1] https://news.ycombinator.com/item?id=31889518 https://news.ycombinator.com/item?id=31889518
- lloeki 4y ago
  > "Why did you do it this way?" While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like: I feel I might be missing some insight or context, could you walk me through your rationale? Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.
- adhesive_wombat 4y ago
  > Why did you do it this way instead of X? Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor. You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already. What wouldn't wind me up? "I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?" Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob. Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear. Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.
pfortuny 4y ago
Yep: the operative word being “because”, absent in all the OP’s sentences.
mmcnl 4y ago
I agree with the tone of your suggestions, but your suggestions still take quite some time to read. They can easily be shortened without losing any information. For example, no need to to apologize for being the process person (being overly apologetic decreases the value of apologies anyway). Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?
samuell 4y ago
These work and sound great, but stating them without a "please" can come off as a little brusque. Please add a "please" before the orders, and I'll accept the advice.
jsnelgro 4y ago
These suggestions hit the nail on the head. Additionally, the reviewer also becomes a better dev because they’re forced to really think about and articulate why they think the code smells. Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.
osigurdson 4y ago
The original post seemed to be concerned about tone, not content.
- SilasX 4y ago
  That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.
  - osigurdson 4y ago
    How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?
    - SilasX 4y ago
      Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)

by IncRnd 4y ago

broast 4y ago

I agree with this. Don't give demands, give reasoning and examples.

wayne-li2 4y ago

Great post! One quibble with your last point — your guide is probably most helpful to the devs that are too direct and blunt, and expect others to be that way to them.

IncRnd 4y ago
Haha! That's an excellent way to state the issue.

jszymborski 4y ago

Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people). These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.

IncRnd 4y ago
Thanks!
glouwbug 4y ago
It's your standard Socratic Method[1] that's devised to help you (the learner) arrive at solutions on your own terms. Just don't push it too deep - others may become a little too annoyed, and, well, read up on Socrates to learn what happens next [1] https://en.wikipedia.org/wiki/Socratic_method https://en.wikipedia.org/wiki/Socratic_method

sebastien_b 4y ago

Agree with adding additional reasoning, regardless of the actual tone. I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).

dhzhzjsbevs 4y ago
Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal. My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.
redwolf22 4y ago
I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.

mmazing 4y ago

.. and sometimes after going through that exercise you might find you don't have a good reason for what you're asking for. :)

deleted 4y ago
[deleted]

gonehome 4y ago

+1 the reasoning is the entire point and all of these examples are good. Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).

convolvatron 4y ago
my rule is to weigh whether or not its worth making a comment very carefully. nothing without a clear justification. never say 'I would have done it this other way' - unless its a good friend who you often discuss style issues with. comment on things that you feel would make a substantial difference on schedule, maintainability, or function - and provide a clear reasoning with which to argue against. wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.

somethoughts 4y ago

Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better. And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful. Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused. And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.

drewcoo 4y ago
This! Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this. If discussing actual code problems in code reviews rankles people, just escalate to your manager. If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.
dhzhzjsbevs 4y ago
I ignore style check nitpick comments. Fix your linter and autoformatter. You don't pay me to lint code.

BeetleB 4y ago

I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews. Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]: "If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess." The worst comment I see in code reviews (and sadly, all too often) is: "Why did you do it this way?" I have no idea why I'm being asked this. The answer is "Because it solved the problem." Even this is problematic: "Why did you do it this way instead of X?" Possible (unhelpful) answers: "Because I didn't think of X." (I still don't know if you want me to change the code and why). "Because it solved the problem." Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of." Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion. [1] https://news.ycombinator.com/item?id=31889518 https://news.ycombinator.com/item?id=31889518

lloeki 4y ago
> "Why did you do it this way?" While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like: I feel I might be missing some insight or context, could you walk me through your rationale? Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.
adhesive_wombat 4y ago
> Why did you do it this way instead of X? Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor. You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already. What wouldn't wind me up? "I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?" Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob. Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear. Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.

pfortuny 4y ago

Yep: the operative word being “because”, absent in all the OP’s sentences.

mmcnl 4y ago

I agree with the tone of your suggestions, but your suggestions still take quite some time to read. They can easily be shortened without losing any information. For example, no need to to apologize for being the process person (being overly apologetic decreases the value of apologies anyway). Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?

samuell 4y ago

These work and sound great, but stating them without a "please" can come off as a little brusque. Please add a "please" before the orders, and I'll accept the advice.

jsnelgro 4y ago

These suggestions hit the nail on the head. Additionally, the reviewer also becomes a better dev because they’re forced to really think about and articulate why they think the code smells. Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.

osigurdson 4y ago

The original post seemed to be concerned about tone, not content.

SilasX 4y ago
That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.
- osigurdson 4y ago
  How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?
  - SilasX 4y ago
    Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)