5 ms·

"I don’t understand this." is a valid review comment. Of course sometimes you can expand on the comment, but sometimes it's enough.

jchw 7y ago
IMO, it’s a waste of time. How does the person who wrote the code know what exact thing you don’t understand? You also aren’t asking a question so it’s not really easy to answer. This stuff tends to take multiple review cycles to resolve where a more specific question might actually clarify things in one cycle (It’s never good to assume your lack of understanding means it doesn’t make sense, of course.)
- VBprogrammer 7y ago
  My general approach to something like that is to ask (in person usually) to sit down with the author for a couple of minutes to discuss it. Getting into a back and forth conversion in review comments is in my experience too slow and cumbersome.
- nicoburns 7y ago
  Depends on your review process - if that comment acts as a trigger/marker for a face-to-face conversation then it makes sense.
  - jchw 7y ago
    Reviews at Google very often span timezones. Understandably, this sort of approach isn’t always ideal for optimal usage of time.
  - bradford 7y ago
    If you want to trigger a conversation you could just as easily say: "lets discuss this" And avoid putting the burden of ambiguity on the author of the original code under review.
- coryrc 7y ago
  For me (and I usually say this instead) it means "This should be explained/documented better for future readability"
- ken 7y ago
  > How does the person who wrote the code know what exact thing you don’t understand? The suggested alternative, "If this is an optimization, can you please add comments?", is a guess. Are we supposed to guess what uncommented code does, and put those guesses in the code review? That sounds even more unproductive than just saying "I don't understand".
- nawitus 7y ago
  It's usually not a waste of time, and the person can expand what the commented line does. A more specific question can be preferred sometimes, like my comment stated.
- bena 7y ago
  Yes, it's the equivalent of "It doesn't work." That's not a problem, that's a complaint. If you don't understand something, explain what you think it should do and where you're understanding is failing.
hotcarl 7y ago
“I don’t understand this” reflects a valid feeling but is not a constructive comment in a code review. What about using “Would you explain this to me?” A good code review is not a series of walls or hoops for the writer to jump through. It’s a critical discussion conducted by a team. Code reviewers should be collaborators not obstacles.
- ken 7y ago
  How is "Would you explain this to me?" not a "hoop for the writer to jump through", then, if "I don't understand this" is? They sound exactly the same to me, except one is longer, and slightly more explicit. When I shake someone's hand and tell them my name, I expect them to understand from context that I'm expecting them to tell me their name. Nobody considers this a particularly impolite way to ask for your name. I certainly don't have to spell it out with "Would you please tell me your name?"
- nfoz 7y ago
  I disagree, I find it perfectly constructive. There's no animosity in the statement. You wrote some code, they don't understand it. So the ball's in your court, with a few possibilities. Maybe your code is too complex, or maybe it needs a comment explaining what's going on, or maybe the particular reviewer just needs some missing background info. Or maybe you don't know what's best and can ask them for help/advice. YOU get to decide that. The reviewer didn't spend too long trying to guess which solution is best. They just gave you what you need to know to find the best solution. This is a plain and respectful conversation of peers. If you're a priori feeling intimidated by your coworker then you could probably interpret a lot of innocent stuff in a negative way.... but in that light, then to me "Could you explain this?" sounds just as negative as "I don't understand this". Just worse because now I have to explain it instead of improving the complex code.
  - adrianmonk 7y ago
    The problem I would have is that it is too vague. What do you not understand about it? Usually there's more than one point you could be confused on. For example, let's say I'm using a hash to eliminate duplicate strings when building a message to send over the network. Do you not understand how duplicates could occur in the first place? Do you not understand why I'm using a hash instead of some other implementation choice? Or do you not understand why duplicates cannot be put into the message I'm building? In other words, when you say "I don't understand this", is it clear and unambiguous what "this" refers to? Sometimes the line number you choose to put your comment on makes it pretty clear. But not always. You might have a problem with that line, or you might have a problem with a chunk of code that begins at that line.
hoorayimhelping 7y ago
If that's a valid review comment, "ok." is a valid response to it. Low effort comments get low effort responses. If you want to have a discussion, maybe point out what you don't understand and what you expected. "I don't quite get this - I would expect this to use the existing pattern used in [file 1] and [file 2]. Would you mind walking me through your reasoning here?" can actually start a discussion, and isn't so adversarial.
bena 7y ago
No, it doesn't impart any information. Why don't you understand it? Is it because the code is hard to understand, because the reader doesn't understand the domain, because the code is wrong, because the reader has his monitor turned off? "This is supposed to florp the flozzle, but after it flammles, it appears to flizzle the florp and skip over the flozzle. Am I missing something here?"
deeteecee 7y ago
Eh, this is 50/50 in my book. I lean towards adding more context than just this but if you write it in my PR, I have no problems with it unless you repeatedly did not absorb my code and wrote this kind of comment.

