4 ms·

This is an amazing list. I just want to point out that there's an implicit requirement in this list, which is that this is optimized for reviewers who are willi

This is an amazing list. I just want to point out that there's an implicit requirement in this list, which is that this is optimized for reviewers who are willing to give you quick feedback. At a previous job, I worked on a team where we iterated towards the list that OP provides. It worked really well, and it worked because we had a culture of providing rapid reviews. The senior people on our team even thought that reviews were important enough to drop their own work. A lot about the team was suboptimal, but I was spoiled by getting such rapid feedback for my work. We had some partner teams that really challenged this. One was halfway around the world, so you were guaranteed to only get one review round per day. If you wanted to make progress while working with them, you needed to either send them huge changes or send them a bunch of changes at once. But structural feedback in an early change would cause you to need to rewrite the later changes. We broke this cycle by having them eventually give local reviewers stronger conditional approval in the integration path, which fixed our particular issue but showed that there were cracks in how we optimized our code review flow. The real trouble came from a local team, which was an overloaded platform team that we were built on. No matter what you did, they would only review your changes about once per week. Their feedback was super insightful, too, and they often caught very subtle misuses of their platform that would cause subtle user-visible bugs. You couldn't just skip their feedback. To try to manage their flow, they wanted teams to review pull requests locally before sending changes to them. So people on my team would write a change for this platform, which we'd review on our team. Our team would apply our rules and break the change up. We'd send the first one out. Then it would take a full month to get the entire thing in, to account for the back-and-forth. If you needed to get something substantial in, it could take months. My manager fought for months for them to acknowledge that this was a problem and fix it, and they did eventually dig out of it by setting SLAs and increasing capacity. I don't want to take away from this list, but I want to point out that some of it is a function of your reviewer's incentives. If your reviewers have different incentives, then you need to find your own list that works within those bounds.

mumblemumble 6y ago
I'm curious if a separate "design review" phase could have helped with your challenges with the team halfway around the world? I worked at one company where, due to the nature of the business, you really couldn't reasonably expect other teams to do code review in a timely manner. But most inter-team code review challenges had to do with high-level design, interface protocols, stuff like that. And they had to do with tasks that we could usually see coming down the pipe weeks ahead of time. So we developed a culture of just finding a time that worked for everyone to call a meeting with one or two stakeholders from each affected team, in order to hash out those design decisions before starting on the actual implementation work. It would stay pretty high-level - if any concrete work product came out of it, it would be an ADR or two. We did code review, too, of course. But agreeing on a plan ahead of time greatly reduced the risk of code review turning up problems that led to costly rework.
- jakevoytko 6y ago
  This is a good point. We did have a heavy up-front design phase for any projects between these teams, and it mitigated lots of problems before any code was written. This helped modify the conversations to a back-and-forth about specific implementation details instead of "you need to completely throw this out and do something else"
p5a0u9l 6y ago
those are fascinating, occasionally horrifying cross team examples you give. i suppose i’m spoiled to work in a small company where reviews are always local.

mumblemumble 6y ago

I'm curious if a separate "design review" phase could have helped with your challenges with the team halfway around the world? I worked at one company where, due to the nature of the business, you really couldn't reasonably expect other teams to do code review in a timely manner. But most inter-team code review challenges had to do with high-level design, interface protocols, stuff like that. And they had to do with tasks that we could usually see coming down the pipe weeks ahead of time. So we developed a culture of just finding a time that worked for everyone to call a meeting with one or two stakeholders from each affected team, in order to hash out those design decisions before starting on the actual implementation work. It would stay pretty high-level - if any concrete work product came out of it, it would be an ADR or two. We did code review, too, of course. But agreeing on a plan ahead of time greatly reduced the risk of code review turning up problems that led to costly rework.

jakevoytko 6y ago
This is a good point. We did have a heavy up-front design phase for any projects between these teams, and it mitigated lots of problems before any code was written. This helped modify the conversations to a back-and-forth about specific implementation details instead of "you need to completely throw this out and do something else"

p5a0u9l 6y ago

those are fascinating, occasionally horrifying cross team examples you give. i suppose i’m spoiled to work in a small company where reviews are always local.