Pull request reviews. The term either fills you with something approaching joy, or, more likely, dread. In my experience, PR reviews are one of the best ways to ensure code quality, catch bugs, help engineers learn about the codebase. But they have to be handled with a bit of intentionality.
If you’ve ever worked on a growing engineering team, you might recognize this story. PR reviews start off fine, but as the codebase grows, suddenly one or two people are buried under a mountain of reviews while everyone else is waiting for days to get their code reviewed. Reviewers are stressed, PRs get stale, and no one is happy.
I humbly suggest a simple rotation that I’ve employed for nearly three years.
Basics of the rotation
Each week, one engineer is the primary reviewer, another is the backup.
PR authors assign their code to be reviewed by those two.
If you happen to open a PR the same week you’re primary, the backup covers your PR.
Every Monday morning, the baton passes to the next pair, and the change is announced to the team. 1
The schedule is kept in a public document 2 and assignments can be traded to account for vacations, illnesses, etc.
One or two approvals
Small changes? One review is fine. Bigger, riskier ones? Grab two. The idea is proportionality We’re not dogmatic about it, but the idea is to get more eyes on changes that could have big negative consequences.
If one of the designated reviewers is unavailable, it’s the authors responsibility to grab another teammate for the second review. The process should serve the team, not the other way around.
What to review
What to review deserves it’s own essay, but briefly, these are things I ask my team to review:
Functionality: Use test environments to test the basic functionality.
Code Quality: Check for clean, readable, and maintainable code. 3
Variable and Method Naming: Make sure variable and method names are clear.
Performance: Look for potential performance catastrophes. (That nested loop over API calls. The query that’s going to inadvertently scan the entire database.)
Security: Identify any security vulnerabilities or potential risks.
Testing: Ensure appropriate unit or integration tests are included. The goal isn’t 100% coverage, but critical paths should be tested so we can make future updates with confidence.
Documentation: Check that any tricky code is well-commented. If you had to think twice to understand a bit of code, future developers will too.
What shouldn’t you review? Style. If something works and is logically written, don’t block a PR from being merged just because it isn’t exactly like you would have written it. 4
Lessons learned
It’s not all wine and roses, some weeks are heavier on reviews than others. Reviewers might have a tough time getting to their scheduled reviews quickly if the rest of their workload is heavy. But I still think this simple system has had a positive effect on the team. PR authors know exactly who to reach out to for reviews, reviewers don’t get overwhelmed, and knowledge about the codebase grows naturally on the team.
We’re still tuning the rotation, but it’s the first system that’s made PR reviews feel less like a chore and more like a shared responsibility.
I went overboard and wrote a bot that automatically posts the rotation change each Monday. ↩
I used AI to generate the rotation about 6 months out. AI can also help slot in new teammates as your team grows and changes. ↩
If there are style tweaks I want to suggest, I’ll put them in my review, but I’ll grant the approval. Then it’s up to the PR author if they want to adopt my suggestions or not. ↩
After years of writing code, reviewing pull requests, debugging tricky production issues, I’ve developed a few opinions on how to write maintainable software. I’m not going to claim that any of these ideas are particularly revolutionary, but I wanted a document that outlines the principles I use for my records and as a resource for those who work for and with me on projects.
1. Names matter more than you think
The first principle I follow is using descriptive names for variables and methods. It sounds obvious, but the difference between h and hoursSinceLastLogin can mean the difference between a one minute code review and ten minutes of confusion. When you write getUserAccountDetails() instead of getUAD(), sure it’s a few more key strokes today, however future developers (including yourself a few months from now) will thank you for making the code simple to understand.
2. Simple is better than clever
Building on principle one, you should strive for simplicity in your code over cleverness. Sure, it’s impressive that you can write a complex data transformation as a single line with multiple chained array methods, and nested ternaries. But you probably shouldn’t.
This isn’t about dumbing down code, it’s about being aware that maintainability often trumps elegance when you’re writing software with a team. The cleverest code in the world is a liability if your teammates can’t confidently debug or modify it in the future.
Side note: I’m particularly cautious about functional patterns in languages that weren’t designed for them (like JavaScript). I appreciate the elegance of functional programming and have used it where appropriate, but, in my experience, many engineers you work with won’t be comfortable with functional concepts like currying, function composition, or monads. Write software for the team and language you have, and everyone will likely be more productive for it.
3. Keep functions short
Functions should ideally do one thing, and that thing should be in the name (see principle #1). I’ve written my fair share of screen long functions, and they often turn into a future source of pain. Smaller functions tend to be easier to read, test, refactor, and reuse.
A function called validateInput() shouldn’t also send an email, update the database, and turn on your coffee maker. Each of those things should likely be their own functions. This separation makes your code more readable and might even prevent bugs that come from overlooked side effects.
4. Stay local if you can
My last principle is about code organization. I prefer to keep types and utility functions in the same file as modules or components until they need to be shared.
Too often I see projects where every type lives in a types directory and every utility function lives in a utils directory, even when these pieces are only used by a single component. This kind of premature optimization creates unnecessary cognitive overhead. Instead of scrolling through a single file from top to bottom, now I’m switching between a handful of tabs. It makes creating a mental model needlessly difficult.
Code isn’t just for machines
These principles all come from a single maxim, that code is primarily for humans, not computers. The computer doesn’t care if your variable name is descriptive. The computer doesn’t care if your function is five lines or five hundred. But your teammates will care. Future you will care.
I recently ran into an issue where the Intercom chat widget was interfering with my Playwright integration tests. The widget was opening up and keeping some key elements from being clickable. A user could have easily dismissed the widget, but I didn’t want to bother dismissing it within Playwright. I couldn’t find any particular guidance on how to keep the widget from loading in the usual places on the web, so I had to come up with my own solution.
What I ended up doing was intercepting all outbound requests Playwright was making and skipping any that called the Intercom domain.
And then called it in the test files that were affected:
import{Page,test}from'@playwright/test'import{blockIntercom}from'helpers/block-intercom'letpage:Pagetest.beforeAll(async ({browser})=>{page=awaitbrowser.newPage()blockIntercom(page)})// your tests begin here
It shouldn’t be too difficult to modify this to also block chat widgets from other vendors like Zendesk or Drift.
I recently finished a book by Rolf Dobelli titled Stop Reading the News. Dobelli makes a compelling case to switch from daily news consumption to reviewing news once a week or not at all. As Dobelli argues that there’s not a lot you can do about the news literally as it happens, so why amp up your anxiety by consuming it every single day?
Now, I’m not ready to totally abandon the news — especially these days with COVID-19 raging in my home country — so I’m trying out the once a week strategy with a subscription to the print edition of The Economist. They give you the week’s news in a relatively condensed format that you can read in one sitting. Also, since it’s the print edition, I’m not getting lost following links all around the web. I’m also keeping tabs on COVID-19 cases in my area with tools like then Google News Coronavirus map, and as Dobelli suggests, I still read news about tech and development that have an impact on my career.
Stop Reading the News is a short book and I’d suggest giving it a read if you’re at all interested in understanding Dobelli’s reasoning for cutting back on the news and strategies for how to replace news in your life.
Sites that are served statically (meaning they aren’t backed by a scripting language or a database) have big advantages in terms of speed, simplicity, and security. But as Leon Paternoster notes, the definition of static sites is changing, and we’re losing speed and simplicity in the process.
Under the traditional static model, the heavy lifting of building pages from includes and local or external data is done when the website is compiled into flat HTML files, whether that’s on a PC or a server. This happens out of view (hence Jekyll, incidentally), completely separately from any user involvement. Javascript is used to enhance UI, perhaps through offering sorting or filtering functions. All the user does is download the HTML file and its assets.
Under a newer model (which even has its own Netlify-created brand name of JAMstack) much of this heavy lifting is moved to the user’s browser. Websites are created as SPAs, where HTML, CSS, data and javascript are downloaded in one bundle and the javascript creates pages based on user interaction.
The issue with a “newer model” static site is that the JavaScript required to render a page is often so large and complex that it can take a significant amount of time to download and then execute on a device. That could be a recipe for frustration when visitors have spotty internet connections or are using relatively slow phones or laptops.
