Code Health: Respectful Reviews == Useful Reviews(testing.googleblog.com)
testing.googleblog.com
Code Health: Respectful Reviews == Useful Reviews
https://testing.googleblog.com/2019/11/code-health-respectful-reviews-useful.html
267 comments
This is really interesting, I think this is how the general population views social justice and other activist issues actually. It seems to be a similarly structured setup where we implement a byzantine list of things that people of privilege are supposed to keep track up, starting out as a list of good intentions that then spirals out of control. Afterwards nobody is real with anyone anymore and we are simply left with a system where most social interaction feels like a robotic scripted attempt to follow the rules of privilege.
The problem is they come up with all these rules for others to follow. They don't seem to realize the rules also apply to them. It's perfectly fine to call for the exclusion of a man because of a controversial tweet but when protected groups do the same thing and people try to hold them accountable they get laughed at as if it was absurd and wrong to even try.
I think you’ve fallen prey to propaganda that paints proponents of social justice in a certain way.
The social justice warriors I know _are_ part of the general population, and mind their business, and work hard every day to help people in need.
I am always disappointed when I see people write off the entire cause of social justice as pointless meddling. You’ve been duped if you think that’s all that’s going on.
The “general population” you aligned yourself with is chock full of SJWs who you have had only positive interactions with. But you would never know it...
Like trans people who pass... how would you ever realize they exist? And yet there they are.
The social justice warriors I know _are_ part of the general population, and mind their business, and work hard every day to help people in need.
I am always disappointed when I see people write off the entire cause of social justice as pointless meddling. You’ve been duped if you think that’s all that’s going on.
The “general population” you aligned yourself with is chock full of SJWs who you have had only positive interactions with. But you would never know it...
Like trans people who pass... how would you ever realize they exist? And yet there they are.
While I’m in agreement with a lot of their causes, I have to say all SJW types I’ve met have been an very particular slice of the population, almost all educated, urban dwelling millennial age people. As a contract worker I see lots of job environments and I kind of enjoy the less upscale more down to earth (ironically more racially diverse) jobs where you don’t have to remember whose pronouns are “they” this week. I can’t even remember what pronouns are supposed to be modified so I messed up last time saying mine were “he/his”.
After saying your preferred pronouns are “he/him/his”, would you feel offended if people intentionally ignored that and instead referred to you as “her”?
Dismissing an entire class of people with pithy comments about “they/their” pronouns might seem fine to you, but it sure as hell doesn’t feel good when you’re on the other side of it. Be respectful to all people and not just the ones that don’t inconvenience you.
Dismissing an entire class of people with pithy comments about “they/their” pronouns might seem fine to you, but it sure as hell doesn’t feel good when you’re on the other side of it. Be respectful to all people and not just the ones that don’t inconvenience you.
> Be respectful to all people and not just the ones that don’t inconvenience you.
This is a good ideal to strive for. However, people should not be getting fired, banned, excluded, ostracized or otherwise sanctioned over this. For example:
https://github.com/joyent/libuv/pull/1015
https://www.joyent.com/blog/the-power-of-a-pronoun
What happened in that pull request is completely disproportionate. These reactions and punishments will cause chilling effects. People are watching.
This is a good ideal to strive for. However, people should not be getting fired, banned, excluded, ostracized or otherwise sanctioned over this. For example:
https://github.com/joyent/libuv/pull/1015
https://www.joyent.com/blog/the-power-of-a-pronoun
What happened in that pull request is completely disproportionate. These reactions and punishments will cause chilling effects. People are watching.
If anything is disproportionate in your examples, it’s the reactions of those against merging the pull request. Using “he/him” in technical documentation is as tone-deaf as using “she/hers” when writing recipes. Men, women and non-binary engineers exist and we should not hold principles that prevent entirely innocuous changes like changing “him” to “their” in a readme, period. The only reason that GitHub PR thread and the Joyent blog post escalated as they did was the strong reaction by the individuals who were against the change. If the PR was just merged, there would’ve been no controversy.
There were valid reasons to reject the change:
> Us maintainers tend to reject tiny doc changes because they're often more trouble than they're worth.
> You have to collect and check the CLA, it makes git blame less effective, etc.
Even if you don't agree, nothing in there even comes close to justifying the abuse leveled against the developer. A small documentation change led to the departure of a core developer. Was it worth it?
> Us maintainers tend to reject tiny doc changes because they're often more trouble than they're worth.
> You have to collect and check the CLA, it makes git blame less effective, etc.
Even if you don't agree, nothing in there even comes close to justifying the abuse leveled against the developer. A small documentation change led to the departure of a core developer. Was it worth it?
It led to Ben's departure because he abjectly refused to listen to the user community, encouraged a tremendous amount of public debate over his personal decision to go against the community's wishes, then single-handedly reverted Isaac Childers' commit accepting the change with the following pithy comment[0]:
> Revert "doc: Removed use of gendered pronouns"
> @isaacs may have his commit bit but that does not mean he is at liberty
> to land patches at will. All patches have to be signed off by either
> me or Bert. Isaac, consider yourself chided.
Do you feel that this is acceptable behavior that should be encouraged? Ben was fully in the wrong here, and if this was instead about Tabs vs Spaces instead of something political in nature, we wouldn't be having this debate. Ben acted childishly, was called out on it, and then _decided to leave of his own volition_ because he felt he was unwanted, which was true given his behavior.
Was it worth it? I don't know, ask Ben.
[0]https://github.com/joyent/libuv/commit/804d40ee14dc0f82c482d...
> Revert "doc: Removed use of gendered pronouns"
> @isaacs may have his commit bit but that does not mean he is at liberty
> to land patches at will. All patches have to be signed off by either
> me or Bert. Isaac, consider yourself chided.
Do you feel that this is acceptable behavior that should be encouraged? Ben was fully in the wrong here, and if this was instead about Tabs vs Spaces instead of something political in nature, we wouldn't be having this debate. Ben acted childishly, was called out on it, and then _decided to leave of his own volition_ because he felt he was unwanted, which was true given his behavior.
Was it worth it? I don't know, ask Ben.
[0]https://github.com/joyent/libuv/commit/804d40ee14dc0f82c482d...
> encouraged a tremendous amount of public debate
I don't think "debate" is the right word for what happened in that thread.
> then single-handedly reverted Isaac Childers' commit
A perfectly good reason for reverting the commit was provided in the message itself: "All patches have to be signed off by either me or Bert." It looks like the patch didn't meet that criteria. He also cited other valid reasons for rejecting it: "have to collect and check the CLA, it makes git blame less effective, etc."
Is this change so overwhelmingly important and urgent that it overrides the aforementioned policy and concerns? I don't know.
Does the fact he rejected the patch excuse the way people reacted to his decision? No.
I don't think "debate" is the right word for what happened in that thread.
> then single-handedly reverted Isaac Childers' commit
A perfectly good reason for reverting the commit was provided in the message itself: "All patches have to be signed off by either me or Bert." It looks like the patch didn't meet that criteria. He also cited other valid reasons for rejecting it: "have to collect and check the CLA, it makes git blame less effective, etc."
Is this change so overwhelmingly important and urgent that it overrides the aforementioned policy and concerns? I don't know.
Does the fact he rejected the patch excuse the way people reacted to his decision? No.
I agree with respecting everyone and would never, for example, intentionally call a trans woman “he” because I can see by looking what to say. My issue is with people who are asking to have all of us remap language to remember a special exception for them (ie she looks female but feels nonbinary so wants to be called they) and gets offended if you don’t remember. It just feels like a highly politicized exercise, and though I try to comply it feels awkward.
I absolutely disagree, you are making a major incorrect assumption about why I have the opinions I do. I readily engaged with SJW activists and helped organize/participated in numerous events and protests. I know EXACTLY what they are like, and it isn't propaganda that causes me to think of them in this light, it was being around them and trying to work together with them in common cause that caused me to have this attitude.
Normal people can hold social justice opinions and go about their business normally like you say, but these people are not at all what I'm talking about in this discussion. And yes I would say that the general population is NOT chock full of the type of SJWs I hung out with in college. I would know.
Cite me a statistic if you want to convince me otherwise.
Normal people can hold social justice opinions and go about their business normally like you say, but these people are not at all what I'm talking about in this discussion. And yes I would say that the general population is NOT chock full of the type of SJWs I hung out with in college. I would know.
Cite me a statistic if you want to convince me otherwise.
I am not trying to discount your experience. I have met people like that too.
I’m just saying, that’s not the entirety, or even the majority of people who are fighting for social justice.
And there are powerful media campaigns working hard to get you to think that they are.
What you’re saying is a little like sating “I’ve been to every Taco place in Chicago, I am speaking from experience, tacos are trash.”
And I’m saying, there are a lot of other tacos out there.
I’m just saying, that’s not the entirety, or even the majority of people who are fighting for social justice.
And there are powerful media campaigns working hard to get you to think that they are.
What you’re saying is a little like sating “I’ve been to every Taco place in Chicago, I am speaking from experience, tacos are trash.”
And I’m saying, there are a lot of other tacos out there.
> What you’re saying is a little like sating “I’ve been to every Taco place in Chicago, I am speaking from experience, tacos are trash.”
It's more like saying "I've been to every Taco Warrior place in Chicago...".
While I think the term SJW is often used too broadly to include "reasonable people who care about social justice", I also think the term SJW quite often is specific enough to be used as a label for people who take the issue of social justice a little too seriously, often to the point of making it a (seemingly) core part of their identity.
Personally I'd say let's be reasonable and stop using the overloaded term 'SJW', but at the same time I do agree that there are certain people for which it would be nice to have a label, where SJW (emphasis on 'warrior') is kind of the right one.
It's more like saying "I've been to every Taco Warrior place in Chicago...".
While I think the term SJW is often used too broadly to include "reasonable people who care about social justice", I also think the term SJW quite often is specific enough to be used as a label for people who take the issue of social justice a little too seriously, often to the point of making it a (seemingly) core part of their identity.
Personally I'd say let's be reasonable and stop using the overloaded term 'SJW', but at the same time I do agree that there are certain people for which it would be nice to have a label, where SJW (emphasis on 'warrior') is kind of the right one.
I guess I would ask you: why do you want a special term for the shitty version of this specific category of people?
Do you have other terms for the shitty people within other specific groups? Do you have a term you use specifically for the shitty business people? Do you have a term you use specifically for shitty athletes? For shitty politicians? For shitty women?
We have a word for the shitty version of a gay person. We also have a word for the shitty version of a black person. Worth noting. Louis CK says he has nothing against gay people, but he wants to use the F-word to connote a special kind of shitty gender nonconforming man.
Have you thought about where this SJW label came from and why it exists in the first place?
Do you have other terms for the shitty people within other specific groups? Do you have a term you use specifically for the shitty business people? Do you have a term you use specifically for shitty athletes? For shitty politicians? For shitty women?
We have a word for the shitty version of a gay person. We also have a word for the shitty version of a black person. Worth noting. Louis CK says he has nothing against gay people, but he wants to use the F-word to connote a special kind of shitty gender nonconforming man.
Have you thought about where this SJW label came from and why it exists in the first place?
This should only apply for large companies. Only in large companies do you end up employing large numbers of idiots. And only then do you need the rules that a minority go crazy with.
In my experience, the cure is usually worse than the disease. Give someone a rulebook that they can use to justify the awful inclinations of their personality, and they will spend 99% of their energy doing this.
Small companies manage this better in my experience - outside of the guy who came from LARGE TECH COMPANY and spends all day writing lists and telling people how they do things at LARGE TECH COMPANY - because they will get rid of someone who is being disruptive rather than try to move them somewhere else. HR in large companies tends to be far softer.
Btw, my point is that all this applies to social activism too. We need rules because eventually someone will do something dumb. The rules will get taken too far. And that small minority ends up making almost everyone else unhappy.
Incidentally: there is a probably a parallel with this forum. Of any internet forum, this place is probably the most humourless and exacting when it comes to people quoting rules (often by number) to each other. The monkeys will take over the zoo 100% of the time in these cases.
I have actually even had interviews like this. I remember one particularly funny one was for a front-end role where I was witness to a lecture from a "senior dev" on the topic of: why X technology/pattern is "wrong"?
In my experience, the cure is usually worse than the disease. Give someone a rulebook that they can use to justify the awful inclinations of their personality, and they will spend 99% of their energy doing this.
Small companies manage this better in my experience - outside of the guy who came from LARGE TECH COMPANY and spends all day writing lists and telling people how they do things at LARGE TECH COMPANY - because they will get rid of someone who is being disruptive rather than try to move them somewhere else. HR in large companies tends to be far softer.
Btw, my point is that all this applies to social activism too. We need rules because eventually someone will do something dumb. The rules will get taken too far. And that small minority ends up making almost everyone else unhappy.
Incidentally: there is a probably a parallel with this forum. Of any internet forum, this place is probably the most humourless and exacting when it comes to people quoting rules (often by number) to each other. The monkeys will take over the zoo 100% of the time in these cases.
I have actually even had interviews like this. I remember one particularly funny one was for a front-end role where I was witness to a lecture from a "senior dev" on the topic of: why X technology/pattern is "wrong"?
> 2) The CoC became a tool for power plays. For a few bad apples, the CoC became a list of gotchas that they could use as a power play against others. At one point we had someone reading every Slack conversation and code review just to call out possible CoC violations (remember, this includes things like using emojis in code review) so they could call other people out on their violations. Substantiative conversations moved to private messages or in-person conversations. Not healthy. Partially solved by adding a CoC rule about minding your own business and explaining healthy ways to share the CoC with others.
I've seen this happen too. Not for code reviews, but for process rules in general. IMO the key issue is that rules can't be used as a substitute for good judgement, and bad judgement needs to be an explicitly fire-able performance metric.
Unfortunately, it's really hard to make sure "good judgement" is educated and constantly re-evaluated, and not just whatever baggage the person carries (misconceptions at best, outright ageism/sexism/racism at worst). From the perspective of leadership, this is a tough but solvable problem: read about people management and sociology diversely and avidly, establish actually anonymous lines of feedback for ground level employees to upper management, and set a precedent for firing jerks promptly. From the perspective of an employee facing bad management, good luck fixing that. It's probably easier to just find a new job.
I've seen this happen too. Not for code reviews, but for process rules in general. IMO the key issue is that rules can't be used as a substitute for good judgement, and bad judgement needs to be an explicitly fire-able performance metric.
Unfortunately, it's really hard to make sure "good judgement" is educated and constantly re-evaluated, and not just whatever baggage the person carries (misconceptions at best, outright ageism/sexism/racism at worst). From the perspective of leadership, this is a tough but solvable problem: read about people management and sociology diversely and avidly, establish actually anonymous lines of feedback for ground level employees to upper management, and set a precedent for firing jerks promptly. From the perspective of an employee facing bad management, good luck fixing that. It's probably easier to just find a new job.
But that sounds like an inconsistently enforced rule which means selectively applied, eventually done so on factors that would be considered unacceptable if written down.
If a rule is written down, I'm prefer it be fully enforced other than being used as a club to selectively bludgeon certain individuals. Yes, at first it will be used against those who 'deserve' it based on some social metric. But how long before it is used against unattractive people more than attractive people (see Halo effect) or used against people with less social skills more than those with greater charisma?
I've seen it happen one too many times to think the new rule won't end up with the same abuses.
If a rule is written down, I'm prefer it be fully enforced other than being used as a club to selectively bludgeon certain individuals. Yes, at first it will be used against those who 'deserve' it based on some social metric. But how long before it is used against unattractive people more than attractive people (see Halo effect) or used against people with less social skills more than those with greater charisma?
I've seen it happen one too many times to think the new rule won't end up with the same abuses.
> But that sounds like an inconsistently enforced rule which means selectively applied, eventually done so on factors that would be considered unacceptable if written down.
You're going to have to pick a position on the spectrum between always-enforced rules and purely honor codes (and social pressure). We know the former fails because hard and fast rules can be and are gamed. We know honor codes and social pressure only keep honest people honest. So in practice, where on this spectrum do you stand?
If it's your company, or team, or club, or whatever other gathering of humans, you have to make a choice on this question.
You're going to have to pick a position on the spectrum between always-enforced rules and purely honor codes (and social pressure). We know the former fails because hard and fast rules can be and are gamed. We know honor codes and social pressure only keep honest people honest. So in practice, where on this spectrum do you stand?
If it's your company, or team, or club, or whatever other gathering of humans, you have to make a choice on this question.
It's something I have spent a lot of time thinking about, and while I have yet to reach a conclusion, I believe in a function that approaches always enforced rules as penalty for breaking the rules increases. When the penalty is high, the harm the average player feels from someone gaming the rules is preferable to the harm they feel from having the rules selectively applied to them.
I also strongly encourage pointing out the 'real rules' that the written rules are enforced by when they are selectively enforced. Many times they are much harder to tolerate once called out.
I also strongly encourage pointing out the 'real rules' that the written rules are enforced by when they are selectively enforced. Many times they are much harder to tolerate once called out.
What you're talking about here is creating a robust mechanism for monitoring, investigating, and revising the rule system, which I think we both agree about. My point is that if you are a leader, there is a general approach to make sure your organization has that. Not so much as a peon.
To bring it back to your original point, you expressed a clear preference for a letter-of-the-law approach, and for rules that are always applied and always enforced. You place the burden of fairness on the creator of the rule instead of the enforcer. That's an understandable approach. I don't agree now, but I did at one time, so let's run with it.
How would you define guidelines/rules for code reviews specifically, or engineering culture in general? Would you? How would you remediate brilliant jerks? How would you measure and incentivize working well with the rest of the team/company?
To bring it back to your original point, you expressed a clear preference for a letter-of-the-law approach, and for rules that are always applied and always enforced. You place the burden of fairness on the creator of the rule instead of the enforcer. That's an understandable approach. I don't agree now, but I did at one time, so let's run with it.
How would you define guidelines/rules for code reviews specifically, or engineering culture in general? Would you? How would you remediate brilliant jerks? How would you measure and incentivize working well with the rest of the team/company?
Code cop. Naming standards would very consistent. If it is SSN, Ssn, or SocialSecurityNumber, it will be that way all across all databases of the same system. Rules would differ per system, because there are different best practices per system, which is what I would start with as there is far more expertise into those best practices than what I have. Rules would be equally enforced, but anyone would have the ability to request change.
Overall, where possible I rather not have a rule and only create them as needed. Once needed it is created and enforced consistently.
>How would you remediate brilliant jerks? How would you measure and incentivize working well with the rest of the team/company?
These are areas I'm not proficient in and thus I would first need more study.
One thing is that I don't really think there is a choice between two options, but that both options are the same. The only difference from social pressure is that the rule system is much more complex and not written down because people cannot tolerate facing the rule system once written. Imagine a law that says attractive people get less time in prison, would that stand? That women get less prison time? That whites get less prison time? Yet this is what happens when judges are free to given sentences and prosecutors are free to pick what charges to bring.
What I advocate is any rule must be written down, and thus any unfairness like the above must be faced and either codified and thus open for challenge or be done away with by removing the subjective application of a rule.
Overall, where possible I rather not have a rule and only create them as needed. Once needed it is created and enforced consistently.
>How would you remediate brilliant jerks? How would you measure and incentivize working well with the rest of the team/company?
These are areas I'm not proficient in and thus I would first need more study.
One thing is that I don't really think there is a choice between two options, but that both options are the same. The only difference from social pressure is that the rule system is much more complex and not written down because people cannot tolerate facing the rule system once written. Imagine a law that says attractive people get less time in prison, would that stand? That women get less prison time? That whites get less prison time? Yet this is what happens when judges are free to given sentences and prosecutors are free to pick what charges to bring.
What I advocate is any rule must be written down, and thus any unfairness like the above must be faced and either codified and thus open for challenge or be done away with by removing the subjective application of a rule.
The pattern I see is the person spearheading code of conduct is quarantined in their own office so the rest of the team doesn't have to put up with them. Not necessarily managers.
One of the most important things I've learned over the years I've managed people is that processes and policies of any sort are two-edged swords. It's easy to add a policy, and to convince oneself of the advantage the policy will confer. What is very often ignored is the cost of a policy.
Every policy comes with an associated cost, and those costs can add up quickly. People always ask "what will this policy make better", but, just as important, is to ask, "what will this policy make worse".
Every policy comes with an associated cost, and those costs can add up quickly. People always ask "what will this policy make better", but, just as important, is to ask, "what will this policy make worse".
I've come to believe that ignoring one side of the cost/benefit calculus is the root cause of an enormous number of mistakes.
To spell the two errors out:
A: "This has a benefit, so we should do it!"
B: "This has a cost, so we should not do it!"
To spell the two errors out:
A: "This has a benefit, so we should do it!"
B: "This has a cost, so we should not do it!"
I like to talk about trade-offs a lot, and opportunity cost is also a good way to look at it. But yeah, it’s not often I see that happen.
Even better, make that a function of time so your cost/benefit analysis looks further beyond your initial preference. Spending more to save more is a highly effective choice in the right circumstances, and equally so, a perceived rapid gain can cost a fortune a bit later on.
It’s never so easy as saying always do D, prefer Y, avoid Z. They’re prescriptive and reactionary.
Even better, make that a function of time so your cost/benefit analysis looks further beyond your initial preference. Spending more to save more is a highly effective choice in the right circumstances, and equally so, a perceived rapid gain can cost a fortune a bit later on.
It’s never so easy as saying always do D, prefer Y, avoid Z. They’re prescriptive and reactionary.
Conflict isn't necessarily bad and it's often very, very healthy as long as it doesn't descend into name calling or something toxic.
Agreed, but I consider this kind of a non-statement.
Conflict isn't bad. It's misunderstandings that are bad and lead to name calling or worse. In my experience, the majority of bad behavior at work is due to a misinterpretation of good/neutral intentions.
Conflict avoidance is pretty bad, in my opinion. However, it is a mistake to look at codes of conduct purely from an avoidance angle. This blog post is likely not about avoiding conflicts.
Conflict isn't bad. It's misunderstandings that are bad and lead to name calling or worse. In my experience, the majority of bad behavior at work is due to a misinterpretation of good/neutral intentions.
Conflict avoidance is pretty bad, in my opinion. However, it is a mistake to look at codes of conduct purely from an avoidance angle. This blog post is likely not about avoiding conflicts.
I think what makes HN so high-quality overall is the constant policing for personal attacks and toxicity. Some sites have 'rules' that are barely or not enforced at all that you must be respectful but this is the only site that consistently and objectively enforces good conduct.
One weakness is that users can gang on politically incorrect comments or threads and flag them.
Last I've seen this was a thread about the professor which was claiming that women aren't disadvantaged in physics. It was a totally harmless thing, about how a journal decided to publish his views.
Last I've seen this was a thread about the professor which was claiming that women aren't disadvantaged in physics. It was a totally harmless thing, about how a journal decided to publish his views.
ime, what is called “political correctness” is often interpreted as “respecting others” by different people.
You can disagree with what expectations they have for interpersonal respect, but I think that’s how the other side sees it.
You can disagree with what expectations they have for interpersonal respect, but I think that’s how the other side sees it.
Downvoting without offering counterargument is disrespectful and counter productive.
But this is enforced by the users themselves to 99%. This is a very fundamental difference.
I find this really interesting. I'm wondering if this is a cultural thing.
I didn't interpret the list as being a set of instructions at all. More of an expression of an opinion on what people should do. You read it, learn, incorporate it into your working life.
It happens to be that I pretty much agree with it all, but if I didn't, I wouldn't find myself consulting the list to decide whether I should or shouldn't say something.
If someone 'called me out' on a 'violation' of this document, I'm not sure how I'd respond. Probably with laughter? I honestly don't know, perhaps I'm just in a bubble.
We can discuss as adults, if there's a problem with my communication I want to know. I don't want to be pointed at the rule book, this isn't primary school.
I didn't interpret the list as being a set of instructions at all. More of an expression of an opinion on what people should do. You read it, learn, incorporate it into your working life.
It happens to be that I pretty much agree with it all, but if I didn't, I wouldn't find myself consulting the list to decide whether I should or shouldn't say something.
If someone 'called me out' on a 'violation' of this document, I'm not sure how I'd respond. Probably with laughter? I honestly don't know, perhaps I'm just in a bubble.
We can discuss as adults, if there's a problem with my communication I want to know. I don't want to be pointed at the rule book, this isn't primary school.
> If someone 'called me out' on a 'violation' of this document, I'm not sure how I'd respond. Probably with laughter? I honestly don't know, perhaps I'm just in a bubble.
Let's say, hypothetically, because this is neither you nor me, that I wrote something like "Were you paying attention in CS100?" to a dumb mistake in your pull request. We all slip sometimes, so generally these shouldn't be big deals. Quick fix and move on.
You said to me: "Hey, in your last code review you openly insulted my competence by saying '______'. Please don't do that."
I responded: "No, you deserved that for the type of mistake you made."
I know 1-2 developers personally who would genuinely believe that. I've encountered far more here and on Reddit that would as well.
I think the real versions of us would both agree that the hypothetical me is being a jerk. Having a guideline that says "Don't criticize the person" gives you a way to take this up your managerial chain with a specific problem, as opposed to a general statement of "munchbunny was being a jerk."
While you might not need this in order to manage the situation, I know of several fresh college grads who have not yet learned the communication and political skills to manage this, and so having a document spelling it out is helpful for both teaching and enforcement, even if the line items are not explicitly rules.
Let's say, hypothetically, because this is neither you nor me, that I wrote something like "Were you paying attention in CS100?" to a dumb mistake in your pull request. We all slip sometimes, so generally these shouldn't be big deals. Quick fix and move on.
You said to me: "Hey, in your last code review you openly insulted my competence by saying '______'. Please don't do that."
I responded: "No, you deserved that for the type of mistake you made."
I know 1-2 developers personally who would genuinely believe that. I've encountered far more here and on Reddit that would as well.
I think the real versions of us would both agree that the hypothetical me is being a jerk. Having a guideline that says "Don't criticize the person" gives you a way to take this up your managerial chain with a specific problem, as opposed to a general statement of "munchbunny was being a jerk."
While you might not need this in order to manage the situation, I know of several fresh college grads who have not yet learned the communication and political skills to manage this, and so having a document spelling it out is helpful for both teaching and enforcement, even if the line items are not explicitly rules.
So managers are supposed to be part time kindergarten nannies now? Is this really something that happens to an extent where educated adults cannot resolve such issues among themselves?
This is again a situation that's just hard for me to imagine, from both sides.
This is again a situation that's just hard for me to imagine, from both sides.
In my management experience, I've spent much more time dealing with people's stupid, childish bullshit than doing "real" work.
That's in my management experience as well. You don't believe it's a real thing until it happens, and then you're still left wondering, "Seriously, am I really dealing with this shit?"
> So managers are supposed to be part time kindergarten nannies now?
If their employees act like children, then yes. And I'm pretty sure we can all come up with examples of adults acting like children.
If their employees act like children, then yes. And I'm pretty sure we can all come up with examples of adults acting like children.
Kindergarten nannies generally don't have the talent for teaching people how to not be jerks - they just put the jerks in timeout. Some eventually grow out of it. Some eventually get medication and/or therapy that addresses the issue. Some eventually respond to a lack of environmental stressors when they move out of their parents' house. And some simply stay that way.
If you've hired someone in the latter category, yes, delivering business value requires you to deal with it. Firing them is an option, if that's really net positive for business value; sometimes it isn't. Seeing if you have the knack for teaching them is another, and likely highly profitable if you can do it. Handing down rigid rules is yet another option - not great, but quick and usually effective.
If you've hired someone in the latter category, yes, delivering business value requires you to deal with it. Firing them is an option, if that's really net positive for business value; sometimes it isn't. Seeing if you have the knack for teaching them is another, and likely highly profitable if you can do it. Handing down rigid rules is yet another option - not great, but quick and usually effective.
When the two adults are a man and a woman, yes, the company gets pretty scared pretty fast at the implications. Hence the rules.
> I'm not sure how I'd respond
That might depend on whether or not you were worried about losing your job over it.
That might depend on whether or not you were worried about losing your job over it.
Right, hence my comment. I wouldn't be worried at all. It feels like a different working culture (or country).
It seems like one of those cultural things, like how Europeans and Americans have inverse views on the severity of sex and violence.
I think you may have even misinterpreted my comment as me thinking I'm somehow "above the rules" or whatever, even. It's not that. It's that it would have literally never crossed my mind that my continued employment would be related to how I behave in relation to this document.
In my personal life I left behind that sort of 'rules lawyering' when I left school. No job I've ever held has actually functioned like that. The closest I've been is working at a large accounting firm, but even there most people had a mutual understanding that the 'book' (a lot more official than this post) was just a CYA fluff document invented to keep certain people busy and certain other people happy.
It seems like one of those cultural things, like how Europeans and Americans have inverse views on the severity of sex and violence.
I think you may have even misinterpreted my comment as me thinking I'm somehow "above the rules" or whatever, even. It's not that. It's that it would have literally never crossed my mind that my continued employment would be related to how I behave in relation to this document.
In my personal life I left behind that sort of 'rules lawyering' when I left school. No job I've ever held has actually functioned like that. The closest I've been is working at a large accounting firm, but even there most people had a mutual understanding that the 'book' (a lot more official than this post) was just a CYA fluff document invented to keep certain people busy and certain other people happy.
I also wonder this when looking at the "Workplace" Stack Exchange site or at Reddit's /r/legaladvice. So much CYA stuff, "you must collect a paper trail", "email yourself your recollection of the situation, so it's time stamped", how quickly you can become a liability for HR and how much companies are scared of getting sued for, to me, frivolous reasons.
Sure this also exists in corporate Europe, but it seems America is more "robotic" in this respect. Well, they'd just call it being professional I guess.
Sure this also exists in corporate Europe, but it seems America is more "robotic" in this respect. Well, they'd just call it being professional I guess.
Eh, many of the questions in Workplace are about FUBAR situations. It makes sense to take extreme measures in those cases.
Sure, but I think that is itself the cultural thing. I'll take your word for it that some people experience this, but the idea that anyone would fear being fired for missing fiddly details of a document like this is completely foreign to me. I've never seen a code review checklist that people followed even 75% of.
> I'll take your word for it that some people experience this, but the idea that anyone would fear being fired for missing fiddly details of a document like this is completely foreign to me.
There is currently a very public example of this going on at Stack Exchange over a code of conduct that was still in review status.
https://meta.stackexchange.com/questions/336526/stack-overfl...
There is currently a very public example of this going on at Stack Exchange over a code of conduct that was still in review status.
https://meta.stackexchange.com/questions/336526/stack-overfl...
Yes, that's a good example of what's completely foreign to me. I don't have time to trawl through all the details of the story you linked, but it's incomprehensible to me why Stack Exchange would do what's described here, and I've multiple times made negative comments about a proposed set of rules without facing or fearing punishment.
> I don't have time to trawl through all the details of the story you linked
Having done so myself out of morbid curiosity (and a sense of "there must be another side of the story that we're not getting"[0]) I would not suggest it. It's a rabbit hole that, as far as I can tell, leads back to the post I linked as the most accurate summary of the situation.
[0]: If there is, I have yet to find it after hours and hours of time wasted.
Having done so myself out of morbid curiosity (and a sense of "there must be another side of the story that we're not getting"[0]) I would not suggest it. It's a rabbit hole that, as far as I can tell, leads back to the post I linked as the most accurate summary of the situation.
[0]: If there is, I have yet to find it after hours and hours of time wasted.
CoC are a tool like any other - helpful if applied to the right situations and harmful if misused. I think you've made a really insightful point about keeping it small and maybe less a rule book and more a guideline for interaction.
#3 used to be called "professionalism"
A comic take on professionalism: https://www.youtube.com/watch?v=ZRzxJZrm3iU
https://arresteddevelopment.fandom.com/wiki/Wayne_Jarvis
A comic take on professionalism: https://www.youtube.com/watch?v=ZRzxJZrm3iU
https://arresteddevelopment.fandom.com/wiki/Wayne_Jarvis
The ever-expanding-CoC made me think a lot about the parallels to how laws are made.
I do agree the prolific laundry list of Do/Don’t criteria becomes more mentally hampering over time to circumnavigate.
Realistically speaking, If there was feedback/questions to bring up, I would just want to leave a comment putting my thoughts into words as efficiently as possible to then continue reviewing. Not a snarky, underhanded jerk comment like something an opponent would say in a multiplayer videogame - just a regular, normal person’s comment in a plain way.
For example, “What is the goal of this section?”, “Why use X method?”, “Please use ABC naming convention for the function here :)“.
Some of the “Do” ways are just about repackaging a concise statement into a more lengthy form for the sake of issue indirectness (or trying to avoid anything negative). This to me seems less efficient in a code review because it then takes longer to jump through all the mental gymnastics and mulling over phrasing of how to write a sentence referring to a piece of code.
I’m not at all saying be a ruthless hard-ass with the emotion of a robot. It’s just policies like a CoC that grows and grows and grows to retroactively counteract every single offense or misgiving feels like drudging through a bunch of HR bullshit. Coders already have enough mental exertion as it is.
The simpler and shorter the list for the CoC, the better (IMO). Will it encompass every possible instance or example? No. Will it please everyone? No. Will it get the job done so everyone can go home? Yes.
Look, every coder is going to make a mistake at some point and every reviewer is going to make a mistake, too. It’s ok to respectfully agree/disagree/debate perspectives about the code concisely. There is no need to turn an (unfun) diligence process into a pissing contest or a witch-hunt or a gatekeeping exercise. The people who do are wasting everyone’s time. Accept responsibility. Accept feedback. Reach a resolution and move on.
I do agree the prolific laundry list of Do/Don’t criteria becomes more mentally hampering over time to circumnavigate.
Realistically speaking, If there was feedback/questions to bring up, I would just want to leave a comment putting my thoughts into words as efficiently as possible to then continue reviewing. Not a snarky, underhanded jerk comment like something an opponent would say in a multiplayer videogame - just a regular, normal person’s comment in a plain way.
For example, “What is the goal of this section?”, “Why use X method?”, “Please use ABC naming convention for the function here :)“.
Some of the “Do” ways are just about repackaging a concise statement into a more lengthy form for the sake of issue indirectness (or trying to avoid anything negative). This to me seems less efficient in a code review because it then takes longer to jump through all the mental gymnastics and mulling over phrasing of how to write a sentence referring to a piece of code.
I’m not at all saying be a ruthless hard-ass with the emotion of a robot. It’s just policies like a CoC that grows and grows and grows to retroactively counteract every single offense or misgiving feels like drudging through a bunch of HR bullshit. Coders already have enough mental exertion as it is.
The simpler and shorter the list for the CoC, the better (IMO). Will it encompass every possible instance or example? No. Will it please everyone? No. Will it get the job done so everyone can go home? Yes.
Look, every coder is going to make a mistake at some point and every reviewer is going to make a mistake, too. It’s ok to respectfully agree/disagree/debate perspectives about the code concisely. There is no need to turn an (unfun) diligence process into a pissing contest or a witch-hunt or a gatekeeping exercise. The people who do are wasting everyone’s time. Accept responsibility. Accept feedback. Reach a resolution and move on.
No emoji’s in code reviews? I have a hard time seeing how that helps instead of hurts things.
What was the reasoning behind it?
What was the reasoning behind it?
Your description here is what I would characterize as an accurate description of how most laws "just growed" in most communities.
That is, at least up until the time that the special interests got involved. Then they made a point of ensuring that the laws got written to only be applied to "others".
That is, at least up until the time that the special interests got involved. Then they made a point of ensuring that the laws got written to only be applied to "others".
> Your description here is what I would characterize as an accurate description of how most laws "just growed" in most communities.
> That is, at least up until the time that the special interests got involved. Then they made a point of ensuring that the laws got written to only be applied to "others".
When was this time that special interests did not exist? One might argue (only partly in jest) that the entire purpose of government is to protect special interests.
> That is, at least up until the time that the special interests got involved. Then they made a point of ensuring that the laws got written to only be applied to "others".
When was this time that special interests did not exist? One might argue (only partly in jest) that the entire purpose of government is to protect special interests.
This blog post originated at Google, but it is not some kind of Code of Conduct everyone at Google has to follow. This was an episode of Testing on the Toilet, as the name implies it is a newsletter, which is posted in the bathrooms.
> The CoC became a tool for power plays. For a few bad apples, the CoC became a list of gotchas that they could use as a power play against others
They should add that you shouldn’t do that to the code of conduct! ;)
They should add that you shouldn’t do that to the code of conduct! ;)
First rule of the CoC: You do not talk about the CoC.
[deleted]
Yes. CoCs are toxic and all workplaces with CoCs quickly become gamed toxic work environments. It's a nightmare. I never accept offers to join places with CoCs. In order to be effective and efficient I require the ability to speak plainly, clearly and directly about code quality and design issues.
Companies that use these tend to stop making good code and products and start using lobbyists to use regulations to anchor their dismally low quality products and squash any upcoming free-thinking rivals with good design.
Companies that use these tend to stop making good code and products and start using lobbyists to use regulations to anchor their dismally low quality products and squash any upcoming free-thinking rivals with good design.
Hmmm number 3 doesn't seem like a negative or counter-intuitive. I feel like software devs (in bay area startup world, or FAANG) are usually a certain demographic and they usually come from a place of privilege where they've never had to do the mental work that's involved in communicating effectively across diverse demographics.
> Hmmm number 3 doesn't seem like a negative or counter-intuitive.
This is the core of the problem: Some people like the idea of robotic, semi-scripted conversations according to a well-defined set of rules. They think that if there's a rulebook, they can predict how people will respond to them. Or worse, they think they can control how people react and respond.
Yes, we should put some constraints around egregious communication issues as well as provide general guidelines, but the important thing is to let people interact naturally within those loose constraints.
> I feel like software devs (in bay area startup world, or FAANG) are usually a certain demographic and they usually come from a place of privilege where they've never had to do the mental work that's involved in communicating effectively across diverse demographics.
It's a fallacy, though. You can't create perfect rules for human conversation. Everyone needs to build those interpersonal and conversational skills, even if it's somewhat painful along the way. The basic skills can be guided with simple rules and guidelines, but the problem comes when you try to control too much of the conversation or set the constraints so tightly that it's a minefield to navigate.
This is the core of the problem: Some people like the idea of robotic, semi-scripted conversations according to a well-defined set of rules. They think that if there's a rulebook, they can predict how people will respond to them. Or worse, they think they can control how people react and respond.
Yes, we should put some constraints around egregious communication issues as well as provide general guidelines, but the important thing is to let people interact naturally within those loose constraints.
> I feel like software devs (in bay area startup world, or FAANG) are usually a certain demographic and they usually come from a place of privilege where they've never had to do the mental work that's involved in communicating effectively across diverse demographics.
It's a fallacy, though. You can't create perfect rules for human conversation. Everyone needs to build those interpersonal and conversational skills, even if it's somewhat painful along the way. The basic skills can be guided with simple rules and guidelines, but the problem comes when you try to control too much of the conversation or set the constraints so tightly that it's a minefield to navigate.
I like robotic conversations at work because my work is not important to me, and I don’t want to bring my “full self” to work. I like to get in, do my shit, conduct communication in a formalized manner, and get out in 6 hours and go do stuff that matters to me.
>Some people like the idea of robotic, semi-scripted conversations according to a well-defined set of rules.
Thinking about the words you use in daily conversation with diverse peers doesn't have to be "robotic". You're just being exposed to how other people in less privileged positions have to think and act, and that's uncomfortable.
>You can't create perfect rules for human conversation.
Is anyone here claiming that their goal is perfection?
>Everyone needs to build those interpersonal and conversational skills
And there's nothing wrong with receiving help and guidance while you're on that journey.
Thinking about the words you use in daily conversation with diverse peers doesn't have to be "robotic". You're just being exposed to how other people in less privileged positions have to think and act, and that's uncomfortable.
>You can't create perfect rules for human conversation.
Is anyone here claiming that their goal is perfection?
>Everyone needs to build those interpersonal and conversational skills
And there's nothing wrong with receiving help and guidance while you're on that journey.
> Thinking about the words you use in daily conversation with diverse peers doesn't have to be "robotic".
I'd much rather save my mental energy for the coding.
If having to carefully think about how every turn of phrase could be seen and completely giving up on being able to make friends at work is the price of hiring less privileged people? I'd rather stay in my privileged monoculture thanks. The underprivileged can go join the company with the underprivileged monoculture and everyone can be happier, communicating in the style they prefer with people who are ok with that style.
I'd much rather save my mental energy for the coding.
If having to carefully think about how every turn of phrase could be seen and completely giving up on being able to make friends at work is the price of hiring less privileged people? I'd rather stay in my privileged monoculture thanks. The underprivileged can go join the company with the underprivileged monoculture and everyone can be happier, communicating in the style they prefer with people who are ok with that style.
> You're just being exposed to how other people in less privileged positions have to think and act, and that's uncomfortable.
No, it has nothing to do with that. As an unprivileged person, CoCs were harder for me to follow because I grew up using much harsher language and being more direct.
CoCs mainly seem to be about privileged people affirming to each other how “woke” they are.
No, it has nothing to do with that. As an unprivileged person, CoCs were harder for me to follow because I grew up using much harsher language and being more direct.
CoCs mainly seem to be about privileged people affirming to each other how “woke” they are.
It’s easy to come to a point where we only talk in coded ways to follow the letter of the rules.
People start saying “thanks for your feedback, we’ll look into this issue when planning the next iteration” as code for “you could be pissing in the wind”, and most communication becomes non sensical where you need to feel if they actually mean what they’re saying in text form.
Then we lament that face to face time is needed to communicate efficiently with people.
People start saying “thanks for your feedback, we’ll look into this issue when planning the next iteration” as code for “you could be pissing in the wind”, and most communication becomes non sensical where you need to feel if they actually mean what they’re saying in text form.
Then we lament that face to face time is needed to communicate efficiently with people.
bluntfang(5)
It is a negative. Nobody wants to navigate a social minefield where everything they say can and will be used against them in a court of public opinion. If social interaction becomes too difficult and risky, it's better to not participate at all.
This is why people get stonewalled. Why would anyone speak their mind when there are serious consequences?
This is why people get stonewalled. Why would anyone speak their mind when there are serious consequences?
bluntfang(3)
> software devs are usually a certain demographic and they usually come from a place of privilege where they've never had to do the mental work that's involved in communicating effectively across diverse demographics.
This strikes me as a surprising assertion: most software devs (including in the Bay Area, FAANG) are recent immigrants (or temporary guest workers) who are, by definition, navigating a culture very different than the one they grew up in.
This strikes me as a surprising assertion: most software devs (including in the Bay Area, FAANG) are recent immigrants (or temporary guest workers) who are, by definition, navigating a culture very different than the one they grew up in.
> most software devs (including in the Bay Area, FAANG) are recent immigrants
The only data I can find that speaks to that (from 2015) has far less than half foreign-born (39.2% of "software engineers", 27.4% of "computer programmers", etc.), and obviously not all of those are going to be "recent immigrants".
https://www.americanimmigrationcouncil.org/research/foreign-...
The only data I can find that speaks to that (from 2015) has far less than half foreign-born (39.2% of "software engineers", 27.4% of "computer programmers", etc.), and obviously not all of those are going to be "recent immigrants".
https://www.americanimmigrationcouncil.org/research/foreign-...
40% is a lot. Don't forget tech's exponential growth in jobs over the past few decades.
I’d be curious to see that from FAANG specifically, since they drive so much international hiring.
> most software devs (including in the Bay Area, FAANG) are recent immigrants (or temporary guest workers)
The only data I can find supporting this is that tech workers in Silicon Valley as a whole are majority foreign-born. But that includes everyone from Oracle to HP to Wipro, not just FAANG. (And I think the definition of Silicon Valley in this data is the traditional one, i.e., not including San Francisco.)
A subset of those are recent immigrants.
The only data I can find supporting this is that tech workers in Silicon Valley as a whole are majority foreign-born. But that includes everyone from Oracle to HP to Wipro, not just FAANG. (And I think the definition of Silicon Valley in this data is the traditional one, i.e., not including San Francisco.)
A subset of those are recent immigrants.
While I agree with the core of "DON’T: Criticize the person. ",
I'm not sure I agree with the example. I get that avoiding "you" and "your" can help with setting the tone, but ultimately it matters more what is said in particular. The DO example to me seems overly distant, almost snarky and amplifying criticism in my mind, like somebody explicitly telling you "don't take it personally":
"This concurrency model appears to be adding complexity to the system without any visible performance benefit." just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Might be only me, maybe.
In my experience, I have received a lot of comments with "you" and "your" on them but they were still constructive. But when I received very sterile ones like the DO example I typically felt a bit weird, guilty, like someone actively avoiding to tell me what they think, or colleagues using extremely formal/polite language. A lot of things can carry weird undertones, depending on context.
"This concurrency model appears to be adding complexity to the system without any visible performance benefit." just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Might be only me, maybe.
In my experience, I have received a lot of comments with "you" and "your" on them but they were still constructive. But when I received very sterile ones like the DO example I typically felt a bit weird, guilty, like someone actively avoiding to tell me what they think, or colleagues using extremely formal/polite language. A lot of things can carry weird undertones, depending on context.
It does come off as passive-aggressive. The tone is so final and closed to all discussion.
If I were on the receiving end of that feedback, I'd much rather have the reviewer write something like:
"Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage."
If I were on the receiving end of that feedback, I'd much rather have the reviewer write something like:
"Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage."
This is exactly the tone I try to strike, especially with junior colleagues. I never want to make someone’s decision seem unambiguously wrong-headed when it comes to subjective engineering matters. That just breaks someone’s confidence.
Even if their decisions are sometimes unambiguously wrong-headed. It’s just an experience thing, but I’m trying to figure out how to tell them ‘trust me, I’ve made this mistake before, please don’t do it’, without trying to explain the 2 or 3 years of experience that prompt it.
Hopefully you are able to summarize -- that's what I try to do. Just asking for people to blindly trust you is a big ask, for two reasons.
First, experienced people often develop mistaken beliefs. They try something and it happens to work, even though it might even be wrong in principle. Without any rationale, how is the person you're talking to going to judge whether the beliefs you've developed are correct?
Second, even if they're willing to trust you, just getting the rule handed down does not grow them as an engineer. I might prefer to make a mistake and learn than take the right path without understanding the reasons. In fact, I have done so, intentionally.
So, again, IMO, the goal you've mentioned in this comment ("figure out how to tell them `trust me...`") is an incorrect goal, and one you should not pursue.
First, experienced people often develop mistaken beliefs. They try something and it happens to work, even though it might even be wrong in principle. Without any rationale, how is the person you're talking to going to judge whether the beliefs you've developed are correct?
Second, even if they're willing to trust you, just getting the rule handed down does not grow them as an engineer. I might prefer to make a mistake and learn than take the right path without understanding the reasons. In fact, I have done so, intentionally.
So, again, IMO, the goal you've mentioned in this comment ("figure out how to tell them `trust me...`") is an incorrect goal, and one you should not pursue.
It's a good opportunity to teach them.
You could ask questions that lead them to the same conclusion (with the benefit of hindsight from your experience): "What happens if this system is terminated before the final write call completes?" "How does this handle someone accidentally starting another instance of the application at the same time?" "If this external service we use is down, what happens?"
You could also walk through the story of your prior mistake, draw parallels to the new code, and then discuss how to avoid it.
You could ask questions that lead them to the same conclusion (with the benefit of hindsight from your experience): "What happens if this system is terminated before the final write call completes?" "How does this handle someone accidentally starting another instance of the application at the same time?" "If this external service we use is down, what happens?"
You could also walk through the story of your prior mistake, draw parallels to the new code, and then discuss how to avoid it.
Explaining the experience would make it a learning moment, "trust me don't do this" doesn't do much to legitimately motivate an alternative choice
As some other commenters wrote, you can probably be more direct:
"Why use XYZ pattern instead of ABC? This seems more complicated than necessary right now."
There's also a useful communication/pedagogy technique I learned many years ago called "praise, correct, praise". You frame a "correction" or negative feedback with either neutral or positive feedback. The idea is that the other person receives the feedback without feeling "stung" or associating it with negativity, which could reduce their receptiveness or even their ability to retain and/or apply the information.
"Why use XYZ pattern instead of ABC? This seems more complicated than necessary right now."
There's also a useful communication/pedagogy technique I learned many years ago called "praise, correct, praise". You frame a "correction" or negative feedback with either neutral or positive feedback. The idea is that the other person receives the feedback without feeling "stung" or associating it with negativity, which could reduce their receptiveness or even their ability to retain and/or apply the information.
Leading with 'why' often puts people on the defensive as it usually comes off as demanding. Asking someone to walk you through their rational lets them explain it on their terms, and assumes there is reasoning that you're unaware of that they may have discovered.
That's a good point. I just wanted to demonstrate that the wording could be tighter.
“Praise, correct, praise” is less fawningly known as “shit sandwich” and appears incredibly patronizing to anyone who is either aware of the method or possesses minimal skills of observation.
It's not meant to be a shit sandwich. This might be a "no true Scotsman" argument. But sometimes it's just a matter of re-ordering the sentences in an email.
The fact that at least an effort was made does count for something, I believe.
But the motivation for that effort varies from 'this person is actively seeking to manipulate me with poppsych to use my failed career as a stepping stone' to 'genuinely concerned about how I feel about recieving critical feedback.'
Thank you for the fantastic example for how to respectfully review code. It’s important to remember that a diff does not include the process for how the solution was arrived at, and I try to leave openings like yours so that the submitter of the PR has space to explain why something can’t be e.g. more straightforward without getting confrontational.
I can’t say I agree with this thread. Oftentimes I’ll say “It seems like <bad thing>, I would probably <different approach>.” After years of doing software development I’m well accustomed to side effects, business constraints, legacy compatibility, time, etc forcing a solution that seems overly complex at first look but is actually necessary and sufficient. I want to explain to the person where I’m coming from and give them a fair chance to refute my perspective. Rather than assume I’m correct from the get-go and start using flowery language to get them to change.
At the end of the day I feel like our goals should be aligned to create the ideal balance of software for the larger goal. They should also be able to think critically about the problem I see and the solution I might offer.
If the person seems to be doing something boneheaded over and over after I’ve spent literally days, weeks, or months explaining easier and better solutions, and I keep getting forced to deal with it, then I may well conclude there are no extenuating factors and “this code is shit” just because of the person’s ego.
If somebody starts asking me to ‘walk them through the code’, that’d be a red flag to me. If this is someone technical, then I’d expect them to be able to follow the code themselves. If this is someone non-technical, then I expect that giving them knowledge is going to be more dangerous than useful.
It’s also disingenuous and feels like it creates a sort of prisoner’s dilemma. If I’m open about the pros and cons and details of my ideas, but they’re closed and use persuasive language to advocate for their own ideas, I don’t feel like it’s really a collaborative process anymore. They’re taking a route that gives them a distinct advantage, not because their ideas are better at meeting the overall goal, but because they’re tapping into people’s subjective preferences and drives to further their own self-interests.
Lots of companies do have this culture, but the end result seems to be to create an echo chamber where a blind eye is turned to vast inefficiency. Oftentimes this seems to be things that are hard to quantify like user experience and reliability or security, where you silently lose customers without a ticket being opened, so there’s no business pushback to bad technical decisions that hurt the product.
At the end of the day I feel like our goals should be aligned to create the ideal balance of software for the larger goal. They should also be able to think critically about the problem I see and the solution I might offer.
If the person seems to be doing something boneheaded over and over after I’ve spent literally days, weeks, or months explaining easier and better solutions, and I keep getting forced to deal with it, then I may well conclude there are no extenuating factors and “this code is shit” just because of the person’s ego.
If somebody starts asking me to ‘walk them through the code’, that’d be a red flag to me. If this is someone technical, then I’d expect them to be able to follow the code themselves. If this is someone non-technical, then I expect that giving them knowledge is going to be more dangerous than useful.
It’s also disingenuous and feels like it creates a sort of prisoner’s dilemma. If I’m open about the pros and cons and details of my ideas, but they’re closed and use persuasive language to advocate for their own ideas, I don’t feel like it’s really a collaborative process anymore. They’re taking a route that gives them a distinct advantage, not because their ideas are better at meeting the overall goal, but because they’re tapping into people’s subjective preferences and drives to further their own self-interests.
Lots of companies do have this culture, but the end result seems to be to create an echo chamber where a blind eye is turned to vast inefficiency. Oftentimes this seems to be things that are hard to quantify like user experience and reliability or security, where you silently lose customers without a ticket being opened, so there’s no business pushback to bad technical decisions that hurt the product.
Why would asking someone to walk you through code be a red flag? Isn't it usually a way to understand the context of choices, not that a reader can't understand what code is doing?
While that's a much better formulation, it still lacks substance. Elaborate what is it that you find complex and sketch some concrete simpler alternatives. Review cycles where one side asks for reams of explanations, while being parsimonious with their own effort are quite unproductive.
[deleted]
Most of the time you already know the answer: copy pasted from somewhere else or too much to do, too little time.
"Can you walk me through it" is also something I would ask to a student, not to someone whom I'm sharing the workload with.
If someone says it's too complex, it probably really is.
If someone says it's too complex, probably it also mean that maintaing that code could prove hard in the future.
So, unless you prove there are real, tangible, measurable benefits, it's better to find a different, simpler, implementation, no matter the rationale.
"Can you walk me through it" is also something I would ask to a student, not to someone whom I'm sharing the workload with.
If someone says it's too complex, it probably really is.
If someone says it's too complex, probably it also mean that maintaing that code could prove hard in the future.
So, unless you prove there are real, tangible, measurable benefits, it's better to find a different, simpler, implementation, no matter the rationale.
If someone says it's too complex, they may not be in possession of 100% of the facts; if the code is actually too complex, the author will probably work that out when asked to explain.
Consider a PR using mutexes instead of channels in a go project. Is the code in the hot path, and has it been benchmarked? How many mutexes are there, and are they hidden from external callers? Is the PR implementing an existing algorithm, or an algorithm with some desired behaviour which would be more difficult with channels? Which way is consistent with the rest of the codebase?
This is not an adversarial encounter between a defender of the codebase and a filthy peasant seeking to corrupt all that is pure and good, it's two programmers trying to solve a problem, and neither is likely to have 100% of the available information, so you should talk to each other and work out what's going on. Assuming that you're right will bias you towards your existing opinion, which will cause worse code to make it to production.
Consider a PR using mutexes instead of channels in a go project. Is the code in the hot path, and has it been benchmarked? How many mutexes are there, and are they hidden from external callers? Is the PR implementing an existing algorithm, or an algorithm with some desired behaviour which would be more difficult with channels? Which way is consistent with the rest of the codebase?
This is not an adversarial encounter between a defender of the codebase and a filthy peasant seeking to corrupt all that is pure and good, it's two programmers trying to solve a problem, and neither is likely to have 100% of the available information, so you should talk to each other and work out what's going on. Assuming that you're right will bias you towards your existing opinion, which will cause worse code to make it to production.
You have to realize everyone is coming from different backgrounds with different communication styles. I think of it as everyone has a different "volume" that they listen at.
Some people grew up in families that were more confrontational, and they're at a higher volume. They're comfortable with more confrontational communication styles.
For others, from quieter backgrounds, if someone says, "Why the hell did you do it this way?" it's going to be a slap in the face because, hey, no one in my family talks to me that way, and I'm not going to let YOU do it. It comes across as very, very hostile.
I think everyone though should learn to communicate non-aggressively, though, because it suits the needs of most people. When you start doing it it feels weird or passive aggressive, but you get used to it and it leads to much less friction.
There are some people who might argue "the quieter types need to be less sensitive" but that's not how it works. If I'm a quieter type (and I am), I'm going to leave the company instead of have people yell at me, or it's going to severely affect my productivity.
And honestly, I question if any team can really thrive in a very aggressive environment, where it becomes more about ego and dominance that the quality of ideas at the table. Google did a study that showed the only factor that determined how well a team did was feelings of psychological safety, and a huge part of that is just talking to each other respectfully.
Some people grew up in families that were more confrontational, and they're at a higher volume. They're comfortable with more confrontational communication styles.
For others, from quieter backgrounds, if someone says, "Why the hell did you do it this way?" it's going to be a slap in the face because, hey, no one in my family talks to me that way, and I'm not going to let YOU do it. It comes across as very, very hostile.
I think everyone though should learn to communicate non-aggressively, though, because it suits the needs of most people. When you start doing it it feels weird or passive aggressive, but you get used to it and it leads to much less friction.
There are some people who might argue "the quieter types need to be less sensitive" but that's not how it works. If I'm a quieter type (and I am), I'm going to leave the company instead of have people yell at me, or it's going to severely affect my productivity.
And honestly, I question if any team can really thrive in a very aggressive environment, where it becomes more about ego and dominance that the quality of ideas at the table. Google did a study that showed the only factor that determined how well a team did was feelings of psychological safety, and a huge part of that is just talking to each other respectfully.
For some people (such as myself), hearing very formal commentary makes me feel very psychologicall unsafe. I'd much rather an angry rant because it feels like it's coming from someone who is my equal, rather than a representative of The Company who can ruin me.
I don't think it has to be that dramatic. The key is just avoiding the word "you" and not using laden words like "stupid" and "shitty."
You're still free to say stuff like:
----------------
"I really fundamentally disagree with this approach and know it will lead to problems done the road. Let's talk through it in person."
Or "If we do this maintaining it in the future could be really hard. I veto it."
Or "This isn't going to work with our XYZ system. Please fix that and submit it again."
----------------
You can say a lot of things rather directly that way. You DON'T need to say things like:
----------------
"This is a f*ING awful idea!"
Or "What were you thinking?"
Or "This is horrible. Don't check in crap to our code base, think through things first."
----------------
Is it too great of an imposition to ask people to phrase things the first way? Honest question.
You're still free to say stuff like:
----------------
"I really fundamentally disagree with this approach and know it will lead to problems done the road. Let's talk through it in person."
Or "If we do this maintaining it in the future could be really hard. I veto it."
Or "This isn't going to work with our XYZ system. Please fix that and submit it again."
----------------
You can say a lot of things rather directly that way. You DON'T need to say things like:
----------------
"This is a f*ING awful idea!"
Or "What were you thinking?"
Or "This is horrible. Don't check in crap to our code base, think through things first."
----------------
Is it too great of an imposition to ask people to phrase things the first way? Honest question.
This direct approach sounds really good, but seems dependent on having a certain personality to pull off without sounding aggressive. For those with low EQ, a shit sandwich is just easier to execute.
It's not a personality. It's just posture. Just relax your body, and speak softly.
It's actually quite simple. Relax, speak softly, literally just avoid the word "you" (it makes a difference), avoid loaded words. Be direct and be kind. That's it.
I can't be convinced that software engineers, who can learn incredible amounts of technical details in a limited amount of time, can't learn that too.
It's just a habit. Difficult to form, maybe, but good once you form it.
It's actually quite simple. Relax, speak softly, literally just avoid the word "you" (it makes a difference), avoid loaded words. Be direct and be kind. That's it.
I can't be convinced that software engineers, who can learn incredible amounts of technical details in a limited amount of time, can't learn that too.
It's just a habit. Difficult to form, maybe, but good once you form it.
Then fix the low EQ, aka "not good at working with others". That's not the person beibg reviewed's fault.
> "This concurrency model appears to be adding complexity to the system without any visible performance benefit." just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Might be only me, maybe.
After the knee-jerk "this is shit" reaction, the slightly deeper thought is probably along the lines of "There are better ways to do this that would simplify the code (reducing the chances of bugs, makes it easier to maintain) but it would basically require a complete rewrite".
I think the challenge here is what you don't know is: Did the person writing the code just not see a simpler way? Did they iterate to this, and realize it's overly-complex but don't want to spend the time to redo it now? Do they think this is actually the best approach, and has some other merits that make it better than other approaches?
How have others approached this in the past?
I've definitely been in the situation where I had to review code like this, and I've definitely agonized over how to carefully phrase "I think this is shit" in a nicer way. If my review would really be "Sorry but I pretty much think we need to take a completely different approach and rewrite 95% of this" then I will typically have the conversation in person to discuss it, as it's easier to be clear I'm not criticizing the person and also avoid misunderstanding something (either direction). Sometimes it turns out I was missing something, but maybe there's still an intermediate way that reduces complexity but still addresses the problems the original did.
After the knee-jerk "this is shit" reaction, the slightly deeper thought is probably along the lines of "There are better ways to do this that would simplify the code (reducing the chances of bugs, makes it easier to maintain) but it would basically require a complete rewrite".
I think the challenge here is what you don't know is: Did the person writing the code just not see a simpler way? Did they iterate to this, and realize it's overly-complex but don't want to spend the time to redo it now? Do they think this is actually the best approach, and has some other merits that make it better than other approaches?
How have others approached this in the past?
I've definitely been in the situation where I had to review code like this, and I've definitely agonized over how to carefully phrase "I think this is shit" in a nicer way. If my review would really be "Sorry but I pretty much think we need to take a completely different approach and rewrite 95% of this" then I will typically have the conversation in person to discuss it, as it's easier to be clear I'm not criticizing the person and also avoid misunderstanding something (either direction). Sometimes it turns out I was missing something, but maybe there's still an intermediate way that reduces complexity but still addresses the problems the original did.
Yeah, if someone is just flat out wrong, it's better to talk in person about it, walk through the details, and tell them why you disagree with them or show them their mistakes.
I came here to post the exact same thing. I find using "we" is a good workaround. And it's not just a euphemistic way of saying "you" — as a code reviewer, I know I'm also responsible for catching bugs or performance regressions, and by saying "we" I feel I'm expressing that shared responsibility.
You can also use "I". Trying to avoid pronouns can make it sound like a high school lab report with excessive passive voice (e.g. "The baking soda was mixed with the vinegar" vs. "I mixed the baking soda with the vinegar").
In this example, I might say "This seems very complicated to me. Would a simpler model work?". If I had something simpler in mind specifically I would suggest it.
In this example, I might say "This seems very complicated to me. Would a simpler model work?". If I had something simpler in mind specifically I would suggest it.
This example is bad because it shows two things at once: the "do" comment is impersonal and more descriptive. I somehow interpreted it as "describe the issue better", which is generally good advice, but not exactly the point they are showing.
Anyways, I think that "you" has a place in comments. It is when the comment is addressed to the person. Obvious but let me explain.
For example if I know that some developer is particularly good at optimization, and I see some approach that looks slow, I will say, "why did you use this approach?". This is, in a sense, a mark or respect. "this approach appears to be slow" puts you in place of the teacher, but in some cases, I know that the person I am reviewing is better than me and I'd rather be the student. "why did you do that?" means "if it isn't a mistake, please enlighten me".
Anyways, I think that "you" has a place in comments. It is when the comment is addressed to the person. Obvious but let me explain.
For example if I know that some developer is particularly good at optimization, and I see some approach that looks slow, I will say, "why did you use this approach?". This is, in a sense, a mark or respect. "this approach appears to be slow" puts you in place of the teacher, but in some cases, I know that the person I am reviewing is better than me and I'd rather be the student. "why did you do that?" means "if it isn't a mistake, please enlighten me".
> just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that".
Maybe, but there is a huge difference between "I think this code is shit" and "I think you are shit".
Maybe, but there is a huge difference between "I think this code is shit" and "I think you are shit".
"This concurrency model appears to be adding complexity to the system without any visible performance benefit" is pretty hilariously cruel though.
Personally I might prefer "hmm, why did you do it this way?"
Personally I might prefer "hmm, why did you do it this way?"
I'll reply to your comment and simultaneously address a lot of the subcomments.
Most of what I say is based on what I've read in communications books. One day I hope to write a long post or a series of posts on the (mostly common) advice in the various books I've read.
Disclaimer: I'm not good at these communications tactics live, and often don't put the effort in while posting online, so don't use my comment history as a judge of my knowledge ;-)
> While I agree with the core of "DON’T: Criticize the person. ", I'm not sure I agree with the example.
The example is "Why are you using this approach? You’re adding unnecessary complexity."
To which a perfectly valid response is "Because it solves the problem, and I disagree that the complexity is unnecessary." Not a very constructive conversation, but completely valid. If you're not going to do the work to explain your concerns, I'm not going to do the work to satisfy you. A code review is not an arena where the reviewers get to ask and you the author are compelled to answer every concern. It is a dialogue, and a 2 way street.
I do agree with you and I don't like their suggestion either: "This concurrency model appears to be adding complexity to the system without any visible performance benefit."
Well, umm, that's just your opinion, dude. What do you want me to do about it?
The reviewer did not specify what/where the complexity is, nor an alternative. And worst of all, (s)he is not even asking a question. As the reader, I don't feel compelled to respond. If the reviewer has a better approach handy, by all means suggest it and point out its pros/cons, etc.
Regarding "you" statements, there is almost always a way to phrase them as "I" statements, but the important catch is that when you do, you usually should request something. The example in the blog post didn't, and it very much comes off as someone's opinion. What am I supposed to do with it?
There's no reason not to be direct. But there are different ways of being direct, and you'll be a lot more successful if you give your perspective.
nerdponx said:
> "Why use XYZ pattern instead of ABC? This seems more complicated than necessary right now."
At least this asks a question and gives an alternative. It still lacks any substance on why the reviewer thinks it is complicated (although in some contexts the difference in complexity between XYZ and ABC may be obvious).
Also, in my experience, statements like "complicated", "complex" are often tactics to simply force the reviewer's preferred coding style, and this is a lousy tactic. If I were the author I would ask them to clarify why they think it is complicated. If I were to think of the worst code review experiences I've had, it's where the reviewer is simply pushing for his/her preferred style. Some people like continue statements in for loops, others prefer heavily nested loops. Neither is better. Either have a policy about them in the team or don't highlight it. I don't want to keep switching styles back and forth depending on who will review my code.
> There's also a useful communication/pedagogy technique I learned many years ago called "praise, correct, praise".
Agree with other commenters that this can explode in your face. It is also a tactic that is recommended against in some of the books I've read. In the long run you also lose credibility where people don't take your legitimate praises seriously, and get defensive any time you praise them.
The tactic that is recommended is "Agree, and disagree." State first on the things you agree with the other person on, and then say "And I also think ..." and state your disagreement.
And obviously, don't say "But..."
> Oftentimes I’ll say “It seems like <bad thing>, I would probably <different approach>.”
This is OK if you are just expressing an opinion and don't require the author to respond or to change it. If, OTOH, you really want this changed, then this is a poor way of pointing it out. As a reader, all I see is an opinion. We all have them, and differences in opinion are fine. A difference in opinion alone is not an obligation to discuss it.
> "Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage."
I understand that you're seeing it as complex, but I did it this way because it solved the problem and I did not find it as complex. If you have an alternative solution that solves the problem that you think is not as complex, feel free to suggest it.
I think asking for a rationale without suggesting an alternative means the usual (truthful and valid) response is "Because it solved the problem." There are many scenarios: Maybe I couldn't think of any other approach. Maybe I thought of 5 of them and had reasons to select this one. Do you want me to list a lengthy response examining each of the approaches I thought off with a justification on why I didn't select it? I shouldn't have to do that much work.
It's much easier to respond if you proposed something and I'll address that. My response may well be "Thanks - I hadn't thought of that approach and agree with you."
As a general rule, do not ask questions that could require a lengthy answer unless you are willing to offer something on your side. It requires almost no effort on your part to ask a question, but it always requires effort - sometimes a lot - for the other person to formulate a non-dismissive response. Don't make the other person work hard without good reason. If you're going to ask a question, put in the effort to explain your perspective. Never, ever, assume that the author has an obligation to respond to you.
The Socratic approach, frankly, is a poor approach. It works in a very limited arena (e.g. teacher/student), and code reviews are not part of that arena. I learned this the hard way over many years where I'd ask very innocent questions, and occasionally someone would snap/shout at me. Only when I read the books did I understand: When asking a question, you have to make your concerns clear at the outset. You have to make the person understand why answering is a good idea. And you should never feel entitled to a response (and your words/tone/posture has to match).
Finally, as a general rule, be humble as a reviewer. Very often, the author knows more about the code or domain (or both) than you. And very often the author will have already considered your approach and has a very good reason not to go with it. It's perfectly fair game to point out that if the author had such a reason it would be good to put a comment about it as other people reading the code will share your concerns. But acting as if you know better is already sabotaging the conversation.
I would respond with "I do not understand the rationale behind the approach you picked. In my opinion approach XYZ is preferred and here are the benefits (list them all, including complexity). Did you happen to consider XYZ? If you did I'd like to hear your thoughts on the pros/cons of your approach over XYZ."
Paranoid people may read more in my response than there is, but you can't control that too much.
Most of what I say is based on what I've read in communications books. One day I hope to write a long post or a series of posts on the (mostly common) advice in the various books I've read.
Disclaimer: I'm not good at these communications tactics live, and often don't put the effort in while posting online, so don't use my comment history as a judge of my knowledge ;-)
> While I agree with the core of "DON’T: Criticize the person. ", I'm not sure I agree with the example.
The example is "Why are you using this approach? You’re adding unnecessary complexity."
To which a perfectly valid response is "Because it solves the problem, and I disagree that the complexity is unnecessary." Not a very constructive conversation, but completely valid. If you're not going to do the work to explain your concerns, I'm not going to do the work to satisfy you. A code review is not an arena where the reviewers get to ask and you the author are compelled to answer every concern. It is a dialogue, and a 2 way street.
I do agree with you and I don't like their suggestion either: "This concurrency model appears to be adding complexity to the system without any visible performance benefit."
Well, umm, that's just your opinion, dude. What do you want me to do about it?
The reviewer did not specify what/where the complexity is, nor an alternative. And worst of all, (s)he is not even asking a question. As the reader, I don't feel compelled to respond. If the reviewer has a better approach handy, by all means suggest it and point out its pros/cons, etc.
Regarding "you" statements, there is almost always a way to phrase them as "I" statements, but the important catch is that when you do, you usually should request something. The example in the blog post didn't, and it very much comes off as someone's opinion. What am I supposed to do with it?
There's no reason not to be direct. But there are different ways of being direct, and you'll be a lot more successful if you give your perspective.
nerdponx said:
> "Why use XYZ pattern instead of ABC? This seems more complicated than necessary right now."
At least this asks a question and gives an alternative. It still lacks any substance on why the reviewer thinks it is complicated (although in some contexts the difference in complexity between XYZ and ABC may be obvious).
Also, in my experience, statements like "complicated", "complex" are often tactics to simply force the reviewer's preferred coding style, and this is a lousy tactic. If I were the author I would ask them to clarify why they think it is complicated. If I were to think of the worst code review experiences I've had, it's where the reviewer is simply pushing for his/her preferred style. Some people like continue statements in for loops, others prefer heavily nested loops. Neither is better. Either have a policy about them in the team or don't highlight it. I don't want to keep switching styles back and forth depending on who will review my code.
> There's also a useful communication/pedagogy technique I learned many years ago called "praise, correct, praise".
Agree with other commenters that this can explode in your face. It is also a tactic that is recommended against in some of the books I've read. In the long run you also lose credibility where people don't take your legitimate praises seriously, and get defensive any time you praise them.
The tactic that is recommended is "Agree, and disagree." State first on the things you agree with the other person on, and then say "And I also think ..." and state your disagreement.
And obviously, don't say "But..."
> Oftentimes I’ll say “It seems like <bad thing>, I would probably <different approach>.”
This is OK if you are just expressing an opinion and don't require the author to respond or to change it. If, OTOH, you really want this changed, then this is a poor way of pointing it out. As a reader, all I see is an opinion. We all have them, and differences in opinion are fine. A difference in opinion alone is not an obligation to discuss it.
> "Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage."
I understand that you're seeing it as complex, but I did it this way because it solved the problem and I did not find it as complex. If you have an alternative solution that solves the problem that you think is not as complex, feel free to suggest it.
I think asking for a rationale without suggesting an alternative means the usual (truthful and valid) response is "Because it solved the problem." There are many scenarios: Maybe I couldn't think of any other approach. Maybe I thought of 5 of them and had reasons to select this one. Do you want me to list a lengthy response examining each of the approaches I thought off with a justification on why I didn't select it? I shouldn't have to do that much work.
It's much easier to respond if you proposed something and I'll address that. My response may well be "Thanks - I hadn't thought of that approach and agree with you."
As a general rule, do not ask questions that could require a lengthy answer unless you are willing to offer something on your side. It requires almost no effort on your part to ask a question, but it always requires effort - sometimes a lot - for the other person to formulate a non-dismissive response. Don't make the other person work hard without good reason. If you're going to ask a question, put in the effort to explain your perspective. Never, ever, assume that the author has an obligation to respond to you.
The Socratic approach, frankly, is a poor approach. It works in a very limited arena (e.g. teacher/student), and code reviews are not part of that arena. I learned this the hard way over many years where I'd ask very innocent questions, and occasionally someone would snap/shout at me. Only when I read the books did I understand: When asking a question, you have to make your concerns clear at the outset. You have to make the person understand why answering is a good idea. And you should never feel entitled to a response (and your words/tone/posture has to match).
Finally, as a general rule, be humble as a reviewer. Very often, the author knows more about the code or domain (or both) than you. And very often the author will have already considered your approach and has a very good reason not to go with it. It's perfectly fair game to point out that if the author had such a reason it would be good to put a comment about it as other people reading the code will share your concerns. But acting as if you know better is already sabotaging the conversation.
I would respond with "I do not understand the rationale behind the approach you picked. In my opinion approach XYZ is preferred and here are the benefits (list them all, including complexity). Did you happen to consider XYZ? If you did I'd like to hear your thoughts on the pros/cons of your approach over XYZ."
Paranoid people may read more in my response than there is, but you can't control that too much.
They didn't have my favorite one: using questions instead of statements For example, "Why did you do it this way?" instead of "Here's why you should do it a different way". (or "have you considered...", "what are the benefits of A vs B", "is this going to meet this requirement or avoid that problem")
This approach has helped me because
a) It feels less aggressive (more like you're working through it together),
b) It prods them into thinking through what they missed, and
c) It prods for information that I might be missing. I might actually discover that I'm wrong and the other person considered more information.
This approach has helped me because
a) It feels less aggressive (more like you're working through it together),
b) It prods them into thinking through what they missed, and
c) It prods for information that I might be missing. I might actually discover that I'm wrong and the other person considered more information.
In general I agree with your point. I just want to point out one particular edge case where it can be misapplied.
The one thing I always want to be careful of with questions is to make sure they don't come off condescending. One example of an anti-pattern with questions is to ask
"Why did you do <obvious mistake>?"
In that case, clearly they didn't mean to make the obvious mistake. Asking the question implies that they did it intentionally, when surely what actually happened is they overlooked the error. Instead, I just point out the mistake. "I think you forgot to check for null here." comes across way better than "Why didn't you check for null here?"
The general principle I find useful is to try to say exactly what you mean. When you spot an obvious mistake, there's not really a question to be asked. You are trying to note the mistake, so just use a statement to note the mistake. When it's less obvious, or it's a judgment call, a question is great, because that's exactly what you're trying to do: ask whether another approach might be better.
The one thing I always want to be careful of with questions is to make sure they don't come off condescending. One example of an anti-pattern with questions is to ask
"Why did you do <obvious mistake>?"
In that case, clearly they didn't mean to make the obvious mistake. Asking the question implies that they did it intentionally, when surely what actually happened is they overlooked the error. Instead, I just point out the mistake. "I think you forgot to check for null here." comes across way better than "Why didn't you check for null here?"
The general principle I find useful is to try to say exactly what you mean. When you spot an obvious mistake, there's not really a question to be asked. You are trying to note the mistake, so just use a statement to note the mistake. When it's less obvious, or it's a judgment call, a question is great, because that's exactly what you're trying to do: ask whether another approach might be better.
Few things in a professional setting annoy me more than what you're describing. I left the default date on a form once and got it back with a note that said "why did you put this date here? Today's date is XYZ."
You got me. I thought it was 2004. Thanks for correcting me, now I know it's actually 2019. Pretty shocking that I'm actually 15 years older than I thought! I guess I'm a huge moron!
You got me. I thought it was 2004. Thanks for correcting me, now I know it's actually 2019. Pretty shocking that I'm actually 15 years older than I thought! I guess I'm a huge moron!
For the cases where a question would be too condescending, or things that are mostly a matter of taste, I phrase them like you did: using the words 'I think'. Still leaves open the possibility that I missed something obvious, or that it's ok to disagree with me.
I think you're right that in case of obvious mistake, like a typo, a question would sound condescending (like "I think you can't handle feedback" instead of "let's make sure we understand each other's thinking").
My variant on this tends to go:
"It looks like this <code block/method/module> will cause XYZ to happen. Is that what you intended?"
Simple example (Python):
One thing I like about this approach is that it first describes your understanding of a particular code section, which helps illuminate any assumptions we might hold (and which might be incorrect). This is a good practice generally, IMHO, with or without the followup question. A guy I used to work with would make non-judgemental comments that just summarized his understanding of what was happening in the code, and it was helpful because when the comment was off, it illustrated a way in which the code was perhaps unclear or incorrect.
"It looks like this <code block/method/module> will cause XYZ to happen. Is that what you intended?"
Simple example (Python):
def foo(x):
print(x.lower())
One thing I like about this approach is that it first describes your understanding of a particular code section, which helps illuminate any assumptions we might hold (and which might be incorrect). This is a good practice generally, IMHO, with or without the followup question. A guy I used to work with would make non-judgemental comments that just summarized his understanding of what was happening in the code, and it was helpful because when the comment was off, it illustrated a way in which the code was perhaps unclear or incorrect.
Thank you.
Of all the variants proposed in this topic, this is the only one that puts any burden whatsoever on the speaker in the first place (in a good way).
Demonstrating that you've made an actual attempt to understand what your colleague is doing is far more genuine and likely to be received as such. Unlike the mealy-mouthed carefully-phrased examples that just communicate "I care more office politics than actually ensuring this code is high quality," your approach hits a trifecta of signaling respect, actually identifying the concerning behavior of the code, and inviting all the types of productive responses:
You allow the person to graciously admit an error without particularly losing face (they can simply say "nice catch, thank you" and fix it themselves). You provide an inroad to correcting your OWN potentially false assumption -- very helpful, for example, when a junior dev whose work is being reviewed is actually in the right, but politically might be unable to demonstrate that. And your question about the mechanics opens the door to a collaborative discussion in which both sides contribute to the final codebase.
Of all the variants proposed in this topic, this is the only one that puts any burden whatsoever on the speaker in the first place (in a good way).
Demonstrating that you've made an actual attempt to understand what your colleague is doing is far more genuine and likely to be received as such. Unlike the mealy-mouthed carefully-phrased examples that just communicate "I care more office politics than actually ensuring this code is high quality," your approach hits a trifecta of signaling respect, actually identifying the concerning behavior of the code, and inviting all the types of productive responses:
You allow the person to graciously admit an error without particularly losing face (they can simply say "nice catch, thank you" and fix it themselves). You provide an inroad to correcting your OWN potentially false assumption -- very helpful, for example, when a junior dev whose work is being reviewed is actually in the right, but politically might be unable to demonstrate that. And your question about the mechanics opens the door to a collaborative discussion in which both sides contribute to the final codebase.
> Why did you do it this way?
I think that's a high load question (in terms of time and energy required to answer). It's best used sparingly. Instead point out a specific problem and supply an alternative that does not have that problem.
I think that's a high load question (in terms of time and energy required to answer). It's best used sparingly. Instead point out a specific problem and supply an alternative that does not have that problem.
I use this approach a lot too, but I’ve found that some people consider it condescending so know your audience I guess.
It's condescending if your feedback is just bike shedding. Like if you think this module should be broken out into it's own namespace it's on you to explain why as opposed to "Have you considered putting this in a namespace?" -- most likely I did, and I didn't think it was important and now you are asking me to write up why I preferred 6 of one over half a dozen of the other and in non-violent communication forms on top of it. If there is a good reason, then just say it, otherwise it's super frustrating and time consuming.
BUT, if you are going to do that, please do it in your first pass in review, don't do it in your subsequent pass after I've already responded to your feedback. This is just inconsiderate of my time.
BUT, if you are going to do that, please do it in your first pass in review, don't do it in your subsequent pass after I've already responded to your feedback. This is just inconsiderate of my time.
I follow the opposite rule. Never use a question unless you genuinely don't know the answer. For example the PR contains a bug, which was obviously not noticed by the submitter. The answer to "why did you do it this way?" is "I didn't think of all possible scenarios". The answer to "What would happen if blah happens?" is "Well shit, clearly everything will break!" . The answer to "Have you considered that foo might be null?" is "No I god damn haven't". By asking a question you're asking the submitter to publically admit their mistake (since not answering questions on a PR is rude) and apologize for something that all programmers do naturally (writing buggy code).
Instead a proper comment is "This breaks if blah happens and foo is null" at which point the submitter just fixes the bug. It achieves a) by being a neutral statement, b) naturally by making the submitter rethink their assumptions without having to apologize for that and if the reviewer was in fact wrong and the comment is incorrect, c) would happen due to the submitter explaining why this situation can't happen (and possibly adding a code comment to that effect).
Instead a proper comment is "This breaks if blah happens and foo is null" at which point the submitter just fixes the bug. It achieves a) by being a neutral statement, b) naturally by making the submitter rethink their assumptions without having to apologize for that and if the reviewer was in fact wrong and the comment is incorrect, c) would happen due to the submitter explaining why this situation can't happen (and possibly adding a code comment to that effect).
Agreed. I also find myself just attaching a question mark to most comments unless I'm very sure of them.
E.g. "Add a comment about foo bar?"
E.g. "Add a comment about foo bar?"
I love this approach because it starts from the position that the code writer knows what they are doing, instead of assuming they don't.
I think this is just generally good advice anywhere when you find yourself providing feedback or constructive criticism, especially if there's a potential for a power dynamic. You absolutely nailed all the right reasons why it's effective, and I do it all the time.
The only real downside I run into is that sometimes the questions get answered too earnestly, and I end up with the person on the other end helpfully trying to educate me rather than engaging the question or issue directly. In every case, though, I could and should have phrased myself more clearly. It's a constant learning process, but the system works.
The only real downside I run into is that sometimes the questions get answered too earnestly, and I end up with the person on the other end helpfully trying to educate me rather than engaging the question or issue directly. In every case, though, I could and should have phrased myself more clearly. It's a constant learning process, but the system works.
BeetleB makes some great points in this thread. Answering some questions is complicated if you want to be thoughtful, it places a big burden on the author.
The most disrespectful part of the "Don't" examples are that they are vague about their concerns while simultaneously expecting a concession from the other party in the review. They say, "your code/approach is wrong, but I don't feel like explaining why I think it's wrong".
That's what sets up the context for the you-s to be interpreted as jerky. "You" can certainly be used in a non-disrespectful way, but not when the rest of the comment it is attached to is disrespectful.
Code reviews are like small debates, and successful debates do involve courtesy, patience, and arguing in good faith, which are also basic life skills.
That's what sets up the context for the you-s to be interpreted as jerky. "You" can certainly be used in a non-disrespectful way, but not when the rest of the comment it is attached to is disrespectful.
Code reviews are like small debates, and successful debates do involve courtesy, patience, and arguing in good faith, which are also basic life skills.
On the spectrum of best to worst way to do code reviews - pair programming is the best. Writing comments, no matter how respectful, back and forth is the worst.
When I do code reviews I split the difference and "pair code review".
My preferred method:
I start by realizing the author has had more time to really think it through than I did, give them the benefit of the doubt, and realize I have some faulty assumptions up front (the classic "Building twitter can't be that hard!" type assumptions we see a lot on HN).
Then I read through the code on my own but don't post any comments, or only post "uncontroversial comments" (spelling, forgot to scope a query, etc)
Then it is time to pair. I start a screen share/video call with the author and let the author take the lead and walk me through it. They tend to find most issues on their own just by talking it out (rubber duck code reviews). Usually the author posts comments on the PR while doing this, not me. When I see a problem I can guide them to the problem with a few prompts and we can discuss it and resolve it right there, without any back and forth in pr comments.
Only after doing all this would I consider asking the author to make major changes or re-dos.
Sometimes I'm in a hurry and I don't do all this, and it often leads to hurt feels, pr flame wars, etc.
When I do code reviews I split the difference and "pair code review".
My preferred method:
I start by realizing the author has had more time to really think it through than I did, give them the benefit of the doubt, and realize I have some faulty assumptions up front (the classic "Building twitter can't be that hard!" type assumptions we see a lot on HN).
Then I read through the code on my own but don't post any comments, or only post "uncontroversial comments" (spelling, forgot to scope a query, etc)
Then it is time to pair. I start a screen share/video call with the author and let the author take the lead and walk me through it. They tend to find most issues on their own just by talking it out (rubber duck code reviews). Usually the author posts comments on the PR while doing this, not me. When I see a problem I can guide them to the problem with a few prompts and we can discuss it and resolve it right there, without any back and forth in pr comments.
Only after doing all this would I consider asking the author to make major changes or re-dos.
Sometimes I'm in a hurry and I don't do all this, and it often leads to hurt feels, pr flame wars, etc.
For all but the simplest and most obvious issues, talking through the potential problem is also faster than repeated back-and-forth commentary. I'll still write the comments, but that's more for me to get the thoughts on the issue down on paper than to ensure that my thoughts get communicated accurately and acted upon.
> Sometimes I'm in a hurry and I don't do all this, and it often leads to hurt feels, pr flame wars, etc.
Have you considered framing all your suggestions in the form of questions?
Eg "have you considered doing this using <X>, because the style guide suggests it/it might be faster/etc"
That way if they have already considered it, they can just respond yes, and give the reason they didn't do it. And if they haven't considered it, they can think about it and make a more informed decision.
Maybe this is a very passive approach but it seems to work for me. Inspired by a sci-fi writing workshop I took and the tips they gave on how to critique without turning it into a confrontation.
Have you considered framing all your suggestions in the form of questions?
Eg "have you considered doing this using <X>, because the style guide suggests it/it might be faster/etc"
That way if they have already considered it, they can just respond yes, and give the reason they didn't do it. And if they haven't considered it, they can think about it and make a more informed decision.
Maybe this is a very passive approach but it seems to work for me. Inspired by a sci-fi writing workshop I took and the tips they gave on how to critique without turning it into a confrontation.
Reframing statements as a question is less effective than training yourself to actually ask real questions. If you're 99% convinced that something is wrong, it's no use asking a question like "have you considered not leaving a print statement in the middle of your code?", but if you're only 90% sure then there's already a question you need to ask.
This may or may not be what you suggested, sorry if it's redundant.
This may or may not be what you suggested, sorry if it's redundant.
I try. I never say "you". I always "I think" or "I believe", and almost always use questions. I also try and stay fairly terse out of respect for their time and to avoid "talking down" to them. But I think sometimes that comes across wrong. As soon as I sense hurt feelings I jump on a call though.
My best code review experiences, bar none, were at a company that did all code reviews side-by-side at our desks. This was before the days of online code review tools like those built into github - you physically pulled your chair next to the submitters desk and went through the review face-to-face while they showed the diff on their screen.
There were times this was cumbersome and intrusive. I tend to make many small changes and I remember a few of my co-workers being frustrated at the frequency I asked for reviews. I remember myself feeling frustrated by the interruption to my "code flow" on occasions. But the benefit of the conversation that happened in those reviews far outweighs any negative I remember.
Now that people are socialized to the asynchronous online review process I don't think I can imagine people going back to this synchronous and time consuming process.
There were times this was cumbersome and intrusive. I tend to make many small changes and I remember a few of my co-workers being frustrated at the frequency I asked for reviews. I remember myself feeling frustrated by the interruption to my "code flow" on occasions. But the benefit of the conversation that happened in those reviews far outweighs any negative I remember.
Now that people are socialized to the asynchronous online review process I don't think I can imagine people going back to this synchronous and time consuming process.
> DO: Assume competence.
The code reviews I've had the most trouble with are the ones where I can't. Blatant copy/pasting, 20 blank lines checked in, unrelated configuration changes included, one-line fixes implemented via complex logic, zero git hygiene/cleanup, zero self-review of the code review before assigning it...
If I leave a comment like:
> This logic is repeated in both view controllers. Should be DRY.
I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons.
Far more likely is that they needed it in 3 places and thus pressed CMD-V 3 times.
The code reviews I've had the most trouble with are the ones where I can't. Blatant copy/pasting, 20 blank lines checked in, unrelated configuration changes included, one-line fixes implemented via complex logic, zero git hygiene/cleanup, zero self-review of the code review before assigning it...
If I leave a comment like:
> This logic is repeated in both view controllers. Should be DRY.
I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons.
Far more likely is that they needed it in 3 places and thus pressed CMD-V 3 times.
> This logic is repeated in both view controllers. Should be DRY.
It may well be a good idea not to be DRY.
https://news.ycombinator.com/item?id=20395901
It may well be a good idea not to be DRY.
https://news.ycombinator.com/item?id=20395901
> I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons.
These might speak some to hiring, training, tooling, and team culture.
First, one of these (blank lines) seem to be self correcting with a code style linter (not linting creates PR noise, increases and reviewer and coding frustration).
Second, blatant copy pasting should be difficult, as surely the context is different - thus descriptive names should often be renamed, and then at least some level of integration testing provides quick education to dry up code.
And surely, if the team is reviewing, calling out repeated or obviously copied untested code, with no linting the developer would improve rapidly otherwise tire of the rework.
First, one of these (blank lines) seem to be self correcting with a code style linter (not linting creates PR noise, increases and reviewer and coding frustration).
Second, blatant copy pasting should be difficult, as surely the context is different - thus descriptive names should often be renamed, and then at least some level of integration testing provides quick education to dry up code.
And surely, if the team is reviewing, calling out repeated or obviously copied untested code, with no linting the developer would improve rapidly otherwise tire of the rework.
I think I can live with all of those things, if only the code actually works after it is merged. But too often you merge the crap and it doesn’t even work...
That just baffles me, you’ve been writing all this code, but you never bothered to test that it actually did something?
That just baffles me, you’ve been writing all this code, but you never bothered to test that it actually did something?
Maybe it’s a cultural thing but I find many of examples given to be worded in a way I would find a bit duplicitous. I’d prefer a much more direct approach and I’d be reluctant to trust anyone using these kind of couched expressions.
See here: https://m.imgur.com/gallery/ltn3bla
See here: https://m.imgur.com/gallery/ltn3bla
Practices like this are ridiculous. Of course you should be respectful in a code review, but you should also be to the point and ask questions if you don't understand why they did something they did, even if there is a "better" way you know of. That's how people learn.
The examples they give are just bad.
The examples they give are just bad.
>Of course you should be respectful in a code review
It seems self evident but there are a lot of people who don't believe this and take a kind of perverse pride in "tearing others' code to shreds" so it's worth enshrining in policy if nothing else.
It seems self evident but there are a lot of people who don't believe this and take a kind of perverse pride in "tearing others' code to shreds" so it's worth enshrining in policy if nothing else.
Working in various companies I have seen code reviews become filled with micro-aggressions and outright nastiness. Code reviews have a purpose but that often gets clouded in personal judgements. This ends up making things like refactoring difficult, as people just don’t want to bother with the process.
I agree. Really poorly done - I'm surprised this is on the front page of HN.
Perhaps, but I'm finding that the resulting discussion surfaces a lot of wisdom, good ideas, and different perspectives.
I agree the examples are bad. An article like this would be really helpful with more and better examples.
What do you think are bad about the examples? They all seem reasonable to me.
"I don’t understand this." is a valid review comment. Of course sometimes you can expand on the comment, but sometimes it's enough.
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.)
For me (and I usually say this instead) it means "This should be explained/documented better for future readability"
> 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".
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".
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.
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.
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.
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.
Depends on your review process - if that comment acts as a trigger/marker for a face-to-face conversation then it makes sense.
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.
"lets discuss this"
And avoid putting the burden of ambiguity on the author of the original code under review.
Reviews at Google very often span timezones. Understandably, this sort of approach isn’t always ideal for optimal usage of time.
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.
"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.
“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.
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.
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?"
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?"
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.
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.
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.
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.
Code review comments at google attach to a region rather than a specific line.
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?
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?
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.)
(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.)
> 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.
"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.
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?"
"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?"
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.
As a person who prefers profanity and hyperbolic exaggeration and over-reaction when dealing with technical topics (about which no sane person would do things like threaten violence, so it becomes funny for someone you know to do so), I entirely understand the need for policies like these. I am fully aware that I am not all people or even necessarily normal. Reviews inherently involve other people, and unless you are in a closed and close-knit social group, it is very easy for people to misunderstand things and be unaware of the subtext a person might be intending to communicate with.
I don't really understand why (at least not enough that I could effectively change the situation) some folks conduct themselves as if everyone else is their best friend and knows them intimately, knowing when they are joking, etc. Is it a matter of stereotypical 'social awkwardness' that society derides intellectuals with? Or something different? I haven't personally come across people that are earnestly writing code review comments with malice, but have certainly seen some that could be seen as overly negative/aggressive/etc by an outsider (they were written in a very close-knit team amongst friends, though). I presume most all problematic comments come down to misunderstandings.
I don't really understand why (at least not enough that I could effectively change the situation) some folks conduct themselves as if everyone else is their best friend and knows them intimately, knowing when they are joking, etc. Is it a matter of stereotypical 'social awkwardness' that society derides intellectuals with? Or something different? I haven't personally come across people that are earnestly writing code review comments with malice, but have certainly seen some that could be seen as overly negative/aggressive/etc by an outsider (they were written in a very close-knit team amongst friends, though). I presume most all problematic comments come down to misunderstandings.
I’ve experienced where code reviews have worked well and where they actually have been more hurting for morale than useful. The difference between the two was that how well people knew how to work with the frameworks they were using. In the failing team the participants didn’t know what their tools and code was doing and the code reviews basically became ravings about code formatting.
Another effect were it took days to do a code review because each review had to be explained in detail, instead of have a common understanding of why the changes should work or be different.
The effectively consequences of this was that people only asked reviews from other people that either accepted everything or didn’t have the adequate knowledge about tools so the code just became worse and even less aligned with what the frameworks required.
This was recognized by the management and a huge effort were put into educating the staff so the common knowledge about the tools were raised. This was really appreciated move and, although the team still struggled at least the reviews became better.
I don’t know if just respectful reviews work if you always feel, even if the reviews are kind and helpful, as the one needing a lot of explaining to commit your code, particularly if you are on a deadline and stressed out. People get fearful and defensive if you don’t get what’s wrong with your code.
It can also be the other way around, that you get loads of questions and downright wrong reviews which then has to be defended. And this is the beginning of disagreements which could cause tensions and ultimately disrespect.
Another effect were it took days to do a code review because each review had to be explained in detail, instead of have a common understanding of why the changes should work or be different.
The effectively consequences of this was that people only asked reviews from other people that either accepted everything or didn’t have the adequate knowledge about tools so the code just became worse and even less aligned with what the frameworks required.
This was recognized by the management and a huge effort were put into educating the staff so the common knowledge about the tools were raised. This was really appreciated move and, although the team still struggled at least the reviews became better.
I don’t know if just respectful reviews work if you always feel, even if the reviews are kind and helpful, as the one needing a lot of explaining to commit your code, particularly if you are on a deadline and stressed out. People get fearful and defensive if you don’t get what’s wrong with your code.
It can also be the other way around, that you get loads of questions and downright wrong reviews which then has to be defended. And this is the beginning of disagreements which could cause tensions and ultimately disrespect.
funny, I'd add :-
DO: Avoid using DO and DON'T
DON'T: Use DO and DON'T
They feel like "Political Correctness" rules and that can backfire (not that the advice isn't good advice!), oddly enough, another google guideline posted a while back on code reviews I thought got the language better https://google.github.io/eng-practices/review/reviewer/ . It uses language like "In general, reviewers should favor..." which, to me, comes off a lot better than "DO:", I think it leads to the "better" idea of improving the way you do things and conduct yourself and the advice is giving you more tools to use during reviews, and that it feels like its always an open to further improvement and that there is a certain tolerance to not always doing it in a good way. Where the DO/DON'T approach seems to create an idea of there being a definitive way of conducting yourself which tends to irk people. Base definitive rules are the same as most legal requirements of conducting yourself in a employment situation.
DO: Avoid using DO and DON'T
DON'T: Use DO and DON'T
They feel like "Political Correctness" rules and that can backfire (not that the advice isn't good advice!), oddly enough, another google guideline posted a while back on code reviews I thought got the language better https://google.github.io/eng-practices/review/reviewer/ . It uses language like "In general, reviewers should favor..." which, to me, comes off a lot better than "DO:", I think it leads to the "better" idea of improving the way you do things and conduct yourself and the advice is giving you more tools to use during reviews, and that it feels like its always an open to further improvement and that there is a certain tolerance to not always doing it in a good way. Where the DO/DON'T approach seems to create an idea of there being a definitive way of conducting yourself which tends to irk people. Base definitive rules are the same as most legal requirements of conducting yourself in a employment situation.
I think this is particularly good point. In general, i would consider sentences in direct imperative mood as grossly inappropriate (for code reviews or anything else), unless from someone in position of legitimate power / competence.
It is interesting that they do not hesitate to use such disrespectful form in blogpost about respectful code reviews.
It is interesting that they do not hesitate to use such disrespectful form in blogpost about respectful code reviews.
I fully agree. I think the problem is that people want to talk about communicating effectively, and they just naturally mentally glom onto the CoC style of thinking, even if they don't put it there. The CoC style of correct/incorrect, do/don't, (and I'd argue a subconscious framing of good/evil), just aren't a good fit for something as complex and difficult as writing well and understanding the psychology of other people.
I think the right way to think about this is to separate communication into acceptable and unacceptable. The unacceptables fit perfectly into the CoC, these are the unimpeachably bad things you absolutely shouldn't do.
But the acceptables are a huge range that needs space to breathe. Putting advice meant to take someone's writing from good to great in the same place as where you tell people not to sexually harass each other is setting ourselves up for failure. The CoC style has inherited an implicitly punitive framework. Just like real legal codes they have no concept of a spectrum of excellence on the compliance side. You're never going to get pulled over by a cop and thanked for driving with exceptional courtesy. The law is binary, you're either speeding or you aren't, you're either complying with it or you aren't, the only spectrum exists on the no compliance side.
Really, the acceptables should be refactored out of the CoC, both in not explicitly being written into it, and not implicitly inheriting the same style. The unwritten assumption in the legal system is that human behavior is governed by outside pressures like punishment and reward, and because there is no outside pressure to motivate varying levels of good behavior, and behavior is only motivated by externals, good behavior is defined only as everything that's not worthy of punishment. But I think a lot of people genuinely intrinsically care about the quality of their communication, just like the quality of any other part of their work. There are people who want to teach, and people who want to learn, and it would be to the benefit of everyone if they had their own place to do it.
I think the right way to think about this is to separate communication into acceptable and unacceptable. The unacceptables fit perfectly into the CoC, these are the unimpeachably bad things you absolutely shouldn't do.
But the acceptables are a huge range that needs space to breathe. Putting advice meant to take someone's writing from good to great in the same place as where you tell people not to sexually harass each other is setting ourselves up for failure. The CoC style has inherited an implicitly punitive framework. Just like real legal codes they have no concept of a spectrum of excellence on the compliance side. You're never going to get pulled over by a cop and thanked for driving with exceptional courtesy. The law is binary, you're either speeding or you aren't, you're either complying with it or you aren't, the only spectrum exists on the no compliance side.
Really, the acceptables should be refactored out of the CoC, both in not explicitly being written into it, and not implicitly inheriting the same style. The unwritten assumption in the legal system is that human behavior is governed by outside pressures like punishment and reward, and because there is no outside pressure to motivate varying levels of good behavior, and behavior is only motivated by externals, good behavior is defined only as everything that's not worthy of punishment. But I think a lot of people genuinely intrinsically care about the quality of their communication, just like the quality of any other part of their work. There are people who want to teach, and people who want to learn, and it would be to the benefit of everyone if they had their own place to do it.
How do you deal with people who take _any_ critique of their code as a personal affront? I'm working with one of such people right now, for the first time in my 25 years in the industry. I'm a consultant, and he's an FTE. I'm considerably more experienced and senior and management knows it. The guy's PRs have been sitting in the queue for over a month now because there's no way I'll let him submit his shit code to master, and he just won't clean it up. That's not how I phrased it in the review obviously. I explained how the code could be improved (and why it needs to be improved) and gave concrete suggestions, using the most diplomatic language I could come up with. I explained to his manager why the PR is stuck. His manager is conflict-averse, so he won't push the guy, knowing how easily he gets offended.
I mean ultimately, my contract ends in January, so I could just merge the PR and say "après moi, le déluge", but in case I encounter such people again in the future, what do people usually do?
I mean ultimately, my contract ends in January, so I could just merge the PR and say "après moi, le déluge", but in case I encounter such people again in the future, what do people usually do?
Go sit next to him with a cup of coffee tomorrow morning, and fix the PRs right there. Say "Let's do some quick pair programming and close these lingering PRs real quick."
He should appreciate the initiative, and communication is much more effective and resonant in person.
He should appreciate the initiative, and communication is much more effective and resonant in person.
Thanks for the suggestion, but judging by my other interactions with the guy that'd be catastrophic. It goes without saying that we've already communicated in person about this.
I find really hard about code reviews is how people have different styles. Some people writing more procedural, some more functional, some want to use Actor model, others like layers, others like having loads of dependencies injected. What are you supposed to do? Haven't even started on the plethora of libraries and frameworks.
Learn to be flexible. I've been writing and reading code for 30 years. You'll fight a losing battle if you try to enforce much more than "code must pass tests and work in production."
Third party dependencies are generally toxic to the long term health of your application. So I push back hard against adding new frameworks or libraries unless they really carry their weight.
But if someone wants to use a new language feature we don't use often, or try out a new pattern I try and give it a chance. If you shut down everything you're not giving your people room to grow their skills, and they'll move on.
But if someone wants to use a new language feature we don't use often, or try out a new pattern I try and give it a chance. If you shut down everything you're not giving your people room to grow their skills, and they'll move on.
The style isn't as important unless it impacts the reviewer's ability to read and understand the code, or the maintainability of the code.
No mention of providing positive comments? e.g. "Very readable. I think this abstraction will save us a lot of time in the future too.". It's tiring to work days on a commit only to get a bunch of fixes back with no understanding of what people consider good tradeoffs and high quality for future commits.
And then your commit is merged, and all your great code is going to be used.
While I understand wanting to be praised to some extend, wanting to be praised for doing your job still sounds a bit off to me.
That said, getting multiple rounds of fixes bothers me to no end, so I guess it’s always merge after the first round of fixes.
While I understand wanting to be praised to some extend, wanting to be praised for doing your job still sounds a bit off to me.
That said, getting multiple rounds of fixes bothers me to no end, so I guess it’s always merge after the first round of fixes.
> And then your commit is merged, and all your great code is going to be used.
It's more about understanding what qualities in your code your team appreciates. There's a big spectrum in code that gets accepted for being amazing and code that only gets barely accepted too.
> While I understand wanting to be praised to some extend, wanting to be praised for doing your job still sounds a bit off to me.
Do you think it's weird that people in other industries might say "great work!" from time to time? Seems like a basic part of social interaction to me. I don't want to work with robots, especially the people in code review discussions that say everyone should just have a thicker skin.
Only getting negative comments back from people and "pull request accepted" notifications is a cold environment to work in, especially if you work remote.
It's more about understanding what qualities in your code your team appreciates. There's a big spectrum in code that gets accepted for being amazing and code that only gets barely accepted too.
> While I understand wanting to be praised to some extend, wanting to be praised for doing your job still sounds a bit off to me.
Do you think it's weird that people in other industries might say "great work!" from time to time? Seems like a basic part of social interaction to me. I don't want to work with robots, especially the people in code review discussions that say everyone should just have a thicker skin.
Only getting negative comments back from people and "pull request accepted" notifications is a cold environment to work in, especially if you work remote.
> Do you think it's weird that people in other industries might say "great work!" from time to time?
Not at all, it’s just that I personally don’t say, or expect, a “good work” message unless I’m honestly impressed by what was written.
I’ve received that once or twice, and given it a few more times (in 10 years).
But just saying good work to keep someone’s motivation up, that is something I would have to consciously think about.
Not at all, it’s just that I personally don’t say, or expect, a “good work” message unless I’m honestly impressed by what was written.
I’ve received that once or twice, and given it a few more times (in 10 years).
But just saying good work to keep someone’s motivation up, that is something I would have to consciously think about.
I often phrase code review comments as questions with specific goals in mind, such as "Would this be simpler/faster/clearer if [refactor idea]?" or "How about [alternative]?" This makes it clear that I, as a reviewer, do not have all the context that the author does, and that it's a good chance they did consider those ideas but rejected them based on previous experience. This may be as simple as a broken third party thing that they had to work around, or prioritizing differently.
The only thing this misses is code style, but holy crap automated code formatting tools are such enormous time savers for reviews. Black + isort on Python code and Prettier for everything else makes code reviews easily three times faster.
The only thing this misses is code style, but holy crap automated code formatting tools are such enormous time savers for reviews. Black + isort on Python code and Prettier for everything else makes code reviews easily three times faster.
These are all just basic principles of being a working professional, if any of this is non-obvious you have a culture problem not a code review problem.
I'm a manager and you'd be surprised at how many engineers (and people in general) can take issue with wording in communications. And all it takes is one person. More times than not that person is an overachiever - one who wants everyone else to be perfect, or one who wants to always be perfect. So much time is wasted sorting out miscommunication.
Just read through the other comments here regarding feelings for each of the comment types.
Personally I have a thick skin. Even harsh, direct verbal criticism doesn't faze me. I'll laugh in your face and tell you to get the fuck out of mine.
Just read through the other comments here regarding feelings for each of the comment types.
Personally I have a thick skin. Even harsh, direct verbal criticism doesn't faze me. I'll laugh in your face and tell you to get the fuck out of mine.
>Personally I have a thick skin. Even harsh, direct verbal criticism doesn't faze me. I'll laugh in your face and tell you to get the fuck out of mine.
I like this attitude. How do you develop this. I am exact opposite of you what you said. I get depressed for the slightest criticism. I want to learn how to develop thick skin. I think it arises from my need to impress everyone around me and my need for their approval.
I like this attitude. How do you develop this. I am exact opposite of you what you said. I get depressed for the slightest criticism. I want to learn how to develop thick skin. I think it arises from my need to impress everyone around me and my need for their approval.
Your value as a human being isn't dependent on your coding ability. It sounds stupid and trite, but it's true. A lot of these types of insecurities arise because we allow things access to our self worth that we shouldn't. Letting other people's opinions affect your core self worth is like giving them root access to your computer and letting them write directly to /
And I think in this analogy having thick skin is kind of like a firewall, it'll keep things out, but it's often just security theatre.
It makes sense to give other people access to your computer. The world is boring as a singleplayer game, but how much access is the right amount? It's worth thinking deeply about where your self worth is stored and who and what in your life has r/w privileges.
Also, this may sound really out there, but if you don't have a good basis for self worth/self love, but if you can figure out the right way to frame it, consider asking your parents or other close friends why they love you. It's a very on the spot kind of question so don't put any value on if they don't answer, but I asked my mom that once when I was in a really dark place(and we don't have a very good relationship) and I was really blown away by her answer to the question. She straightforwardly listed all the positive qualities she admired in me and asking her that was probably one of the best decisions I ever made. It really improved our relationship and probably forever positively impacted my view of myself.
And I think in this analogy having thick skin is kind of like a firewall, it'll keep things out, but it's often just security theatre.
It makes sense to give other people access to your computer. The world is boring as a singleplayer game, but how much access is the right amount? It's worth thinking deeply about where your self worth is stored and who and what in your life has r/w privileges.
Also, this may sound really out there, but if you don't have a good basis for self worth/self love, but if you can figure out the right way to frame it, consider asking your parents or other close friends why they love you. It's a very on the spot kind of question so don't put any value on if they don't answer, but I asked my mom that once when I was in a really dark place(and we don't have a very good relationship) and I was really blown away by her answer to the question. She straightforwardly listed all the positive qualities she admired in me and asking her that was probably one of the best decisions I ever made. It really improved our relationship and probably forever positively impacted my view of myself.
I see what you're saying, but "I'll laugh in your face and tell you to get the fuck out of mine" is usually a very poor way to respond to criticism in the workplace so you shouldn't necessarily aspire to it.
Unless you're working in a frathouse, it's an incredibly poor way to deal with other people in a workplace.
This rubs many people the wrong way, and you'll quickly find that most of them will not go out of their way to help someone who acts like an asshole. Having people not want to volunteer their assistance to solve your problems is not great for your career trajectory.
This rubs many people the wrong way, and you'll quickly find that most of them will not go out of their way to help someone who acts like an asshole. Having people not want to volunteer their assistance to solve your problems is not great for your career trajectory.
I tend to disagree. Solving conflicts with this personality type is often easier because it will stay on topic and you can still share a coffee afterwards.
Many people have been raised in a social environment, where that sort of behavior is considered hostile and disrespectful. If you're laughing in someone's face, they have no idea of whether or not you think their work is shit, or they are shit.
The OP believes that it is not hostile or disrespectful.
If neither group changes their behavior, he's going to be the one who lose out in this exchange. He can feel he is in the right, he may even be in the right, but that's a bit of a questionable hill to die on.
The OP believes that it is not hostile or disrespectful.
If neither group changes their behavior, he's going to be the one who lose out in this exchange. He can feel he is in the right, he may even be in the right, but that's a bit of a questionable hill to die on.
> More times than not that person is an overachiever - one who wants everyone else to be perfect, or one who wants to always be perfect.
This used to be me. For the early stages of my career, I perhaps was the smartest person / best programming in the room, and I definitely thought so. Perhaps because of this, I had very little patience for people who made mistakes I didn't think I would have.
As my career has progressed, I've managed to get jobs where my coworkers are increasingly capable. And truth by told, I've become a little less sharp over time as my brain has aged. Humbling experiences started to pile up.
Only because of those embarrassments did I gain empathy for the limitations / mistakes made by other software developers. And I believe that empathy was what helped me to curb my haughty, critical communication style.
+1 for the wisdom that can come with age. -1 for the pain I caused in others prior to that.
This used to be me. For the early stages of my career, I perhaps was the smartest person / best programming in the room, and I definitely thought so. Perhaps because of this, I had very little patience for people who made mistakes I didn't think I would have.
As my career has progressed, I've managed to get jobs where my coworkers are increasingly capable. And truth by told, I've become a little less sharp over time as my brain has aged. Humbling experiences started to pile up.
Only because of those embarrassments did I gain empathy for the limitations / mistakes made by other software developers. And I believe that empathy was what helped me to curb my haughty, critical communication style.
+1 for the wisdom that can come with age. -1 for the pain I caused in others prior to that.
I disagree. Your comment suggest that it's obvious to write this way, while I would argue, if it would be obvious for everyone, you wouldn't even need to write these down.
Also it implies, that it's obvious, and if I didn't knew these rules exist I'm not a professional and may be a cause of a culture problem.
The thing is, most of the examples aren't really rude, they just communicate that the one who wrote the code is doing something wrong, while the reason you do code reviews is to improve the code. But it's not easy to communicate without hurting peoples feelings even when you didn't mean to. Even between cultures there is a lot of difference in how direct you should be and what is considered rude.
Also it implies, that it's obvious, and if I didn't knew these rules exist I'm not a professional and may be a cause of a culture problem.
The thing is, most of the examples aren't really rude, they just communicate that the one who wrote the code is doing something wrong, while the reason you do code reviews is to improve the code. But it's not easy to communicate without hurting peoples feelings even when you didn't mean to. Even between cultures there is a lot of difference in how direct you should be and what is considered rude.
It is helpful to have things like these written down, read and discussed even if they seem obvious. Nobody is perfect.
My personal feedback and discussion quality can easily vary depending on circumstances. Also there are many differences in style both preferred and applied. Communication can be very difficult between polar opposites.
Easy to follow guidelines can help with these things.
e: typo
My personal feedback and discussion quality can easily vary depending on circumstances. Also there are many differences in style both preferred and applied. Communication can be very difficult between polar opposites.
Easy to follow guidelines can help with these things.
e: typo
I can see how some of these could be non-obvious for neuroatypicals though. Heck, emotional nuance of written conversation is hard to grasp even for the average person.
I think this requires training/awareness, I don’t think it’s automatically bad culture or bad people.
I think this requires training/awareness, I don’t think it’s automatically bad culture or bad people.
I had a coworker with aspergers, and he was absolutely fine at code reviews, because he was never even tempted to play the status games that everyone else constantly had to check themselves for. You could really trust his reviews to be about the code and not the person, and he never fell into the trap of trying to prove how smart he was.
I don't know if this is typical, but it shattered some of my preconceptions.
I don't know if this is typical, but it shattered some of my preconceptions.
> If this is an optimization, can you please add comments?
That’s a snarky one.
That’s a snarky one.
CMU's RPP: https://www.cs.cmu.edu/~weigand/staff/
Was surprisingly effective. Probably something to it - that department has maintained lasting productivity longer than the lifetimes of many companies.
Was surprisingly effective. Probably something to it - that department has maintained lasting productivity longer than the lifetimes of many companies.
Good guidelines for the most part. I don't really see anything wrong with "Why are you using this approach? You’re adding unnecessary complexity" though.
I just want to see more examples. Every situation is different and people should adapt their responses to different personalities. Real examples taken from real code reviews with contexts (chat logs and whatnot) and its aftermath would be fun. Maybe I can dig some GitHub discussions and list "funny/great/bad code reviews that went great/awry?" Also, as a thought experiment, it would be fun to think what if Gordon Ramsey did code review.
>what if Gordon Ramsey did code review
Just read the Linux or OpenBSD developer mailing lists. Linus puts the fear of God into Linux developers. Theo puts the fear of Theo into OpenBSD developers.
Just read the Linux or OpenBSD developer mailing lists. Linus puts the fear of God into Linux developers. Theo puts the fear of Theo into OpenBSD developers.
My biggest concern with this sort of thing, and it's a general criticism, is that the suggested way always comes across as very weak, roundabout, and doesn't ever convey the same kind of sentiment as the language that is discouraged.
You can be direct and unambiguous without being an asshole, but the suggestion usually is to use some artificial passive-voice construction that no real person ever speaks like, and which completely blunts the criticism.
You can be direct and unambiguous without being an asshole, but the suggestion usually is to use some artificial passive-voice construction that no real person ever speaks like, and which completely blunts the criticism.
so which of the following things are the author claiming with the ==?
- all respectful reviews are useful - all useful reviews are respectful - all disrespectful reviews are useless - all useless reviews are disrespectful
Which of these things would the author defend when challenged, and which are they gesturing vaguely at and hope to slip under the radar?
- all respectful reviews are useful - all useful reviews are respectful - all disrespectful reviews are useless - all useless reviews are disrespectful
Which of these things would the author defend when challenged, and which are they gesturing vaguely at and hope to slip under the radar?
Before criticizing something, ask yourself "Is it wrong? Or is just not the way you would do it?"
Ultimately, any rules you create tend to break down, as there is always an edge case personality that will dislike the way you worded something.
The best advice I have seen applies to regular conversation as well as code reviews:
When you see something that is confusing, incorrect, or debatable, take a stance of genuinely attempting to understand the other person's mindset before offering feedback.
--
For example, asking questions in a respectful manner such as:
"Would you mind elaborating on why you chose this pattern of implementation?"
Will result in a positive conversation while also having them document in the review their mindset for posterity and further clarity of discussion.
--
From there, constructive conversation and asking if they're interested in feedback/alternative approaches can happen.
This way you maintain respect and show an honest attempt to understand others with every comment and suggestion you make to a PR.
--
tldr; ask questions to gain perspective before you start dictating to others how things should be done.
The best advice I have seen applies to regular conversation as well as code reviews:
When you see something that is confusing, incorrect, or debatable, take a stance of genuinely attempting to understand the other person's mindset before offering feedback.
--
For example, asking questions in a respectful manner such as:
"Would you mind elaborating on why you chose this pattern of implementation?"
Will result in a positive conversation while also having them document in the review their mindset for posterity and further clarity of discussion.
--
From there, constructive conversation and asking if they're interested in feedback/alternative approaches can happen.
This way you maintain respect and show an honest attempt to understand others with every comment and suggestion you make to a PR.
--
tldr; ask questions to gain perspective before you start dictating to others how things should be done.
All of the examples of dos and don'ts in this article recommend obtuse, indirect, and unclear baroque language over clear and straightforward language.
I think it's worth noting that the three authors of the article are all women. This is an explicit rejection of the "Lead Developers are your gods, and if you can't accept their vile/vulgar/abusive takedowns during code reviews as sage wisdom, you need to grow thicker skin" mentality that is particularly repelling to women in the tech community.
Those of us who got into programming BECAUSE of the famous ‘characters’ of open source and their ranty comments find it quite repellent to have codes of conduct and women in the position of cultural politeness enforcers. Even using an emoji/joke might be wrong? Really? The idea that women are uniquely invested in this sort of stuff only damages my perception as a female programmer. Who’s going to want to joke around with me or review my code if I’m the fun police?
You missed the point. The point is a person with power over you needs to be respectful of that power. Your boss is never going to be as buddy-buddy with you as a coworker, because your boss has a lot more say over your pay and whether or not you're going to be fired.
The famous "characters" of open source are notorious, not famous. No one looks at a Linus rant and says "Wow I want to try my hand at adding a patch for Linux!". No, they say "Wow, I want to have that kind of power!"
That's just the rub, isn't it, you don't get to decide that we have to be hazed because you want the potential to hold that power of us.
The famous "characters" of open source are notorious, not famous. No one looks at a Linus rant and says "Wow I want to try my hand at adding a patch for Linux!". No, they say "Wow, I want to have that kind of power!"
That's just the rub, isn't it, you don't get to decide that we have to be hazed because you want the potential to hold that power of us.
> The famous "characters" of open source are notorious, not famous. No one looks at a Linus rant and says "Wow I want to try my hand at adding a patch for Linux!". No, they say "Wow, I want to have that kind of power!"
This is where the disconnect is, I think. You stated this very confidently, presumably because it seems obvious to you that everyone would feel this way. But let me assure you, there are few things I would enjoy more as a programmer than having Linus critique my code, including the parts where he swears at me and calls me a moron who shouldn't be allowed within 50 feet of a computer. I had a couple different professors whose feedback on my work was often Linus-esque, and that's part of the reason why they were some of my favorite teachers. I can also assure you I am very much not alone in feeling this way.
Now, this doesn't mean that allowing Linus style communication would be a good idea in most work environments and I wouldn't advocate for it, but your comment shows a pretty concerning lack of appreciation for the fact that a lot of people are wired very differently than you are.
Codes of conduct like the one being laid out in this article are often seen by people like me as tyrannical - behave and interact in the way that we personally like best, or be cast out. So naturally, I work as hard as I can to prevent them from being put in place, and undermine them if and when they are. An attempt by the "woke" crowd to meet us thicker skinned types (for lack of a better way to describe it) is something I'd welcome with open arms, but nobody seems much interested in that.
This is where the disconnect is, I think. You stated this very confidently, presumably because it seems obvious to you that everyone would feel this way. But let me assure you, there are few things I would enjoy more as a programmer than having Linus critique my code, including the parts where he swears at me and calls me a moron who shouldn't be allowed within 50 feet of a computer. I had a couple different professors whose feedback on my work was often Linus-esque, and that's part of the reason why they were some of my favorite teachers. I can also assure you I am very much not alone in feeling this way.
Now, this doesn't mean that allowing Linus style communication would be a good idea in most work environments and I wouldn't advocate for it, but your comment shows a pretty concerning lack of appreciation for the fact that a lot of people are wired very differently than you are.
Codes of conduct like the one being laid out in this article are often seen by people like me as tyrannical - behave and interact in the way that we personally like best, or be cast out. So naturally, I work as hard as I can to prevent them from being put in place, and undermine them if and when they are. An attempt by the "woke" crowd to meet us thicker skinned types (for lack of a better way to describe it) is something I'd welcome with open arms, but nobody seems much interested in that.
> No one looks at a Linus rant and says "Wow I want to try my hand at adding a patch for Linux!".
People who think it would be awesome to get their code past Linus, and at least very funny and instructive to have their code shat on by Linus, do :) Some of us just don’t care about or even like ‘hazing’. You know, cause once you show you can take the hazing, you’re in the group.
Yeah I don’t want to be, like, epically pwned by my boss on a code review at work, but neither do I want my coworkers believing that I may find a smiley emoticon or “I don’t understand this” to constitute some sort of hostile environment.
People who think it would be awesome to get their code past Linus, and at least very funny and instructive to have their code shat on by Linus, do :) Some of us just don’t care about or even like ‘hazing’. You know, cause once you show you can take the hazing, you’re in the group.
Yeah I don’t want to be, like, epically pwned by my boss on a code review at work, but neither do I want my coworkers believing that I may find a smiley emoticon or “I don’t understand this” to constitute some sort of hostile environment.
What is the technical advantage of being intentionally abrasive?
It has the drawbacks of ostracizing those who are less confident with English and would prefer direct communication, the potential of being misunderstood and intimidating those who would prefer a more professional approach. If it cannot make the code better then it is poor engineering practice.
It has the drawbacks of ostracizing those who are less confident with English and would prefer direct communication, the potential of being misunderstood and intimidating those who would prefer a more professional approach. If it cannot make the code better then it is poor engineering practice.
It's kind of odd to ask this question in response to a comment that already specified two possible advantages. Namely:
1. It sets up a challenge that incentivizes people to try to contribute; they "think it would be awesome to get their code past Linus"
2. It serves a form of initiation ritual (a hazing) to give people a sense of belonging to the group
(I'm not sure I personally buy point 2, because as far as I know Linus is nice to newbies.)
Beyond that, I can list plenty more:
3. It builds trust in your sincerity. If you openly bitch about everything that pisses you off, then when you say that something is decent work, people know you mean it.
4. As a direct consequence of point 3, it makes the environment much more welcoming for people with scrupulosity complexes, who might otherwise worry that they're just being a nuisance who everyone would rather not have around (but is too polite to shoo away)
5. It drives away hyper-PC social-justicey types who would otherwise impose CoCs and disrupt everything with conflict and hatred.
6. It sets a tone in which people feel free to speak freely, which sets people at ease who would otherwise be anxious about accidentally giving offence (perhaps because they have poor English skills, perhaps because they're neuroatypical in some way that affects their social skills).
Are there downsides too? Sure. But this is just the age-old set of tradeoffs between having a politically correct space and a politically incorrect space; different people will feel alienated (or be rejected by the group) in each of them. For me, anti-PC spaces are home; they're where I feel comfortable and safe and able to contribute, while PC / intersectional feminist / social justice spaces feel oppressive. I guess silveroriole feels the same way.
1. It sets up a challenge that incentivizes people to try to contribute; they "think it would be awesome to get their code past Linus"
2. It serves a form of initiation ritual (a hazing) to give people a sense of belonging to the group
(I'm not sure I personally buy point 2, because as far as I know Linus is nice to newbies.)
Beyond that, I can list plenty more:
3. It builds trust in your sincerity. If you openly bitch about everything that pisses you off, then when you say that something is decent work, people know you mean it.
4. As a direct consequence of point 3, it makes the environment much more welcoming for people with scrupulosity complexes, who might otherwise worry that they're just being a nuisance who everyone would rather not have around (but is too polite to shoo away)
5. It drives away hyper-PC social-justicey types who would otherwise impose CoCs and disrupt everything with conflict and hatred.
6. It sets a tone in which people feel free to speak freely, which sets people at ease who would otherwise be anxious about accidentally giving offence (perhaps because they have poor English skills, perhaps because they're neuroatypical in some way that affects their social skills).
Are there downsides too? Sure. But this is just the age-old set of tradeoffs between having a politically correct space and a politically incorrect space; different people will feel alienated (or be rejected by the group) in each of them. For me, anti-PC spaces are home; they're where I feel comfortable and safe and able to contribute, while PC / intersectional feminist / social justice spaces feel oppressive. I guess silveroriole feels the same way.
It sounds like you have a lot of triggers and do not do well when confronted with opinions that contradict your own. Many people prefer a less emotional environment but if you need to be surrounded by like-minded people and cannot put your personal feelings aside for the benefit of the product then you have the right to pursue a comfortable environment.
At this point, it doesn’t really matter what you (or anyone, for that matter) personally want any more, because the company will have regulated acceptable behavior based on the snowflakes that take offense at literally everything.
> No one looks at a Linus rant and says "Wow I want to try my hand at adding a patch for Linux!".
He probably just thinks the kernel itself is more important than the people hacking on it. He's probably right.
He probably just thinks the kernel itself is more important than the people hacking on it. He's probably right.
When these sorts of issues come up people tend to argue that learning to speak respectfully is a huge imposition. It's hard to change, but it's a really, really useful skill to learn. It makes everything go more smoothly.
It's totally possible to joke and have fun, just don't... attack people and their ideas. Try not to blame. It's actually very simple in practice and isn't very far reaching at all.
You can still have fun, you can still joke around, but you have to realize if you attack people it's going to create a hostile environment for a lot of people, men and women included.
It's totally possible to joke and have fun, just don't... attack people and their ideas. Try not to blame. It's actually very simple in practice and isn't very far reaching at all.
You can still have fun, you can still joke around, but you have to realize if you attack people it's going to create a hostile environment for a lot of people, men and women included.
The problem is that respectful speech is itself a widely varying cultural standard.
For example, one problem I consistently face is with interruptions. In my native culture, interruptions are entirely normal outside of formal speech. I try to tone this down in the workplace to accommodate other people, and similarly, most people try to understand that I mean well when my lifetime of cultural baggage slips through. But I have to be incredibly careful with my speech patterns around "respect and inclusivity" people, because they always interpret my interruptions as intolerable and deliberate rudeness.
For example, one problem I consistently face is with interruptions. In my native culture, interruptions are entirely normal outside of formal speech. I try to tone this down in the workplace to accommodate other people, and similarly, most people try to understand that I mean well when my lifetime of cultural baggage slips through. But I have to be incredibly careful with my speech patterns around "respect and inclusivity" people, because they always interpret my interruptions as intolerable and deliberate rudeness.
> find it quite repellent to have... women in the position of cultural politeness enforcers.
Wait, what? I assume you mean you don't want _anyone_ to be in position of "cultural politeness enforcer"? But by literally saying you don't want women in that position (it only or especially bothers you when it's a woman?)... you seem to be kind of revealing some unappealing things about yourself.
Wait, what? I assume you mean you don't want _anyone_ to be in position of "cultural politeness enforcer"? But by literally saying you don't want women in that position (it only or especially bothers you when it's a woman?)... you seem to be kind of revealing some unappealing things about yourself.
It's always fascinating to me how two people can read the same words and come to two polar opposite conclusions.
My read of silveroriole's comment is that the commenter is a woman lamenting the fact that women are in the position of "politeness enforcer" far more often than men, and this causes people to treat her differently (i.e., like the "fun police") because they assume she is aligned with that world view.
My read of silveroriole's comment is that the commenter is a woman lamenting the fact that women are in the position of "politeness enforcer" far more often than men, and this causes people to treat her differently (i.e., like the "fun police") because they assume she is aligned with that world view.
You're in the minority. Most people don't want to deal with assholes berating and mistreating them.
Are you seriously saying you can't deal with people being courteous, professional, clear, and inclusive?
Are you seriously saying you can't deal with people being courteous, professional, clear, and inclusive?
Hey, you know where it's appropriate to be a character and make ranty comments? Stand up comedy! Maybe you should use your love of ranty comments and characters to change careers.
Would you please stop posting in the flamewar style to HN? It's against the site guidelines and destroys the curiosity that this site is supposed to be for. Therefore we ban accounts that do it. Crossing into personal attack, as you've done more than once today, is particularly not ok.
If you'd please review https://news.ycombinator.com/newsguidelines.html and make your substantive points thoughtfully from now on, we'd be grateful.
If you'd please review https://news.ycombinator.com/newsguidelines.html and make your substantive points thoughtfully from now on, we'd be grateful.
Why haven't you made this post to the people that reply to me?
It's always great to see more women authors and diversity in tech. At the same time, I think interpretating this as an explicit rejection of the above seems a bit stretch. I've seen these advice at Google for a long time. My intern host taught me to not be personal, and that "criticizing the code is not criticizing you" 7 years ago. I'd love more help to understand the above claim.
I learned that in grad school. From the transmitting side: "criticize the ideas, not the person." From the receiving side: "they're criticizing your work, not you." This was actually the most useful thing I learned in grad school. Thank god those two years were on the donors' dime(s).
I also think the parent commenter's interpretation is an exaggeration.
I also think the parent commenter's interpretation is an exaggeration.
| From the receiving side: "they're criticizing your work, not you."
For a lot of developers they see their work as an extension of themselves. I think this might be the case for a lot of folks whose primary work is thought work.
For a lot of developers they see their work as an extension of themselves. I think this might be the case for a lot of folks whose primary work is thought work.
“Oh honey, bless your heart. You are a lovely person, honest. But you can’t program for shit.”
Is this so heavily upvoted because it's written by women or because it's by google?
These all seem Professional Workplace 101 stuff. Blog bait really.
These all seem Professional Workplace 101 stuff. Blog bait really.
Sure, if you have a company that wants to produce useful code and not haze employees. For many companies, especially in big tech, code reviews are more about asserting dominance than actual helpful suggestions. Looking at you, AMZN...
I've noticed that at my current company as well. There's one reviewer who puts a comment forcing someone to come meet with him every PR, often the comment shows he hasn't even read the description, let alone the code. It's just a way to prove he's the owner and gatekeeper and no one else can commit to that area without coming to him in person.
Oblivious everyone prefers to keep a good office atmosphere, but code reviews also has to convey clearly what the problem and ensure code quality is sustained or improved over time. Being to vague and "nice", can mislead developers to think that it is optional to fix the problem. Companies that produces great code, are those who have a strong meritocratic culture where directness and quality stands above niceness and vagueness. Companies with the opposite, whatever that is (perhaps a democracy or indifference), tends to produce worse code and prevent developers from improving.
So, be decent and direct (not nice and vague). And make sure to also praise progress and good code. End of the day, most developers prefers honesty and to become a better programmer.
So, be decent and direct (not nice and vague). And make sure to also praise progress and good code. End of the day, most developers prefers honesty and to become a better programmer.
Don't confuse aggression and assertiveness. Have rules, and stick to them.
I can scream and yell at my dog all day long because he pulls on the leash, but I'll just be angry and the dog will continue to pull on the leash.
If I'm calm, kind, but firm, I find it's usually quite a bit more effective. I'm not getting angry, but if my dog wants to keep walking, he's got to calm down first. I'm willing to wait.
I can scream and yell at my dog all day long because he pulls on the leash, but I'll just be angry and the dog will continue to pull on the leash.
If I'm calm, kind, but firm, I find it's usually quite a bit more effective. I'm not getting angry, but if my dog wants to keep walking, he's got to calm down first. I'm willing to wait.
Most of these guidelines are reasonable enough. I think it's important to keep the DOs and DON'Ts list to the minimum, though. These interaction guidelines become counter-productive when they try to be all-encompassing definitions of good and bad interpersonal behavior.
I worked with one team who had an ever-expanding code of conduct. Each time they had a conflict, they tried to add rules to the CoC that they thought would prevent the conflict. Good intentions, but counter-productive execution. Three things happened:
1) New hires had to learn to navigate a minefield of obscure code of conduct rules. They tended to shutdown and socially isolate themselves for fear of accidentally triggering another obscure CoC issue, like including emojis in code reviews. The safest thing to do is only interact with close allies within the company, and minimize unnecessary interaction with anyone else. Silos.
2) The CoC became a tool for power plays. For a few bad apples, the CoC became a list of gotchas that they could use as a power play against others. At one point we had someone reading every Slack conversation and code review just to call out possible CoC violations (remember, this includes things like using emojis in code review) so they could call other people out on their violations. Substantiative conversations moved to private messages or in-person conversations. Not healthy. Partially solved by adding a CoC rule about minding your own business and explaining healthy ways to share the CoC with others.
3) Human interactions started to feel robotic, almost scripted. This one is harder to describe, but each interaction with someone in public or outside your close friend group started to feel like everyone was following a mental script for how our conversations could proceed. Conversations felt like a mental checklist of what was an acceptable way to discuss a differing opinion or answer a question without the possibility of stepping on anyone's toes. It became draining in ways that I haven't experienced since. It was a huge relief when I moved to another company where people were allowed to talk freely to each other, like professional adults.