GitHub Community
How do you give constructive feedback without being a d*ck? #189230
-
Select Topic AreaQuestion BodyCode review culture seems to vary wildly. Some teams are brutal, some are overly gentle to the point of being useless.
|
Beta Was this translation helpful? Give feedback.
Replies: 1 comment 1 reply
-
|
A simple rule that works well in most teams is review the code, not the person. A pattern I often use is: Observation → Suggestion → Reason Example: I noticed this method is doing both validation and database logic. It might be worth separating those concerns into different functions because it would make testing and maintenance easier. When the code is genuinely problematic, it's still better to focus on the impact, not the developer: This implementation works, but it could become hard to maintain as the logic grows. Splitting it into smaller functions might improve readability and make future changes safer. To separate “wrong” vs “personal preference”, many teams follow this rule: Wrong: bugs, security issues, performance problems, broken patterns → should be fixed. Preference: naming style, structure, formatting → suggest, don’t enforce unless the team style guide requires it. Good reviews usually aim to: improve readability prevent bugs share knowledge keep consistency with the codebase If a review doesn’t help the next person understand or improve the code, it’s probably not a very useful review. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you so much for this! The Observation → Suggestion → Reason pattern is something I'll start using right away, it makes it so much easier to give feedback that doesn't feel personal. The distinction between "wrong" and "preference" is also really clarifying. I think a lot of bad review culture comes from treating preferences as absolutes. Really appreciate you taking the time to write this out so clearly! |
Beta Was this translation helpful? Give feedback.
A simple rule that works well in most teams is review the code, not the person.
A pattern I often use is:
Observation → Suggestion → Reason
Example:
I noticed this method is doing both validation and database logic. It might be worth separating those concerns into different functions because it would make testing and maintenance easier.
When the code is genuinely problematic, it's still better to focus on the impact, not the developer:
This implementation works, but it could become hard to maintain as the logic grows. Splitting it into smaller functions might improve readability and make future changes safer.
To separate “wrong” vs “personal preference”, many teams follow this rule:
Wrong…