jchw 7y ago

IMO, it’s a waste of time. How does the person who wrote the code know what exact thing you don’t understand? You also aren’t asking a question so it’s not really easy to answer. This stuff tends to take multiple review cycles to resolve where a more specific question might actually clarify things in one cycle (It’s never good to assume your lack of understanding means it doesn’t make sense, of course.)

VBprogrammer 7y ago
My general approach to something like that is to ask (in person usually) to sit down with the author for a couple of minutes to discuss it. Getting into a back and forth conversion in review comments is in my experience too slow and cumbersome.
nicoburns 7y ago
Depends on your review process - if that comment acts as a trigger/marker for a face-to-face conversation then it makes sense.
- jchw 7y ago
  Reviews at Google very often span timezones. Understandably, this sort of approach isn’t always ideal for optimal usage of time.
- bradford 7y ago
  If you want to trigger a conversation you could just as easily say: "lets discuss this" And avoid putting the burden of ambiguity on the author of the original code under review.
coryrc 7y ago
For me (and I usually say this instead) it means "This should be explained/documented better for future readability"
ken 7y ago
> How does the person who wrote the code know what exact thing you don’t understand? The suggested alternative, "If this is an optimization, can you please add comments?", is a guess. Are we supposed to guess what uncommented code does, and put those guesses in the code review? That sounds even more unproductive than just saying "I don't understand".
nawitus 7y ago
It's usually not a waste of time, and the person can expand what the commented line does. A more specific question can be preferred sometimes, like my comment stated.
bena 7y ago
Yes, it's the equivalent of "It doesn't work." That's not a problem, that's a complaint. If you don't understand something, explain what you think it should do and where you're understanding is failing.

hotcarl 7y ago

“I don’t understand this” reflects a valid feeling but is not a constructive comment in a code review. What about using “Would you explain this to me?” A good code review is not a series of walls or hoops for the writer to jump through. It’s a critical discussion conducted by a team. Code reviewers should be collaborators not obstacles.

ken 7y ago
How is "Would you explain this to me?" not a "hoop for the writer to jump through", then, if "I don't understand this" is? They sound exactly the same to me, except one is longer, and slightly more explicit. When I shake someone's hand and tell them my name, I expect them to understand from context that I'm expecting them to tell me their name. Nobody considers this a particularly impolite way to ask for your name. I certainly don't have to spell it out with "Would you please tell me your name?"
nfoz 7y ago
I disagree, I find it perfectly constructive. There's no animosity in the statement. You wrote some code, they don't understand it. So the ball's in your court, with a few possibilities. Maybe your code is too complex, or maybe it needs a comment explaining what's going on, or maybe the particular reviewer just needs some missing background info. Or maybe you don't know what's best and can ask them for help/advice. YOU get to decide that. The reviewer didn't spend too long trying to guess which solution is best. They just gave you what you need to know to find the best solution. This is a plain and respectful conversation of peers. If you're a priori feeling intimidated by your coworker then you could probably interpret a lot of innocent stuff in a negative way.... but in that light, then to me "Could you explain this?" sounds just as negative as "I don't understand this". Just worse because now I have to explain it instead of improving the complex code.
- adrianmonk 7y ago
  The problem I would have is that it is too vague. What do you not understand about it? Usually there's more than one point you could be confused on. For example, let's say I'm using a hash to eliminate duplicate strings when building a message to send over the network. Do you not understand how duplicates could occur in the first place? Do you not understand why I'm using a hash instead of some other implementation choice? Or do you not understand why duplicates cannot be put into the message I'm building? In other words, when you say "I don't understand this", is it clear and unambiguous what "this" refers to? Sometimes the line number you choose to put your comment on makes it pretty clear. But not always. You might have a problem with that line, or you might have a problem with a chunk of code that begins at that line.

hoorayimhelping 7y ago

If that's a valid review comment, "ok." is a valid response to it. Low effort comments get low effort responses. If you want to have a discussion, maybe point out what you don't understand and what you expected. "I don't quite get this - I would expect this to use the existing pattern used in [file 1] and [file 2]. Would you mind walking me through your reasoning here?" can actually start a discussion, and isn't so adversarial.

bena 7y ago

No, it doesn't impart any information. Why don't you understand it? Is it because the code is hard to understand, because the reader doesn't understand the domain, because the code is wrong, because the reader has his monitor turned off? "This is supposed to florp the flozzle, but after it flammles, it appears to flizzle the florp and skip over the flozzle. Am I missing something here?"

deeteecee 7y ago

Eh, this is 50/50 in my book. I lean towards adding more context than just this but if you write it in my PR, I have no problems with it unless you repeatedly did not absorb my code and wrote this kind of comment.