Code That Works vs. Code That Is Maintainable

Lose 5 lbs in 5 seconds? Easy! Just cut off your arm.

Most of us are familiar with the concept of SMART goals: Specific, Measurable, Attainable, Realistic, and Timely. You might argue cutting off one’s arm isn’t really realistic, but by the typical SMART definition, it most certainly is. Therein lies the flaw in SMART goals for engineering; I’d argue Sustainable needs to be added to the mix. There is a difference between simply achieving your outcome vs. achieving it in a sustainable—or when it comes to code, maintainable—way.

Some of our best lessons come from internal Slack rants from our Engineers; this is a good one I shared after diving into old code.

Here's the old version of the code:

While the code does the job it needs to do—in the sense that it's not actively broken—it could've done with a little bit of polish in a few places that would've made a big difference in the readability and maintainability of it. Looking at old code is basically like looking at Elvish!

/*
* To do its job, the PDFer only needs -
* 1. resource_id/order_id
* 2. picasso.use_preflight feature flag
* However, UpdateOrder and Thumbnailer need more and therefore it gets passed
* more than what it needs. We'll have to refactor the flow to get past this
* undesirable situation. To minimize the spillover of larger than needed objects
* within this code, we use this method to return only whats needed.
* This should also help in refactoring later.
*/
function getWhatsRequired(msg) {
 const id = msg.resource.id;
 const required = {
   id,
   resource_type: msg.resource_type,
   receiptHandle: msg.receiptHandle,
   rawUri: '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '_raw.pdf',
   rawX1a: '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '_raw.pdfx',
   rawNonX1a:
     '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '_raw_non_x1a.pdf',
   proofUri: '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '.pdf',
   proofX1a: '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '.pdfx',
   proofNonX1a:
     '/' + id.substring(id.indexOf('_') + 1) + '/' + id + '_non_x1a.pdf',
   usePreflight:
     msg.launch_darkly_flags['picasso.use_preflight'] === 1 ? true : false,
   strictPreflight:
     msg.launch_darkly_flags['picasso.strict_preflight'] === 1 ? true : false,
 };
 return required;
}

The first problem

The name doesn't really tell me what's happening. `getsWhatsRequired` is a name that would apply to just about any function that gets and/or formats data. It doesn't tell me why this function lives on its own, nor what it is doing that's particularly useful.

The second problem

There's a whole whack of repetition taking place inside the URIs. This code is repeated 6 times inside this one function. Being repeated 6 times makes it harder to reason about because you either need to read each repetition and decide whether they're doing exactly the same thing OR you end up skipping over it and assuming it does exactly the same thing each time. In this case it is doing the same thing each of the six times, but you have to exert a lot of mental energy just to figure that out.

The third problem

Inside the repeated code there's an intention and goal that's being lost. We're not chopping up strings and adding slashes and stuff just for the heck of it, we're doing it with a specific purpose! But why? From this code, it's not clear at all.

The fourth problem

Those double equals? They're load-bearing! During some minor code cleanup one of my colleagues went and converted them to the === strict equality operator like ESLint suggested because there wasn't any reason to think that this needed to be a loose equality check.

Turns out, the values coming through weren't 1, they were actually "1". That's very different and this caused an incident because we weren't sending any files for a period of time.

There's one of two ways this can be improved on:

1. If the value coming through is always a string, then use the string representation instead with ===.

2. If the value is coming through either as a string or as a number and there's a really good reason for that to keep happening, then use the double equals and document this specific behavior so that Future You / your peer knows that you're depending on this very specific behavior.

And the fifth (and by far the nittiest of picks)

If you're going to define the variable and then immediately return it, you can just return that value immediately without assigning it to a variable first. It's a small detail but it makes the function shorter.

Here is the new code:

Most of the changes I've made above map pretty well to the opportunities I just mentioned, but there's one area I wanna spend a bit more time explaining:

/*
* Returns the expected file locations for each of the raws and proofs before and after preflighting.
*/
function getPathsAndInstructions(msg) {
 const id = msg.resource.id;
 const orderIdWithoutPrefix = id.substring(id.indexOf('_') + 1);
 const baseURL = '/' + orderIdWithoutPrefix + '/' + id;

 return {
   id,
   resource_type: msg.resource_type,
   receiptHandle: msg.receiptHandle,
   rawUri: baseURL + '_raw.pdf',
   rawX1a: baseURL + '_raw.pdfx',
   rawNonX1a: baseURL + '_raw_non_x1a.pdf',
   proofUri: baseURL + '.pdf',
   proofX1a: baseURL + '.pdfx',
   proofNonX1a: baseURL + '_non_x1a.pdf',
   usePreflight: msg.launch_darkly_flags['picasso.use_preflight'] === '1',
   strictPreflight: msg.launch_darkly_flags['picasso.strict_preflight'] === '1'
 };
}

These are the two lines I've introduced in place of the code that was repeated six times.
const orderIdWithoutPrefix = id.substring(id.indexOf('_') + 1);
const baseURL = '/' + orderIdWithoutPrefix + '/' + id;

You could easily argue that these two lines could be one line that reads like this:

That'd be even more space efficient, right? Right! That'd be 100% correct! And here's when context comes into play. You wouldn't know it by reading this code on its own, but all the order assets? They're uploaded into the order assets service, with the folder being the order ID without the prefix.

const baseURL = '/' + id.substring(id.indexOf('_') + 1) + '/' + id;

Knowing that the order ID without prefix has a specific meaning outside of this context, I think it reads better to separate the id prefix slicing from the path composition.

So having criticized this bit of code at length, I want to be real clear about a few things:

It is not the fault of the engineer that this code that was merged in was unpolished. Speaking on behalf of EMs and Staff Engineers, it's our responsibility to set the standards and support our colleagues in writing code that is well-polished and maintainable—and we failed to do so here. A valuable lesson learned.

Also, none of this applies to code you're not ready to merge. The code I write when I first start solving something is often very different from the code I end up with. Don't spend time polishing code you'll throw away in 15 minutes.

In response to my ranting, I received the following valuable advice from my colleagues, and I wholeheartedly agree:

“Instead of running into analysis paralysis when I'm trying to figure out the "best" way to write a chunk of code, I'll often just start writing the crappiest, brute-forciest, whatever-comes-to-mind, first-thing-that-works. Then I'll write my tests (if I didn't already write them using TDD, which I recommend of course). Now that I have a safety net that the code is doing what it needs to, now I can clean it up and feel confident that it still works.” –Erin Doyle, Staff Software Engineer

And I’ll leave you with a perfect summary of the above from Ross Martin, Engineering Manager:

‍“Make it work, make it right, make it fast—in that order."