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?
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.
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.
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.
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.
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 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.
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".
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.
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?
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.
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'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?
Can you pop open an un-audited video session and have the reviewer drive while they inspect and talk about what they see?
Pycharm also has a similar service if you use Pycharm.
Then ask the team how to improve code quality?
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.
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
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.
