8 ms·

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 cou

by 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.
- im3w1l 7y ago
  Code review comments at google attach to a region rather than a specific line.
  - raxxorrax 7y ago
    Really? isn't that impractical? I think you should be mindful of the context, but I would believe this to be a hindrance if I want to discuss specifics. Or is this a mechanism precisely to not get lost in details?
    - adrianmonk 7y ago
      I think they mean if your comment refers to lines 310 to 313, you can explicitly specify that range instead of having to arbitrarily select 310 and let the reader infer that you mean the following lines too. (On a side note, I'd observe you may not be able to rely on reviewers consistently and accurately specifying the exact range they mean. It would be like assuming that nobody makes spelling errors in emails.)
BeetleB 7y ago
> You wrote some code, they don't understand it. So the ball's in your court "The ball's in your court" is the part that is not in agreement. If it's that simple, then by saying "OK" the ball is back in your court. I spent a lot of time in university. The one thing teachers don't want to hear is "I don't understand." Quite a few professors would give out information on how to handle it. Do not say "I don't understand," Start talking about what you do understand and what the gaps are. > 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. As the author, I do not want to start playing a guessing game. And your list is way too short. I could sit for an hour and think of many more scenarios. I'm not going to explore all of them in the hope that I hit the main problem you are having. If it is too complex, say so with some explanation. If you want a comment describing this at a higher level, say so. Why should I ask for help? I have code that solves the problem. I do care about readability, of course, but it's not clear whether my code is the problem or your skills are the problem. Your comment very much reads like what your parent is complaining about: "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." You have an obligation as a reviewer. Your obligation is not limited to asking questions or making statements. I know a lot of code reviews aren't structured as such, but the best code reviews are the ones where the reviewer and author have equal power. The worst ones are where the reviewer has the power to prevent a commit/merge because he/she wasn't happy with all the answers that were given. In my experience, the former are the ones where reviewers quickly learn not to give statements like "I don't understand." > 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. And this is precisely the problem. I think I picked the best solution. If you don't like it, as a minimum suggest an alternative. Now in reality, if I got such a statement, I probably would respond with "What aspect of the code is causing you trouble?" It's not a defensive response, nor is it an attack on you. And this is fine. But I could have skipped it altogether if you had led with this.

by nfoz 7y ago

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.

im3w1l 7y ago
Code review comments at google attach to a region rather than a specific line.
- raxxorrax 7y ago
  Really? isn't that impractical? I think you should be mindful of the context, but I would believe this to be a hindrance if I want to discuss specifics. Or is this a mechanism precisely to not get lost in details?
  - adrianmonk 7y ago
    I think they mean if your comment refers to lines 310 to 313, you can explicitly specify that range instead of having to arbitrarily select 310 and let the reader infer that you mean the following lines too. (On a side note, I'd observe you may not be able to rely on reviewers consistently and accurately specifying the exact range they mean. It would be like assuming that nobody makes spelling errors in emails.)

BeetleB 7y ago

> You wrote some code, they don't understand it. So the ball's in your court "The ball's in your court" is the part that is not in agreement. If it's that simple, then by saying "OK" the ball is back in your court. I spent a lot of time in university. The one thing teachers don't want to hear is "I don't understand." Quite a few professors would give out information on how to handle it. Do not say "I don't understand," Start talking about what you do understand and what the gaps are. > 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. As the author, I do not want to start playing a guessing game. And your list is way too short. I could sit for an hour and think of many more scenarios. I'm not going to explore all of them in the hope that I hit the main problem you are having. If it is too complex, say so with some explanation. If you want a comment describing this at a higher level, say so. Why should I ask for help? I have code that solves the problem. I do care about readability, of course, but it's not clear whether my code is the problem or your skills are the problem. Your comment very much reads like what your parent is complaining about: "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." You have an obligation as a reviewer. Your obligation is not limited to asking questions or making statements. I know a lot of code reviews aren't structured as such, but the best code reviews are the ones where the reviewer and author have equal power. The worst ones are where the reviewer has the power to prevent a commit/merge because he/she wasn't happy with all the answers that were given. In my experience, the former are the ones where reviewers quickly learn not to give statements like "I don't understand." > 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. And this is precisely the problem. I think I picked the best solution. If you don't like it, as a minimum suggest an alternative. Now in reality, if I got such a statement, I probably would respond with "What aspect of the code is causing you trouble?" It's not a defensive response, nor is it an attack on you. And this is fine. But I could have skipped it altogether if you had led with this.