Ask HN: Over the shoulder Code-Review in remote settings
28 comments
I’ve found that pull request size solves a lot of the issues that people have with code reviews and quality. When people see a very large pull request, there is a tendency to skim and then slap on an Approval. Keeping pull requests small typically leads to a more thorough review because it’s much easier to parse the changes and build a mental model. This usually leads to better feedback. This also helps prevent less experienced devs from going crazy down the rabbit hole and making a huge code change. Small and steady is best, and fostering a culture where people are often asking each other questions and collaborating is key.
> Keeping pull requests small typically leads to a more thorough review because it’s much easier to parse the changes and build a mental model.
Corollary, the practice that exists in some organizations or some peoples minds to change everything (code structure, names, formatting, ...) that doesn't match current definitions and/or personal taste as part of a change for some specific feature slows things down, and hides potential problems by covering the intended modification in the fog of the other changes.
Separation helps. Even for some intended changes, by splitting into incremental steps. Of course starting with N+1 requires a speedy review of N. Which requires N to be focused, to be easy to check.
If something needs a complete rewrite, that's a different story.
Corollary, the practice that exists in some organizations or some peoples minds to change everything (code structure, names, formatting, ...) that doesn't match current definitions and/or personal taste as part of a change for some specific feature slows things down, and hides potential problems by covering the intended modification in the fog of the other changes.
Separation helps. Even for some intended changes, by splitting into incremental steps. Of course starting with N+1 requires a speedy review of N. Which requires N to be focused, to be easy to check.
If something needs a complete rewrite, that's a different story.
Alternatively, as small PRs can be hard (depending on codebase, language, etc), you can try to do small commits.
Aiming for each commit telling a logical part of the story, that together make a PR that is easy to review.
Why is this easier than small PRs? Typically PRs can’t break anything. Whether it’s CI or breaking the build on the main branch or breaking the workflow of other devs, normally main needs to be stable and PRs are the unit of stable addition to that. However, each commit does not necessarily need to be so, especially if you squash or merge so that there are clear working points to revert to on the main branch.
In many cases my commits might not even compile, but this frees me up to do things like: put bulk renaming or code moves in commits that can mostly be ignored, keep business logic changes in small commits that are easily reviewed, write all test function definitions before filling them in to help summarise the testing that is done, etc. thankfully over the last 4 years or so GitHub’s handling of commit level review on PRs has improved a lot.
Small PRs should still be the goal for many reasons, but when it’s going to take way too much work to do, small commits that lead the reviewer through a story of the change being made can be very effective.
Aiming for each commit telling a logical part of the story, that together make a PR that is easy to review.
Why is this easier than small PRs? Typically PRs can’t break anything. Whether it’s CI or breaking the build on the main branch or breaking the workflow of other devs, normally main needs to be stable and PRs are the unit of stable addition to that. However, each commit does not necessarily need to be so, especially if you squash or merge so that there are clear working points to revert to on the main branch.
In many cases my commits might not even compile, but this frees me up to do things like: put bulk renaming or code moves in commits that can mostly be ignored, keep business logic changes in small commits that are easily reviewed, write all test function definitions before filling them in to help summarise the testing that is done, etc. thankfully over the last 4 years or so GitHub’s handling of commit level review on PRs has improved a lot.
Small PRs should still be the goal for many reasons, but when it’s going to take way too much work to do, small commits that lead the reviewer through a story of the change being made can be very effective.
"Over the shoulder" reviews sound like they are halfway between code reviews and pair programming.
Code reviews usually mean someone taking an independent look at their own pace, though quick turnaround is always great. You can get classic reviews by simply mandating approvals on pull requests on any system you are using.
You can also get halfway with screen sharing developer's IDE, though if a reviewer doesn't get to easily jump around the codebase, it's not really the same thing (though it's equivalent to what you've been doing in person).
I found screen sharing to work well for pair programming, so I don't see why it wouldn't work for live reviews either.
Code reviews usually mean someone taking an independent look at their own pace, though quick turnaround is always great. You can get classic reviews by simply mandating approvals on pull requests on any system you are using.
You can also get halfway with screen sharing developer's IDE, though if a reviewer doesn't get to easily jump around the codebase, it's not really the same thing (though it's equivalent to what you've been doing in person).
I found screen sharing to work well for pair programming, so I don't see why it wouldn't work for live reviews either.
Over the shoulder is often the developer explaining their decisions in the code, instead of the reviewer trying to reverse-engineer it, independently. It's just faster and has less resistance -- not necessarily better.
Problem with remote live reviews is that in a remote environment, it's harder to tell if someone is free or they are doing their own deep work.
Either the developer has to wait for the review to be done asynchronously before the merge... or ping someone to review their code through a screenshare and take away their attention.
Problem with remote live reviews is that in a remote environment, it's harder to tell if someone is free or they are doing their own deep work.
Either the developer has to wait for the review to be done asynchronously before the merge... or ping someone to review their code through a screenshare and take away their attention.
The reviewer can ask questions on the pr? You shouldn't need to reverse engineer code to figure out their decisions. If it's really that noteworthy the programmer should leave a comment explaining their decision.
My team posts their PR reviews in our team slack channel. Which bothers the totally async purist in me, but we’ve found it to be a good middle ground between waiting for reviews requested via GitHub email and actually pinging someone.
If something’s super complex, and I don’t feel like it’s the submitter’s fault, or even if I’ve got a PR where I realize the code I’ve written isn’t optimal, and async feedback would be too slow, I’ll usually schedule a meeting on someone’s calendar, giving them at least a day of notice.
But otherwise? If I can’t understand what’s going on without too much difficulty, that’s on the PR submitter to improve their code readability through structuring, naming, comments, or as a last resort, external documentation imo. So I see not having the original submitter involved as a feature. Who knows if they’ll be around when you have questions about the same code in 2 years? Is someone going to document everything that was said in the “over the shoulder” review?
That being said, the work I do isn’t anything cutting-edge. More complex code justifies more involved review practices.
If something’s super complex, and I don’t feel like it’s the submitter’s fault, or even if I’ve got a PR where I realize the code I’ve written isn’t optimal, and async feedback would be too slow, I’ll usually schedule a meeting on someone’s calendar, giving them at least a day of notice.
But otherwise? If I can’t understand what’s going on without too much difficulty, that’s on the PR submitter to improve their code readability through structuring, naming, comments, or as a last resort, external documentation imo. So I see not having the original submitter involved as a feature. Who knows if they’ll be around when you have questions about the same code in 2 years? Is someone going to document everything that was said in the “over the shoulder” review?
That being said, the work I do isn’t anything cutting-edge. More complex code justifies more involved review practices.
> My team posts their PR reviews in our team slack channel. Which bothers the totally async purist in me, but we’ve found it to be a good middle ground between waiting for reviews requested via GitHub email and actually pinging someone.
Why don’t you install the GitHub Slack application? It will tell you immediately when somebody asks for a review and it has other useful functionality as well.
Why don’t you install the GitHub Slack application? It will tell you immediately when somebody asks for a review and it has other useful functionality as well.
Oh, I guess some additional context is that by convention we use Slack emoji to indicate whether we approve, comment or request changes. Then again when it’s merged/deployed. Reviews aren’t specifically requested of individuals, usually just the entire team. That way, if it’s a channel everyone is in, other people can see at a glance whether they should still review the PR.
The github slack integration would post a message in a slack channel and then your team can use emojis to react to it just like you do.
Will have to check that out then, thanks!
> Over the shoulder is often the developer explaining their decisions in the code, instead of the reviewer trying to reverse-engineer it, independently.
If the reviewer needs to reverse-engineer the code to understand it without the author explaining it, this is a strong signal that the code is not high quality and needs more work. In this case, “over the shoulder” is actually bypassing something that a pull request would catch. “It’s not clear what this code does” is a perfectly reasonable cause to ask for changes in a pull request.
If the reviewer needs to reverse-engineer the code to understand it without the author explaining it, this is a strong signal that the code is not high quality and needs more work. In this case, “over the shoulder” is actually bypassing something that a pull request would catch. “It’s not clear what this code does” is a perfectly reasonable cause to ask for changes in a pull request.
> Over the shoulder is often the developer explaining their decisions in the code, instead of the reviewer trying to reverse-engineer it, independently. It's just faster and has less resistance -- not necessarily better.
Ouch, this is a great point. So for code that has to live for a long-ish time over-the-shoulder review is definitely inferior.
I have setup review process in a company I work in, and one of the rules is "the best way to answer the reviewer's comment is to change the code or add a comment".
Ouch, this is a great point. So for code that has to live for a long-ish time over-the-shoulder review is definitely inferior.
I have setup review process in a company I work in, and one of the rules is "the best way to answer the reviewer's comment is to change the code or add a comment".
What size is the company? For a big company, just prevent merge to main unless approved by another team member. Set up pairings, like Junior<-->Senior reviews. The junior reviewer will learn a low from the senior coding style.
For very small companies, ask yourself if you have time for code review. If you do, great -- but sometimes you are all just burning the midnight oil in those 1-5 person companies just to KTLO. Maybe do with less frequency if company is tiny and doesn't have PMF.
For very small companies, ask yourself if you have time for code review. If you do, great -- but sometimes you are all just burning the midnight oil in those 1-5 person companies just to KTLO. Maybe do with less frequency if company is tiny and doesn't have PMF.
I find the distinction you make interesting. To me, code review is an essential tool for code quality. Not just to avoid merging problematic code but even spreading knowledge of design and approaches and unifying style. Skipping it necessarily reduces code quality.
In other words, why would code quality matter more in a larger company, or why would programmers in smaller companies be able to somehow magically produce relatively higher quality so that reviews are not crucial for them?
In other words, why would code quality matter more in a larger company, or why would programmers in smaller companies be able to somehow magically produce relatively higher quality so that reviews are not crucial for them?
But that’s what GP is saying! It took me time to adapt to that idea too, going from a fortune 50 company to a startup.
Code quality is less important at a small company or startup, where you don’t know if the code you write (or the company itself) will even be around in a few years. So building a buggy feature quickly is usually better than a solid, well-tested and reviewed one that takes a lot of time to get out the door.
Code quality is less important at a small company or startup, where you don’t know if the code you write (or the company itself) will even be around in a few years. So building a buggy feature quickly is usually better than a solid, well-tested and reviewed one that takes a lot of time to get out the door.
This is incredibly hard to accept but is so true!
Call with screenshare when there's some issue to ask about/review?
On the whole I prefer it to 'over the shoulder' I think - 'no that bit, where I'm pointing but you can't see, gah' is annoying, but I get my own screen to look at.
(Even better might be some kind of remote editor session, so I could connect from whatever editor I like to use, on my screen at my resolution with my colours, and see your cursor move around while you talk. No idea how you'd do that in a cross-editor sort of way, though I suppose you could support major ones in a way that enabled them to be used with each other, e.g. I view in vim while partner moves cursor in VSCode or whatever.)
On the whole I prefer it to 'over the shoulder' I think - 'no that bit, where I'm pointing but you can't see, gah' is annoying, but I get my own screen to look at.
(Even better might be some kind of remote editor session, so I could connect from whatever editor I like to use, on my screen at my resolution with my colours, and see your cursor move around while you talk. No idea how you'd do that in a cross-editor sort of way, though I suppose you could support major ones in a way that enabled them to be used with each other, e.g. I view in vim while partner moves cursor in VSCode or whatever.)
>I have some positional power, so I can experiment a little with not-so-much resistance.
I've found power from being competent to be more effective.
>While we were in-office we could do simple over the shoulder code reviews and exchange valuable feedback fast without any official process.
What is this trying to solve that regular code review doesn't? Why does it need to be over the shoulder?
>what changes can we start making in our team to introduce code review practices to make the code quality better and avoid the common mistakes?
Require code review for any non-trivial change?
I've found power from being competent to be more effective.
>While we were in-office we could do simple over the shoulder code reviews and exchange valuable feedback fast without any official process.
What is this trying to solve that regular code review doesn't? Why does it need to be over the shoulder?
>what changes can we start making in our team to introduce code review practices to make the code quality better and avoid the common mistakes?
Require code review for any non-trivial change?
What was the point of "over the shoulder?" Did you do that to avoid blame? Or avoid someone outside the team seeing code review comments? This smacks of cultural fear.
Can you pop open an un-audited video session and have the reviewer drive while they inspect and talk about what they see?
Can you pop open an un-audited video session and have the reviewer drive while they inspect and talk about what they see?
VS Code Liveshare sessions allow you to join your colleague's VS Code to do an over the shoulder code review. Really recommend it for pairing and for some code reviews.
Pycharm also has a similar service if you use Pycharm.
Pycharm also has a similar service if you use Pycharm.
Normalize asking for feedback in your team.
Then ask the team how to improve code quality?
Then ask the team how to improve code quality?
If you are going to do a proper review and wish to run stuff on your own you always risk problems with dependencies and environment itself.
You could solve this with devcontainer.json and using tools which support this such as Codespaces or Codeanywhere. In addition in Codeanywhere you can share the screen from inside the IDE, have collaborative terminal or code sharing with location following.
You could solve this with devcontainer.json and using tools which support this such as Codespaces or Codeanywhere. In addition in Codeanywhere you can share the screen from inside the IDE, have collaborative terminal or code sharing with location following.
> While we were in-office we could do simple over the shoulder code reviews and exchange valuable feedback fast without any official process.
I've found out that while over the shoulder code review is good, pair programming (in remote settings or not) is even better. Then you can exchange feedback while coding, which can be more effective than a code review. There are multiple tools like Codeanywhere or Code-With-Me (IntelliJ IDEA), Liveshare in VSCode, etc. that facilitate remote pair programming.
I am heading a couple of teams who are also now fully remote and what I usually see is that the engineers make fairly small and self-contained PRs. Those *generally* don't call for a over-the-shoulder code review. Keep in mind that we simply never merge code without a code-review regardless of how small the change is, so most engineers in our organization use small, easy-to-digest PRs in order to make it easier for their colleagues to reason about the change.
If you find that a PR has attracted a lot of comments and back-and-forth discussion, it's good to encourage the devs to jump on a call or go to a meeting room to try to clear it up synchronously by talking it out. One thing to always consider is that in a fully remote setting you probably don't want to have a lot of those talks going on all the time as they can be fairly disruptive and time-consuming. Having lots of those meetings can be a sign of a couple of things:
- maybe the PRs are too big to digest and create lots of questions
- or maybe team members were not on the same page about the implementation BEFORE coding
- or (rarely) your teams don't have an agreement on consistent coding practices. For example: back-and-forth about tabs vs spaces, linting, or ways to do repeatable tasks like logging or class/variable naming, etc.
TLDR;
- Always requiring a code review for every PR is a good thing. With time your team will work well and everybody's gonna be knowledgeable what others are working
- Keep PRs/code-changes small. That reduces the need for over the shoulder-code-reviews which most of the time can be substituted with a simple approve on the PR
- Over-the-shoulder code reviews are good but pair-programming sessions (remote or not) are even better
- If you do have to jump on a call about a code change, that shows that maybe some of the above things didn't work out. Still go for it as soon as you find out async comments on the PR don't work
I've found out that while over the shoulder code review is good, pair programming (in remote settings or not) is even better. Then you can exchange feedback while coding, which can be more effective than a code review. There are multiple tools like Codeanywhere or Code-With-Me (IntelliJ IDEA), Liveshare in VSCode, etc. that facilitate remote pair programming.
I am heading a couple of teams who are also now fully remote and what I usually see is that the engineers make fairly small and self-contained PRs. Those *generally* don't call for a over-the-shoulder code review. Keep in mind that we simply never merge code without a code-review regardless of how small the change is, so most engineers in our organization use small, easy-to-digest PRs in order to make it easier for their colleagues to reason about the change.
If you find that a PR has attracted a lot of comments and back-and-forth discussion, it's good to encourage the devs to jump on a call or go to a meeting room to try to clear it up synchronously by talking it out. One thing to always consider is that in a fully remote setting you probably don't want to have a lot of those talks going on all the time as they can be fairly disruptive and time-consuming. Having lots of those meetings can be a sign of a couple of things:
- maybe the PRs are too big to digest and create lots of questions
- or maybe team members were not on the same page about the implementation BEFORE coding
- or (rarely) your teams don't have an agreement on consistent coding practices. For example: back-and-forth about tabs vs spaces, linting, or ways to do repeatable tasks like logging or class/variable naming, etc.
TLDR;
- Always requiring a code review for every PR is a good thing. With time your team will work well and everybody's gonna be knowledgeable what others are working
- Keep PRs/code-changes small. That reduces the need for over the shoulder-code-reviews which most of the time can be substituted with a simple approve on the PR
- Over-the-shoulder code reviews are good but pair-programming sessions (remote or not) are even better
- If you do have to jump on a call about a code change, that shows that maybe some of the above things didn't work out. Still go for it as soon as you find out async comments on the PR don't work
Generally, it’s been useful for my teams to have open discussions about establishing coding standards amongst themselves. We’ve also made code reviews mandatory as part of pull requests although it’s not as effective for smaller teams that want to be more agile.
ssh tunneled VNC clients.
The best programming I’ve done is remote with the other person staring at the same screen as me. We also had the same size screen so there was no scaling and such.
Makes switching between drivers seamless. Unfortunately my current team uses screen share on video chat apps. It’s the worst.
The best programming I’ve done is remote with the other person staring at the same screen as me. We also had the same size screen so there was no scaling and such.
Makes switching between drivers seamless. Unfortunately my current team uses screen share on video chat apps. It’s the worst.
Use a screen share or collaborative code editor.
For screen share, I loved Tuple[0] when I tried it out.
[0] https://tuple.app/
[0] https://tuple.app/
We already have lints, type-checking and some specs in place. But no manual reviews, yet.
While we were in-office we could do simple over the shoulder code reviews and exchange valuable feedback fast without any official process.
Since now my team is remote (although in the same timezone), what changes can we start making in our team to introduce code review practices to make the code quality better and avoid the common mistakes?