When and where I started my career we had no automated code checks. When I opened a change request to our code, I had to ask two of my colleagues to review the change and it usually came back with a list of typos to fix, functions in need to break up, formatting problems, requests for explanation and the final boss of comments let's talk about this.
Time passed by, the number of comments went down and usually I only got a single message saying 'LGTM'.
I could say I was a perfect programmer and learned one or two things about spelling words right, but what really happened was my colleagues letting their guards down on smaller mistakes. It's trust. It's beautiful. And it made me a couple of long shifts fixing uncaught bugs.
Now I think we do code reviews for all the bad reasons.
All The Bad Reasons
The PR has to have at least X approvers
Oh those company guidelines! We ask for review because we have to ask for review so we end up with a rubber stamp and not learning anything. Usually the approver doesn't even know that part of the software very well, or is too busy to be bothered.
- Recommend Reviewers: Create groups of people familiar with certain services or parts pf code. Don't assign everyone of every part of the code, unless it's common domain.
- Time to Review: Create tags with estimation on how much time it takes to review the PR. I bet PRs with 5-15 minutes review time won't get stale as 45min monsters do.
- Similar PRs: If you have to update configuration in multiple repos, group the changes!.
I'm not sure this will work
Could be lack of confidence in self or in the system, anyways, you need to talk about the testing strategy.
- Start with tests covering the existing features that might break.
- Test on an ephemeral environment.
Does this fit the code aesthetic
Code aesthetics, typos, grammar or formatting should not be part of the reviewers job but it often still is.
- We are lucky we can implement automated PR checks to run linter, grammar scanner or anything we agree on before even asking a human for feedback. For this, you need to hold the addition of reviewers until all automated checks ran.
All The Good Reasons
I want to talk about my solution
The code feels incomplete or could probably be improved
I have concerns about nearby code
I rebased my branch and I want to know if something relevant changed
Anything can be a good reason until it starts an interesting conversation or pushes us towards smarter solutions. Just don't forget to spend your time with meaningful reviews and automate the boring stuff.
Top comments (1)
I really like this post! I think we're seeing some organizational interest in improving developer velocity by improving this step.