4 ms·

I disagree with the title but found myself agreeing with many points in the article. “Don’t be condescending” seems like generally applicable advice. But IMO,

I disagree with the title but found myself agreeing with many points in the article. “Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!

tccole 4y ago
I think that goes to giving a why with a suggested change and giving a rough sketch.
rhizome 4y ago
The word "discussing" is doing a lot of work there and does nothing to distinguish it from condescension. Knowing something is one thing, being nice about it is quite another. Opposing them with a "but," like "hey that just goes with the territory," is not actually addressing the issue.
ketzo 4y ago
Yeah, I think the title is a little provocative to get you to read, but I ended up agreeing with it. Maybe "don't try to be a teacher during code reviews" is slightly more precise?
- nonethewiser 4y ago
  What's the difference? Presumably teachers teach.
rictic 4y ago
Yeah, and it doesn't take much to convey that this is a conversation between peers. A couple simple changes I get a lot of mileage out of. Where once I'd have written: "Do [X]" Now I write: "I see [problem Y], consider making [change X] to improve it" If the reviewee agrees then the change is easy and straightforward to make, but if they're unconvinced then the phrasing invites a dialog. Or if I think I see a bug, I'll phrase it like: "I think there's a bug here, how does this method behave if foo is null and bar is the empty string? I think we'd throw a null pointer error. Recommend adding a test for that case" Clear, actionable, refutable
- simplotek 4y ago
  > Clear, actionable, refutable That's wisdom, and a clear way to make everyone around you better. I'd add that being humble should also play a role. We have tastes and insights and preferences, and it's not productive to block PRs because of subjective, non-critical aspects. A working CICD pipeline lowers the cost of pushing a change, this we can always revisit things. It's far more important to have a team that trusts each other and feels confident to push changes fast than it is to have gatekeepers whose role ends up being one of needlessly putting breaks on a team for no justifiable reason.
- deleted 4y ago
  [deleted]
emeraldd 4y ago
Agreed, this isn't about teaching. It's about people who stroke their own egos when they should really be instructing. The example comment doesn't tell you anything useful except that the commenter might not understand what the author is trying to do. That has no call to action, no indication of a problem, and basically nothing useful to the author. It is a complete waste of time ...

tccole 4y ago

I think that goes to giving a why with a suggested change and giving a rough sketch.

rhizome 4y ago

The word "discussing" is doing a lot of work there and does nothing to distinguish it from condescension. Knowing something is one thing, being nice about it is quite another. Opposing them with a "but," like "hey that just goes with the territory," is not actually addressing the issue.

ketzo 4y ago

Yeah, I think the title is a little provocative to get you to read, but I ended up agreeing with it. Maybe "don't try to be a teacher during code reviews" is slightly more precise?

nonethewiser 4y ago
What's the difference? Presumably teachers teach.

rictic 4y ago

Yeah, and it doesn't take much to convey that this is a conversation between peers. A couple simple changes I get a lot of mileage out of. Where once I'd have written: "Do [X]" Now I write: "I see [problem Y], consider making [change X] to improve it" If the reviewee agrees then the change is easy and straightforward to make, but if they're unconvinced then the phrasing invites a dialog. Or if I think I see a bug, I'll phrase it like: "I think there's a bug here, how does this method behave if foo is null and bar is the empty string? I think we'd throw a null pointer error. Recommend adding a test for that case" Clear, actionable, refutable

simplotek 4y ago
> Clear, actionable, refutable That's wisdom, and a clear way to make everyone around you better. I'd add that being humble should also play a role. We have tastes and insights and preferences, and it's not productive to block PRs because of subjective, non-critical aspects. A working CICD pipeline lowers the cost of pushing a change, this we can always revisit things. It's far more important to have a team that trusts each other and feels confident to push changes fast than it is to have gatekeepers whose role ends up being one of needlessly putting breaks on a team for no justifiable reason.
deleted 4y ago
[deleted]

emeraldd 4y ago

Agreed, this isn't about teaching. It's about people who stroke their own egos when they should really be instructing. The example comment doesn't tell you anything useful except that the commenter might not understand what the author is trying to do. That has no call to action, no indication of a problem, and basically nothing useful to the author. It is a complete waste of time ...