The Way I Approach Code Reviews

Setting the Scene

‍

A few months ago, my colleague Josh wrote an excellent blog post on what he most often looks out for when reviewing a pull request. The post is excellent, full of accessible information on how to write more performant code. Working on our platform engineering team, with its mission to ensure our users have the experience we intend them to, Josh is very concerned that the code we commit to our application is performant.

However, performance is just one facet of the code we work with at Cleo. Whilst it is something I've grown to increasingly care about over my five years at Cleo as (thanks to colleagues like Josh and many others) I've more deeply associated performance with the end-user experience, it's also certainly not my own personal forte, nor the thing I first look for when reviewing a PR.

With that in mind, I wanted to write a companion to Josh's original post on how I approach code reviews. Neither of our approaches to this is better or worse. The team is stronger for having both; a team of just Joshes or just Cassies will not be as effective as a team with both our perspectives (and many more besides). So, let's get into it.

‍

Readability

‍

We all know that professional software development is a team sport. The well-worn trope of the lone developer sheltered away from the world in a darkened room is long since dead. Writing readable code is therefore of paramount importance—which is also expressed via two of our engineering principles, both we help each other and we are considerate coders.

When reviewing my colleagues’ code this means all the usual things that you've read about in these kinds of blog posts before: well-named variables, methods that do not try to do too much and all the rest of it.

What it also means is paying attention to our own internal patterns and conventions. This is not an exact science, but rather a conversation—a dialogue. Engineers who have joined the company recently, but also tenured engineers, will inevitably have alternative coding styles and patterns. These are entirely valid in their own right, but might be novel to our codebase.

To give an example of this, I recently reviewed the PR of a colleague who joined Cleo recently, which contained the following code:

The above is perfectly valid Ruby syntax, and did exactly what the author of the PR in question wanted to achieve. However, the array & other_array syntax is not something we routinely use in our codebase. After the author of this PR explained to me that this was equivalent to array.select { |entry| entry.in?(other_array) }, I asked them if they’d mind tweaking the code to the latter to make it more consistent with the rest of our internal conventions.

Stylistic consistency can help with readability, so I often bias towards nudging people into adopting existing styles. Equally, this should not hinder us from adopting new patterns as a team where they can serve us.

To give a second example, I also recently pushed back to a colleague who was using Ruby 3’s new hash syntax. I’d biased away from using it previously, as I felt like it was a bit magic, and made code less immediately obvious where certain variables were coming from. However, in this particular instance my colleague countered that we should not be afraid of using new syntactic sugar in the language, and that did not make it ‘magic’. On reflection, I think my colleague was absolutely right—and the PR was merged as it was.

There's no right or wrong answer to this question. There's no rubric nor set of criteria that can produce a canonical answer for us every time. So, whenever I leave these kinds of comments on a PR, I caveat what I am saying by prepending the comment with something like (minor and stylistic). Or, I frame it as a question, attempting to make it clear that this is a suggestion rather than a visit from the code style police.

‍

Extensibility and Simplicity

‍

As a Technical Lead at Cleo, part of my role is to have an awareness and responsibility for the overall health of our systems. This means all the usual Good Software Engineering Stuff™️: modular code, reusable logic, and all the rest of it.

At Cleo, we also have a heavy focus on shipping new features and functionality that will deliver value to our users. We want to solve the problems that are in front of us right now rather than hypothetical problems that might exist a few years down the line. Our engineering principles tell us to do the simple thing (which we should) and that technical debt is useful (which it can be).

All of this can make it harder for teams to take a more heads up view of what's going on around them. Framing problem-solving as a choice between “what's in front of you right now” and “what we imagine might be an issue in a few years” is, of course, a false dichotomy—but it also can make it hard to find the sweet spot in between.

