• 25 May 2024

The older I get, the more I dislike clever code. This is not a controversial take; it is pretty-well agreed upon that clever code is bad.

But I particularly like the on-call responsiblity framing: write code that you can understand when you get paged at 2am.

If you have never been lucky enough to get paged a 2am, I’ll paint the picture for you:

A critical part of the app is down. Your phone starts dinging on your nightstand next to you. You wake up with a start, not quite sure who you are or where you are. You put on your glasses and squint at the way-too-bright screen of your phone. It’s PagerDuty. "Oh shit," you think. You pop open your laptop, open the PagerDuty web app, and read the alert. You go to your telemetry and logging systems and figure out approximate whereabouts in the codebase the issue is.

You open your IDE and start sweating: "I have no idea what the hell any of this code means." The git blame shows you wrote the code 2 years ago. You thought that abstraction was pretty clever at the time, but now you’re paying a price: your code is inscrutable to an exhausted, stressed version of yourself who just wants to get the app back online.

Reasons for clever code #

There are a few reasons for clever code that I have seen over my career.

Thinking clever code is inherently good #

I think at some point a lot of engineers end up in a place where they become very skilled in a language before they understand the importance of writing clean, readable code. Consider the following two javascript snippets:

snippet 1

const sum = items.reduce(
(acc, el) => (typeof el === "number" ? acc + el : acc),
0
);

snippet 2

let sum = 0;

for (const item of items) {
if (typeof item === "number") {
sum = sum + item;
}
}

At one point in my career, I would have assumed the first snippet was superior: fewer lines and uses the reduce method! But I promise far more engineers can very quickly and easily understand what’s going on in the second snippet. I would much rather the second snippet in my codebase any day.

Premature abstraction #

Premature abstractions tend to be pretty common in object-oriented languages. This stackexchange answer made me laugh quite a bit, so I’ll use it as an example. Let’s say you have a system with employee information. Well perhaps you decide employees are types of humans, so we’d better have a human class, and humans are a type of mammal, so we’d better have a mammal class, and so on. All of a sudden, you might have to navigate several layers up to the animal class to see an employee’s properties and methods.

As the stackexchange answer succinctly put it:

As a result, we ended up with code that really only needed to deal with, say, records of employees, but were carefully written to be ready if you ever hired an arachnid or maybe a crustacean.

DRY dogma #

Don’t Repeat Yourself (DRY) is a coding philosophy where you try to minimize the amount of code repeated in your software. In theory, even repeating code once results in an increased chance that you’ll miss updating the code in both places or having inconsistent behavior when you have to implement the code somewhere else.

In practice, DRYing up code can sometimes be complex. Perhaps there is a little repeated code shared between client and server. Do we need to create a way to share this logic? If it’s only one small instance, it simply may not be worth the complexity of sharing logic. If this is going to be a common issue in the codebase, then perhaps centralizing the logic is worth it. But importantly we can’t just assume that one instance of repeated code means we must eliminate the redundancy.

What should we aim for instead? #

There’s definitely a balance to be struck. We can’t have purely dumb code with no abstractions: that ends up being pretty error prone. Imagine you’re working with an API that has some set of required headers. Forcing all engineers to remember to include those headers with every API call is error-prone.

file1

fetch("/api/users", {
headers: {
Authorization: `Bearer ${token}`,
AppVersion: version,
XsrfToken: xsrfToken,
},
});

fetch(`/api/users/${userId}`, {
headers: {
Authorization: `Bearer ${token}`,
AppVersion: version,
XsrfToken: xsrfToken,
},
});

file2

fetch("/api/transactions", {
headers: {
Authorization: `Bearer ${token}`,
AppVersion: version,
XsrfToken: xsrfToken,
},
});

file3

fetch("/api/settings", {
headers: {
Authorization: `Bearer ${token}`,
AppVersion: version,
XsrfToken: xsrfToken,
},
});

Furthermore, having to track down every instance of that API call to update the headers (or any other required info) could be challenging. In this instance, it makes a lot of sense to create some kind of API service that encapsulates the header logic:

service

function apiRequest(...args) {
const [url, headers, ...rest] = args;
return fetch(
url,
{
...headers,
Authorization: `Bearer ${token}`,
AppVersion: version,
XsrfToken: xsrfToken,
},
...rest
);
}

file1

apiRequest("/api/users");
apiRequest(`/api/users/${userId}`);

file2

apiRequest("/api/transactions");

file3

apiRequest("/api/settings");

The apiRequest function is a pretty helpful abstraction. It helps that it is a very minimal abstraction: just enough to prevent future engineers from making mistakes but not so much that it’s confusing.

These kinds of abstractions, however, can get out of hand. I have see code where making a request looks something like this:

const API_PATH = "api";
const USER_PATH = "user";
const TRANSACTIONS_PATH = "transactions";
const SETTINGS_PATH = "settings";

createRequest(
endpointGenerationFn,
[API_PATH, USER_PATH],
getHeaderOverrides("authenticated")
);

createRequest(
endpointGenerationFn,
[API_PATH, USER_PATH, userId],
getHeaderOverrides("authenticated")
);

There’s really no need for this. You’re not saving all that much for making variables instead of using strings for paths. In fact, this ends up making it really hard for someone debugging the code to search! Typically, I’d lok for the string "api/user" in my IDE to try to find the location of the request. Would I be able to find it with this abstraction? Would I be able to find it at 2am?

Furthermore, passing an endpoint-generation function that consumes the path parts seems like overkill and may be inscrutable to more junior engineers (or, again, 2am you).

Keep it as simple as possible #

So I think in the end my message is to keep your code as simple as possible. Don’t create some abstraction that may or may not be needed eventually. Weigh the maintenance value of DRYing up parts of your codebase versus readability.

Enjoy this post? Please subscribe to my RSS feed on Feedly or add my RSS XML file to another reader!

• Previous: The ChatGPT wrapper product boom is an uncanny valley hellscape
• Next: Generative AI will probably make blogs better