4 ms·

This is why I think it's really really important that all PR reviews be synchronous, so that there's never any time spent twiddling your thumbs or context switc

This is why I think it's really really important that all PR reviews be synchronous, so that there's never any time spent twiddling your thumbs or context switching onto another change. Also it just makes it much easier to review a PR when you can sit down and actually talk about it in real time with the author, rather than having the ping messages back and forth interminably until you reach an agreement

wizofaus 4y ago
In one of my first jobs, code reviews were done exactly like this, with the the reviewer (my boss) and I sitting together at one computer going through the changes. It definitely has benefits but it's still important to ensure recommendations/concerns etc. get written down and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. But while the back & forth discussion that often occurs during review can benefit from being synchronous, it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.
- nightpool 4y ago
  > and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. See, I disagree, because this is absolutely the place where it helps the most—you can now go away and keep working on the same task without having to context switch to anything else or remember where you were or what you were working on. So you're never in a state where you're blocked and can't work on your ticket, and you don't have anything else to do or have to pick up another ticket and start learning about a whole completely separate problem. (And yes, definitely everything still needs to be written down, it's important to walk away knowing which changes you need to make, and why!) > it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time. In teams I've worked on, the expectation is that an engineers highest priority is always unblocking another engineer, so this very rarely happened. Unless they had an interview to go to or some kind of meeting—and in that case, you could always just ping somebody else. Obviously it's a style of work that relies really heavily on everyone sharing the same timezone and work hours, but it works really well to eliminate time lost to context switching and minimize the amount of time engineers spend blocked waiting on someone else.
  - wizofaus 4y ago
    But if you're expecting another dev to always be available to help you with a review, then they're the ones having to "context-switch", interrupting their own work. The reality is context-switching is something that comes with the territory if you're working as part of a team developing software. Which isn't to say there aren't opportunities to minimise the disruption it causes, but the idea that you can more or less eliminate it seems like wishful thinking. Perhaps there's a case for minimising it for more junior devs that aren't as capable of dealing with it, though it's equally possible younger brains are better at it!
    - nightpool 4y ago
      I think maybe we're using different definitions of the term "context-switch". Certainly it's an interruption, but I don't really think that sitting down with another engineer to do a focused code review where they've already written out a PR description and thought hard about the problem is comparable to starting a brand new branch or picking it up after a while away and trying to juggle 2, 3, or 4 in-progress tickets.
      - wizofaus 4y ago
        It requires a mental switch in focus. For me anyway, it's not a big difference if it's between working on implementing feature A then switching to working on implementing feature B, and working on feature A then switching to doing a code review for feature B, but I probably take my code reviewing responsibilities more seriously than most (e.g. I always checkout the branch locally, ensure it builds and runs etc.).
  - daniel-grigg 4y ago
    This sounds like you’re just externalising the costs of integration of your changes on to other people. Generally we want to reduce any accidentally complexity and the way to achieve that is to reduce the latency of code reaching production. So for example you want to minimise PR/branches in flight ideally to 0. So you should be asking how you can reduce them - eg trunk based development, omitting asynchronous code reviews etc.

wizofaus 4y ago

In one of my first jobs, code reviews were done exactly like this, with the the reviewer (my boss) and I sitting together at one computer going through the changes. It definitely has benefits but it's still important to ensure recommendations/concerns etc. get written down and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. But while the back & forth discussion that often occurs during review can benefit from being synchronous, it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.

nightpool 4y ago
> and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. See, I disagree, because this is absolutely the place where it helps the most—you can now go away and keep working on the same task without having to context switch to anything else or remember where you were or what you were working on. So you're never in a state where you're blocked and can't work on your ticket, and you don't have anything else to do or have to pick up another ticket and start learning about a whole completely separate problem. (And yes, definitely everything still needs to be written down, it's important to walk away knowing which changes you need to make, and why!) > it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time. In teams I've worked on, the expectation is that an engineers highest priority is always unblocking another engineer, so this very rarely happened. Unless they had an interview to go to or some kind of meeting—and in that case, you could always just ping somebody else. Obviously it's a style of work that relies really heavily on everyone sharing the same timezone and work hours, but it works really well to eliminate time lost to context switching and minimize the amount of time engineers spend blocked waiting on someone else.
- wizofaus 4y ago
  But if you're expecting another dev to always be available to help you with a review, then they're the ones having to "context-switch", interrupting their own work. The reality is context-switching is something that comes with the territory if you're working as part of a team developing software. Which isn't to say there aren't opportunities to minimise the disruption it causes, but the idea that you can more or less eliminate it seems like wishful thinking. Perhaps there's a case for minimising it for more junior devs that aren't as capable of dealing with it, though it's equally possible younger brains are better at it!
  - nightpool 4y ago
    I think maybe we're using different definitions of the term "context-switch". Certainly it's an interruption, but I don't really think that sitting down with another engineer to do a focused code review where they've already written out a PR description and thought hard about the problem is comparable to starting a brand new branch or picking it up after a while away and trying to juggle 2, 3, or 4 in-progress tickets.
    - wizofaus 4y ago
      It requires a mental switch in focus. For me anyway, it's not a big difference if it's between working on implementing feature A then switching to working on implementing feature B, and working on feature A then switching to doing a code review for feature B, but I probably take my code reviewing responsibilities more seriously than most (e.g. I always checkout the branch locally, ensure it builds and runs etc.).
- daniel-grigg 4y ago
  This sounds like you’re just externalising the costs of integration of your changes on to other people. Generally we want to reduce any accidentally complexity and the way to achieve that is to reduce the latency of code reaching production. So for example you want to minimise PR/branches in flight ideally to 0. So you should be asking how you can reduce them - eg trunk based development, omitting asynchronous code reviews etc.