With an engineer's bias towards reusability on one side and a company push towards delivery on the other, finding the right balance is very much an art. So, in my code reviews, I find myself looking at solutions with an eye to whether something has been overengineered, or else where an existing pattern feels like it is no longer scaling. This not only helps ensure the overall health of the codebase, but also helps engineers get a feel for where the sweet spot in that tradeoff lies.

‍

Not Reinventing the Wheel

‍

When you hear words like “infrastructure” in the engineering world, it's very easy to think about performance, availability and uptime, redundancy, and other important technical concepts. “Infrastructure” also mean the bits of code we write that enable others to write their own code quickly. This code exists to make doing something well, make doing something simply and extensibly the easy default choice, rather than something people have to go out of their way to think about.

Over time at Cleo, however, we've also built out several unique-to-our-requirements bits of product infrastructure that are used throughout our system and designed to scale across it. This includes a framework to gate different levels of feature access to users, our own internal A/B testing framework, or tooling to allow writers to define their own audiences for receiving a certain experience.

When they work best (and often in concert), these subsystems allow squads to deliver new functionality simply, sometimes with little to no new code being written—because the problem they are seeking to solve has already been solved by someone else at the company, perhaps several times over.

Ensuring that squads are plugging into our engineering team's common infrastructure allows teams to deliver faster and allows them to reap the rewards for free of shared improvements to the sub-system. However, because they are unique to Cleo, engineers will not always have had exposure to how these work before. They are not google-able like the Rails documentation might be, and will not always be discoverable—you don't know what you don't know.

Having worked at Cleo for over five years now, I've had the opportunity to contribute large amounts to these sub-systems. Code reviews provide an excellent opportunity for suggesting that work plugs into or extends these systems where appropriate. They also enable us to share, update, or create the various bits of documentation associated with them.

‍

Trust and Unblocking

‍

As you've been reading through this post, you might have noticed a common theme emerging: very few of these are hard and fast rules, where one thing is categorically better than the other. They're very often suggestions and opinions—good ones, in my opinion, but suggestions and opinions nonetheless.

I'll try to communicate this by highlighting that comments are nitpicks or non-blocking. I'll often approve a PR (unless there are major issues that need addressing), leaving the decision to action my comments (or not) to the engineer who is implementing it.

At a more meta level, this blog post, which I'll share internally with my colleagues as well, is also an attempt to communicate this to my colleagues.

In this framing the code review is less a “review” per se, but instead an opportunity for knowledge sharing and a chance to think about and practise applying our engineering principles and company values.

‍

One of my favourite PR review interactions to date came when reviewing the code of a more junior colleague in a different squad to me.

They had just implemented a feature using the old version of a framework that I had recently been significantly involved in reworking. I left a comment asking her if she could rework the PR using The New Thing™️. Her response was to push back on this, as the new approach would have required more immediate work. However it also would have been more reusable, reducing engineering overhead over the long term. Her immediate priority was to unblock her team from launching a new A/B test, which this work depended on.

In doing so, and agreeing to come back to this after the test launch to refactor the code, she was able to model all five of our engineering principles. Do the simple thing (by doing the quick thing), technical debt is useful (by using the old approach to allow a test to be launched), being a considerate coder (by coming back to the code after the fact), we help each other (by sharing the knowledge of the new approach with other engineers in her team), and innovating in our product and not our tech stack (by prioritising launching a new test for our users over doing the Nice Engineering Thing).

‍

Summary

‍

There's no one right way to do a code review, and there are as many (or more) different approaches within Cleo as there are engineers in our team.

Indeed, code reviews are a microcosm of the needs of an engineering team as a whole; just as you need an app to be performant and scalable, you also need broad alignment amongst the members of the team working on it—and many other things besides.

The strength of an engineering (or any) team is not in its ability to excel at a single thing, but rather in the diversity of approaches and talents needed to balance a multitude of competing priorities. I'm looking forward to, hopefully, reading other blog posts in the future on different approaches to code reviews.

‍

If you're interested in joining the engineering team, check out our open roles.