Code Reviews Without Pull Requests

I’m going to put my most controversial opinion right out there and wave it around, because I am sick of internal development teams blindly trusting code reviews performed on pull requests.

Don’t get me wrong. I think there are some very clever UI’s provided by most providers (GitHub, Azure DevOps, Bitbucket, etc.) but no matter how well conceived they are, they can’t get around the fact that the limited view of the world provided by a diff is nowhere near enough to give proper feedback.

TL;DR;

Great code is driven by collaboration. Collaboration works best when instead of hand-offs between code writer and code reviewer, both developers work together to write great code up front. That doesn’t necessarily mean pair programming, which I know a lot of developers aren’t comfortable with – it can just mean grabbing someone to talk to about what you’re going to do and generating some ideas.

It’s not only helpful to you as a developer to talk through your plan first, it’s also a discussion that most developers will find enjoyable – getting the chance to geek out about different approaches is fun!

Then, perhaps, when things are starting to take shape, grab that same person again to show how it’s going – take some joy in the fact that it’s working out, or point out the issues you’ve found and discuss ways to pivot to another approach.

Share what you’re doing at least once a day, and there’s no longer a need for code review. It’s been done up front. No rewrites, no hurt feelings, no snubbed noses, no messing around.

Where code review UI’s all fall down

None of the CR tools are good enough to allow a developer to navigate around the code as if it was on their own machine. They are all based around the same fallacy: that the diff is what needs reviewing. The diff does not live in isolation – you can’t read down a list of changes and know whether they are ‘good’ unless you take the surrounding code into account. Not just the code in the changed classes, but the composition of the software in the areas changed. It might be that your main problem with the change is that surrounding code wasn’t refactored to match the changes. So you’re going to have to clone the repo, find the branch, get to the area that’s been changed, and keep switching back and forth with the diff to see the change. Do you do this? Be honest.

Thrashing

“Oh, you meant like the builder pattern!? I’ll try that again. 3rd time lucky!”

No. Not 3rd time lucky – a fun chat up front vs back and forth, back and forth. No. Just no.

Shared understanding

There are so many different approaches to code composition, and different ways to uncover hidden requirements. Discussing things lets developers share their knowledge and opinions. It’s hard to grow in isolation – you can study new ideas and complete online courses, but seeing someone implement a thing right in front of you is a much more efficient way to learn. Again, I’m not saying you must always be pair programming, but you shouldn’t be working in complete isolation.

Branching

And then we come to my main pet peeve.

If you don’t need a code review, and you have a CI/CD pipeline with automated tests and different environments, why would you ever create a feature branch? Even a short-lived branch – why?

If you work directly on trunk, you can instantly share what you’re doing with your collaborator by simply pushing your changes. You instantly get everyone else’s changes, so you know whether anything they’ve pushed breaks the small bit you have not yet pushed. It was their responsibility to make sure their changes worked with what you have already pushed, so it’s in your best interest to push little and often. This is the pinnacle of continuous integration.

Things to try

Whether you have a CR process or not, next time you are picking up a piece of work, grab another developer and talk to them about what you’re planning. See what opinions they have. Be open to learning and share your own knowledge without ego.

Go forth and collaborate.