Won’t Review it? Don’t Approve it.
I’ll tell you a short story.
There’s a Pull Request (PR) that hasn’t been reviewed in 5 working days now, Scrum master comes to standup the next Monday asking why, and engineers give all the reasons they can as to why it hasn’t been reviewed, like “it’s very long”, “I’ve been extremely busy with my tasks”. Scrum master sternly asks for it to be reviewed, approved and deployed to staging today, because QA must test today, and Tuesday will be used for any necessary clean up, as we must roll out by Wednesday IF ALL GOES WELL. Great!
Engineers rush to review hundreds of lines of codes containing new features, modifications and bug fixes in just minutes. The worst comment on the PR is wrong code formatting, author fixes it. PR is approved and merged, pipeline builds, changes get deployed to Staging, QA tests for Acceptance Criteria (AC) on the ticket, everything seems good, and boom, changes are made available on Production. Nobody notices anything, all of a sudden, by Friday, a customer can not transact, KYC is failing, loan rates aren’t proper, and it becomes a disastrous weekend of unrest for engineers to locate the issue and come up with the fixes.
That’s the end of my short story. Whilst, this story may sound cliche to many, you’ll find different variations of this reality in quite a number of engineering teams.
Now let’s back up a bit.
Won’t Review it properly? Then don’t Approve it.
PRs can often stay unreviewed for long, and this is due to a lot of reasons, e.g PR wasn’t properly broken down and hence is too long, reviewers are busy trying to get their own stuff out, it’s been reviewed but the author hasn’t responded to comments yet, unending comments and unending fixes etc.
These are all valid reasons to an extent, no doubt, but not at all my emphasis. So I will say this at this juncture.
NEVER approve a PR by just skimming through it without a thorough/proper review, especially for crucial changes like addition of new features and critical modifications to existing features.
This is my emphasis. If you take nothing else away from this post, take the above.
I’ll tell you what’s worse than a PR being in review for more than a week or more, it’s for that PR to contain code that could break Production actually getting to Production — of course, this has and can happen at times, even here at Carbon, as we’re not perfect — but better caught on staging, and best caught whilst still a PR.
Now there’s the talk of, how did such a bug get past QA/regression? I won’t get into all that here, all I’ll say is it’s very possible.
A Critical Component of a good Code Review
This is one practical way I review code, by no means an industry standard, but I’ve found it to be quite a good strategy. After taking a look at the ticket to understand the requirements, I go back to go through the PR code, checking to see that it fulfils the requirements, this is it.
Now, this is important because I don’t care if your implementation of a design pattern is optimal or not, or if the code isn’t properly formatted, or how you can reduce a 10-line implementation to 2 lines using syntactic sugar or lambdas and streams, when in the first place the code isn’t going to do what it’s meant to do.
There’s a word we use here at Carbon to refer to code that doesn’t do what it’s supposed to do, instead doing something else…
Beans
…e.g “guy, I don cook beans oh!”.
Once requirements are met, only then do I begin to check for places where patterns or designs used can be optimised, where performance/memory usage can be made better, where syntax can be improved, and very importantly possible side effects of the implementation.
Conclusion
Even if a PR has been there for 5 days, and it’s urgent for it to go through, and you’re buzzed, still take your time to do an averagely proper review or ask someone else to be more thorough. It’s okay to reject a PR review if the “reviewee” requires a “quick one” but the code is relatively large. If you can’t review, don’t approve at all. That said, we can and should all do better in reviewing PRs, at least that of our team’s.
On a personal note, the more thorough you are with reviewing my code, the more confident I’ll be with you reviewing my future PRs. This is still the best way to prevent issues in the long run. We have an old mantra here at Carbon that we’re trying to revive.
Be hard on the code, not the coder.
A former Senior Engineer here at Carbon said this…
In the end we all have one goal, to have a bug-free system, that matches the requirements.
In a follow-up article, I will share a couple of blunders that have happened here at Carbon. Mistakes from myself and a maybe a colleague or two, that got past QA, but could have been prevented had a thorough review been done on the code at PR stage.
I’ll leave you with what I think is a very good article on code review, shared by a former Principal engineer — https://engineering.squarespace.com/blog/2019/code-review-culture-part-1. Have a good read.
