4 ms·

> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I f

> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why. The other thing that wasn't mentioned, that I think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they're going to "gate" code they don't agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you're dealing with Junior engineers. I'm generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions/suggestions and mention it in the +1 with (see comments). This builds trust on teams, and stops things getting personal, especially with people who aren't very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you're not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they're in control not you. In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they've degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don't last on healthy teams.

allsystemsgo 11y ago
I agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.
Cymen 11y ago
I agree 100% with the "don't be a blocker." I personally look for about 80-90% yes and if it meets that criteria with no errors, I thumb it up. If I can't thumb it up, I try to put a "thumbs up with commented items fixed". I try to not block -- comment for bad issues, sometimes sigh but agree when it's a 10% disagreement and go forward. Life is a series of iterations. That 10% will be addressed in a future iteration. And I never close another persons PR.
nazbot 11y ago
I'm going to differ on the 'don't be a blocker' thing. It varies by team. If your whole team agrees that the code reviewer is the 'gate' then people understand that they just need to fix the thing and move on. I personally love that style of working because you tend not to write trivial suggestions and everyone knows that what people have written isn't meant as nit-picky and just has to be changed. If you don't think it's the right change then the other person can just ask why it's important and hopefully get educated. I also tend to just trust my colleagues enough that what they suggest is a good change. I often find I may miss small things through like lack of documentation or whatever (get tired after a while) and those reminders in the CR are the equivalent of a spotter getting you to do one last rep at the gym.
nimnio 11y ago
From a certain perspective, you just paraphrased the quote you disagreed with: Him: "If the reviewer makes a suggestion, and I don't have a clear answer as to why the suggestion should not be implemented, I'll usually make the change." You: "[Do] not act as a blocker for code reviews unless it's absolutely necessary." I think these express essentially the same philosophy, with some slight differences in the particulars.

allsystemsgo 11y ago

I agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.

Cymen 11y ago

I agree 100% with the "don't be a blocker." I personally look for about 80-90% yes and if it meets that criteria with no errors, I thumb it up. If I can't thumb it up, I try to put a "thumbs up with commented items fixed". I try to not block -- comment for bad issues, sometimes sigh but agree when it's a 10% disagreement and go forward. Life is a series of iterations. That 10% will be addressed in a future iteration. And I never close another persons PR.

nazbot 11y ago

I'm going to differ on the 'don't be a blocker' thing. It varies by team. If your whole team agrees that the code reviewer is the 'gate' then people understand that they just need to fix the thing and move on. I personally love that style of working because you tend not to write trivial suggestions and everyone knows that what people have written isn't meant as nit-picky and just has to be changed. If you don't think it's the right change then the other person can just ask why it's important and hopefully get educated. I also tend to just trust my colleagues enough that what they suggest is a good change. I often find I may miss small things through like lack of documentation or whatever (get tired after a while) and those reminders in the CR are the equivalent of a spotter getting you to do one last rep at the gym.

nimnio 11y ago

From a certain perspective, you just paraphrased the quote you disagreed with: Him: "If the reviewer makes a suggestion, and I don't have a clear answer as to why the suggestion should not be implemented, I'll usually make the change." You: "[Do] not act as a blocker for code reviews unless it's absolutely necessary." I think these express essentially the same philosophy, with some slight differences in the particulars.