Feedback Ladders: How We Encode Code Reviews at Netlify(netlify.com)
netlify.com
Feedback Ladders: How We Encode Code Reviews at Netlify
https://www.netlify.com/blog/2020/03/05/feedback-ladders-how-we-encode-code-reviews-at-netlify/
47 comments
[deleted]
This, this, this and again this!
I find it helps a lot if people's first interactions are not just in text but preferably in person but at least in video conf. The overall culture of the company plays a big part as well and so does the experience people bring with them from other companies.
If your only experience is adversarial PRs then even if the new company you joined has a perfectly calm conversation in PRs you may read them as adversarial.
Its the whole 'tone is not included in written communication' thing. So whatever expectation I have about how you would actually say the written thing will color everything. If we haven't had any real life interactions so far I may substitute my really bad 'last 3 companies' PR experience.
I find it helps a lot if people's first interactions are not just in text but preferably in person but at least in video conf. The overall culture of the company plays a big part as well and so does the experience people bring with them from other companies.
If your only experience is adversarial PRs then even if the new company you joined has a perfectly calm conversation in PRs you may read them as adversarial.
Its the whole 'tone is not included in written communication' thing. So whatever expectation I have about how you would actually say the written thing will color everything. If we haven't had any real life interactions so far I may substitute my really bad 'last 3 companies' PR experience.
> combined with no tolerance for bad behavior
People are terminated for a single hot-under-the-collar comment?
People are terminated for a single hot-under-the-collar comment?
Don't you think that is quite the disproportionate response? "No tolerance" simply means that bad behaviour will not be ignored, that is to say that there will be consequences.
I do think it's disproportionate, which is why I asked. "No tolerance" suggests something different to me than what you described.
I strongly disagree with the opposition to process documents. I've worked at many companies that functioned without good process documentation (for all functions, not just code reviews) and it simply _does not scale_ with organization size or process complexity.
There's no reason it can't be both intuitive and human while also reflected in process documentation that serves as a shared reference and grounding point.
There's no reason it can't be both intuitive and human while also reflected in process documentation that serves as a shared reference and grounding point.
As a manager this is how I've laid out our code review process, almost exactly. It has worked well since we adopted it. The only two devs who have left the team in the past 6 years were the types you mentioned: the rule follower and the rule maker.
The rule follower could neither handle the ambiguity of the communication media (written text) nor could he understand that the intention of some communications were not to dispute or criticize (cultural and personal communication idiosyncrasies) his work.
The rule maker was just a control freak. I'm glad he's gone.
The rule follower could neither handle the ambiguity of the communication media (written text) nor could he understand that the intention of some communications were not to dispute or criticize (cultural and personal communication idiosyncrasies) his work.
The rule maker was just a control freak. I'm glad he's gone.
> I now give them a talk about how code reviews are not to be taken personally, how they should assume best intent, and some guidance for how to handle disagreements.
That's the best part. Junior hires think their code review went horribly because they had to fix a lot of things. And then their performance review says that they are doing a great job.
That's the best part. Junior hires think their code review went horribly because they had to fix a lot of things. And then their performance review says that they are doing a great job.
Now instead of just dimensions for style, clarity, function, performance to argue about in code review you have a new dimension of terminology to bike-shed too:
"I think this is a boulder of a problem."
"No, no, no this is clearly dust."
"Looks like a pebble to me!"
I get the idea but IMHO you really need from the start to have some way to measure and quantify if this is actually improving code reviews in your organization. Do you have metrics showing amount of time spent on reviews, amount of bugs generated by commits, etc. and are you tracking them going up or down?
I can also see a certain path of dysfunction where people get known by their perception of problems. Suddenly Joe doesn't get asked for many reviews because he's "the mountain guy" that always thinks things are blockers. There really needs to be some thought around calibration and review of people's prioritization. Just adding names and terminology doesn't solve that problem.
"I think this is a boulder of a problem."
"No, no, no this is clearly dust."
"Looks like a pebble to me!"
I get the idea but IMHO you really need from the start to have some way to measure and quantify if this is actually improving code reviews in your organization. Do you have metrics showing amount of time spent on reviews, amount of bugs generated by commits, etc. and are you tracking them going up or down?
I can also see a certain path of dysfunction where people get known by their perception of problems. Suddenly Joe doesn't get asked for many reviews because he's "the mountain guy" that always thinks things are blockers. There really needs to be some thought around calibration and review of people's prioritization. Just adding names and terminology doesn't solve that problem.
in an ideal world, yes it'd be nice to quantify. in practice, it would take extra dev time to quantify. time that we dont have. we have to rely on our engineers' subjective feelings about our engineering practices.
i understand the desire to be data driven. and your point about making everything a blocker. if you're at a point where you dont trust the judgment of your colleagues, you should drop everything and address that right away.
but, most of the time, operating in good faith works.
i understand the desire to be data driven. and your point about making everything a blocker. if you're at a point where you dont trust the judgment of your colleagues, you should drop everything and address that right away.
but, most of the time, operating in good faith works.
Be careful, this is how institutions bake-in bias. Subjective feelings are littered with implicit and explicit bias at every level.
At the end of the day it's all just people management problems. You're right it's a time sink for devs to quibble over and quantify prioritization--that's why I think it's more of a thing for management to assess and review. Trust devs to assess priority on their own, but verify with regular followup and review that those assessments are accurate. Business needs will change too and that might alter priorities--what was dust last week is now an enormous mountain because customer X demands the obscure feature no one cared about before.
At the end of the day it's all just people management problems. You're right it's a time sink for devs to quibble over and quantify prioritization--that's why I think it's more of a thing for management to assess and review. Trust devs to assess priority on their own, but verify with regular followup and review that those assessments are accurate. Business needs will change too and that might alter priorities--what was dust last week is now an enormous mountain because customer X demands the obscure feature no one cared about before.
I get the feeling this sort of solution is for a problem specific to a small number of devs. For example, "we have a new developer on the team and they don't know what to expect" or "we have an expert dev who only speaks in koans!". Instead of handling these on a case by case basis by team leads, a new requirement was made for all developers.
Agreed. This type of process feels obvious to those who implement it, but becomes a burden that everyone else has to bear forever.
As this type of process accumulates, it becomes more and more painful for newcomers trying to join the team without breaking the rules. Obviously one requirement about prefixing code review comments isn't much, but in my experience it usually doesn't stop here. People who like creating process and rules tend to want to create a lot of process and rules for everyone else to follow.
As this type of process accumulates, it becomes more and more painful for newcomers trying to join the team without breaking the rules. Obviously one requirement about prefixing code review comments isn't much, but in my experience it usually doesn't stop here. People who like creating process and rules tend to want to create a lot of process and rules for everyone else to follow.
Bike-shedding doesn't need any specific process to happen, it just needs people who want to bike-shed. If it's not over the severity label on the feedback, it'll just take some other manifestation.
The solution isn't to try to design a process to prevent bike-shedding, because you can't. It's to practice a culture that frowns upon bike-shedding and focuses on the using time well.
> I can also see a certain path of dysfunction where people get known by their perception of problems. Suddenly Joe doesn't get asked for many reviews because he's "the mountain guy" that always thinks things are blockers.
Joe probably doesn't need to declare everything "mountain" to become the guy that makes mountains out of molehills, and probably doesn't need the formalism to be the guy people don't like to ask for code reviews.
> There really needs to be some thought around calibration and review of people's prioritization.
Usually this is where we'd expect the manager to talk to Joe about being more constructive in code reviews.
The solution isn't to try to design a process to prevent bike-shedding, because you can't. It's to practice a culture that frowns upon bike-shedding and focuses on the using time well.
> I can also see a certain path of dysfunction where people get known by their perception of problems. Suddenly Joe doesn't get asked for many reviews because he's "the mountain guy" that always thinks things are blockers.
Joe probably doesn't need to declare everything "mountain" to become the guy that makes mountains out of molehills, and probably doesn't need the formalism to be the guy people don't like to ask for code reviews.
> There really needs to be some thought around calibration and review of people's prioritization.
Usually this is where we'd expect the manager to talk to Joe about being more constructive in code reviews.
Agree. the post is silly beyond words, sorry author. it feels like it was written for linkedin, not HN.
The world of open source have already settled for: "nit" or "nitpick" for things that you can live with without changing in a code review.
Everything else is just noise. Either you must change, or not. Anything in between is just harming communication.
The world of open source have already settled for: "nit" or "nitpick" for things that you can live with without changing in a code review.
Everything else is just noise. Either you must change, or not. Anything in between is just harming communication.
It’s an interesting system. On caveat: the Rock size does not really encode the intention of the comment. Some of my colleagues took the time to write up Conventional Comments which they use, I really like that it distinguishes between blocking/non-blocking issues, questions and even praise.
- Site: https://conventionalcomments.org/
- Discussion: https://news.ycombinator.com/item?id=23009467
- Site: https://conventionalcomments.org/
- Discussion: https://news.ycombinator.com/item?id=23009467
i like your system too!
oh wow nice to see this on HN! I wrote this blogpost - happy to answer any questions!
yes it works very well for Netlify; when i joined the frontend dev team i immediately saw the value of this idea and begged the eng and design team members (that came up with the idea) for permission to write it up
for those interested, I recently adapted this idea for project prioritization https://twitter.com/swyx/status/1366849232959807489?s=20 because I was searching for a better alternative to the nonsensical effort estimation vs impact backlog grooming systems prevalent in product management.
yes it works very well for Netlify; when i joined the frontend dev team i immediately saw the value of this idea and begged the eng and design team members (that came up with the idea) for permission to write it up
for those interested, I recently adapted this idea for project prioritization https://twitter.com/swyx/status/1366849232959807489?s=20 because I was searching for a better alternative to the nonsensical effort estimation vs impact backlog grooming systems prevalent in product management.
Thanks for the write up!
What are you thoughts on enforcing future action (pebbles)? One thing I try to push for is to never have TODOs in code, instead make a backlog ticket/issue and link it. Unfortunately those tend to stay de-prioritized.
How often do your pebbles get the desired follow-up?
What are you thoughts on enforcing future action (pebbles)? One thing I try to push for is to never have TODOs in code, instead make a backlog ticket/issue and link it. Unfortunately those tend to stay de-prioritized.
How often do your pebbles get the desired follow-up?
yeah backlog issue or TODO in code. i personally dont mind the TODOs in code because they remain in context. also, you often get benefits from accumulating some todos of a similar nature, then solving them in one go in a tech debt cleanup sweep.
in my time there (i've since left the company) we didn't get a lot of pebbles. boulders and sand were more common. perhaps i shouldve communicated that distribution more clearly.
in my time there (i've since left the company) we didn't get a lot of pebbles. boulders and sand were more common. perhaps i shouldve communicated that distribution more clearly.
I found this very helpful. As a geologist who just submitted His first commit at a major tech company, the review process was scary and confusing. This type of roadmap would have made it rock solid.
Who are you, strange geo-mime? Are you my evil twin?
... Are you my good twin?
This is tortuous double speak.
I used to work in a 10k employee mega organization where we had this kind of over formalization. It was probably invented to prevent feelings being hurt.
I then moved to a tiny five man consultancy where efficiency was what we lived (or died) by. Never forget the day when my first code review from the CEO was "that is f##king s@#t code!
I used to work in a 10k employee mega organization where we had this kind of over formalization. It was probably invented to prevent feelings being hurt.
I then moved to a tiny five man consultancy where efficiency was what we lived (or died) by. Never forget the day when my first code review from the CEO was "that is f##king s@#t code!
I like it. I've also taken to doing something similar with other interactions - when weighing in on decisions I try to say up front how much I care about the feedback I'm giving. Same with messaging people on slack - if something is non urgent or has no action required, I try to say that up front. No idea if this helps people on the other end of my messages, but I like to think it does.
Personally I prefer to just have two levels, blocking and non-blocking. Outside of that I should be able to trust the code author to appropriately respond to comments, I don't see why I as the reviewer should be the one deciding on the follow up action. I can see how it might be more valuable for more junior members of the team though.
I really like this terminology! I might see if my team could use this, I've found that not everyone interprets code review comments the same way.
I'm working on a new tool to vastly improve human code reviews for teams on GitHub. One of the main things is making sure that each conversation is "resolved" so that there is no disagreement on when a PR is ready to go.
If you're interested in better code review for your GitHub repo, email me at [email protected] and I can show you what I'm working on. We're in a private Alpha but definitely ready for use by real teams in everyday reviews.
I'm working on a new tool to vastly improve human code reviews for teams on GitHub. One of the main things is making sure that each conversation is "resolved" so that there is no disagreement on when a PR is ready to go.
If you're interested in better code review for your GitHub repo, email me at [email protected] and I can show you what I'm working on. We're in a private Alpha but definitely ready for use by real teams in everyday reviews.
I found this very helpful. As a non-programmer who just submitted His first commit at a major tech company, the review process was scary and confusing.
This type of roadmap would have made it understandable and clear.
Edit: The down vote was because it was YAML wasn’t it? I know, I know, it’s not real programming.. But it still felt like an accomplishment!
Edit: The down vote was because it was YAML wasn’t it? I know, I know, it’s not real programming.. But it still felt like an accomplishment!
Hahaha no, that was kinda weird. Why would somebody copy my comment nearly word for word but with 50% more cleverness?
I've prioritized bugs in a similar way for years (P1 "This bug will cause data loss in prod" through to P4 "There's is a spelling mistake"), but it's never occurred to me to do the same sort of useful labelling of things in PRs. This is a great idea that I'm going to take to my team.
This seems to take the view that all feedback is pointing out shortcomings.
That's not the only type of feedback that's useful. Sometimes I might just want to ask a question, or propose another idea that may or may not be better than what was done. Sometimes I have testing suggestions. These are not problems.
That's not the only type of feedback that's useful. Sometimes I might just want to ask a question, or propose another idea that may or may not be better than what was done. Sometimes I have testing suggestions. These are not problems.
While I agree to an extent, because we don't see examples, a lot will depend on the two or more people in the conversation, their past experiences in general as well as with each other. Overall culture will influence this as well. Is it normal to just state facts in your culture or do you usually use extra polite language around it?
A great example I like to cite is this: You are at a company offsite. All the department leads from around the world are there. The mandatory to attend main event is about to begin. The main coordinator goes around to the various groups doing small talk ahead of the event and tells each of them: "If you would be so kind to proceed to the main event hall when you are ready, we are about to start".
Main event starts.
Someone notices that the department leads from Germany aren't there and still outside finishing up their coffee and patisserie, talking loudly.
They don't understand at all, why the coordinator, who is from the UK (or Canada) is upset that they didn't follow her instructions to go to the main event now.
(in case you're wondering, the Germans would have expected you to come up to them and say something like: main event is starting in 5 minutes. Make sure to be there, it's mandatory)
A great example I like to cite is this: You are at a company offsite. All the department leads from around the world are there. The mandatory to attend main event is about to begin. The main coordinator goes around to the various groups doing small talk ahead of the event and tells each of them: "If you would be so kind to proceed to the main event hall when you are ready, we are about to start".
Main event starts.
Someone notices that the department leads from Germany aren't there and still outside finishing up their coffee and patisserie, talking loudly.
They don't understand at all, why the coordinator, who is from the UK (or Canada) is upset that they didn't follow her instructions to go to the main event now.
(in case you're wondering, the Germans would have expected you to come up to them and say something like: main event is starting in 5 minutes. Make sure to be there, it's mandatory)
Is there any metric to prove that this proposal works?
I mean, you can impose all sorts of rules and whatnot, but it's all [dust] until it improves a metric that the company cares about (e.g. review time, # of bugs, etc.).
I mean, you can impose all sorts of rules and whatnot, but it's all [dust] until it improves a metric that the company cares about (e.g. review time, # of bugs, etc.).
I am not sure that I want to get reminded every code review that I am from a country without boulders or mountains
Tried it, didn't catch on because emojis are too far out of reach from the github UI lol
we dont actually use emojis - we just use names, eg. "[dust] i dont like how this looks but whatever", "[boulder] this code doesnt run"
This is a 5 point rating system. Personally, I try to stick to as few points as possible.
The consumer of the rating (in this case, the code author) usually needs to know one thing above all else: Do I take action or not. A two-point system is the default.
IMO, in a code review, the feedback should be either expected action or not. "I recommend you change this" or "FYI, consider this", aka, "required" or "optional".
Ultimately, as the author, I want to know if I'm being told "please change this" or "just FYI". Details can be haggled if necessary.
The strength of the recommendation isn't relevant unless there's push back, in which case the details can be haggled for the situation at hand. But trying to do that bucketing up front sounds like extra work that's usually unnecessary. As long as the reviewer is pursuing productivity, they can adjust their recommendation as they learn more.
The consumer of the rating (in this case, the code author) usually needs to know one thing above all else: Do I take action or not. A two-point system is the default.
IMO, in a code review, the feedback should be either expected action or not. "I recommend you change this" or "FYI, consider this", aka, "required" or "optional".
Ultimately, as the author, I want to know if I'm being told "please change this" or "just FYI". Details can be haggled if necessary.
The strength of the recommendation isn't relevant unless there's push back, in which case the details can be haggled for the situation at hand. But trying to do that bucketing up front sounds like extra work that's usually unnecessary. As long as the reviewer is pursuing productivity, they can adjust their recommendation as they learn more.
I personally think there are three levels of that I need to distinguish between:
- required, to be implemented before the code is merged
- required, but we can merge the code as is and create a separate action for resolving later (e.g. open a Jira)
- optional, for the author's consideration
- required, to be implemented before the code is merged
- required, but we can merge the code as is and create a separate action for resolving later (e.g. open a Jira)
- optional, for the author's consideration
Jesus christ man, just learn to talk gud
Title should be updated to add ‘(2020)’.
This is a really good and simple to use suggestion!
In my experience, setting an expectation that teams need to be respectful and communicative in code reviews, combined with no tolerance for bad behavior, is sufficient to keep the code review process running smoothly.
I'm not a fan of explicit process documents that prescribe communication practices to employees. There is a very specific type of person who loves to be handed a rule book for how to communicate with others, and they love these systems. They prefer to operate within structured environments where they can read the rulebook ahead of time, or perhaps even write the rulebook for others to follow. Having the rules spelled out like this brings an artificial sense of comfort, especially to those who otherwise struggle with organic communication.
For everyone else, the complexity of these systems is unnecessary mental overhead. Do two developers who were already communicating just fine need to adopt this process? More importantly, does the process stop here, or will there be additional sets of communication rules whenever another possible communication ambiguity arises?