Old code deserves a little respect, even when it is bad. It has been running in production for years and has already met all the strange inputs you have not thought of yet. Read it carefully before rewriting it, because the odd conditional in the middle is usually a scar from a real incident.
Write the failing case down before you change anything. It can be a test, a curl command or a sentence in the ticket. The point is that you have something outside your own head that says what wrong looks like, because your memory of the bug starts drifting the moment you begin working on it.
One useful habit is to ask what would have told us sooner. Not who should have caught it, but what signal already existed and was not being looked at. Usually there was a log line or a metric sitting there the whole time, and a small alert costs far less than the outage did.
What it cost
type Result<T> = { ok: true; value: T } | { ok: false; error: string };
export function parse(input: unknown): Result<number> {
if (typeof input !== 'number') {
return { ok: false, error: 'not a number' };
}
return { ok: true, value: input };
}
Nobody reads the documentation until something is on fire, so write it for that reader. Put the command they need at the top. Say what it changes and how to undo it. Leave the history of how we got here for a later section, where it can be appreciated at leisure.
Keep the change small enough to explain in one sentence. If the description needs the word and, it is probably two changes, and the second one will be the one that causes trouble. Reviewers can hold one idea in their heads at a time, and so, at three in the morning, can you.
Naming turned out to matter more than the structure did. Once the function stopped being called process and started saying what it did, two of the three bugs in the surrounding code became obvious to everybody reading it, including the person who had written them, who was me.
What to watch for
no language here, so this block gets line numbers but no colors
The fix itself took four lines. Finding out where those four lines belonged took most of a week, including one afternoon spent proving that the bug was not where everyone, including me, had been absolutely certain it was. That is a normal ratio, and it is worth telling new people about.
Do the rename in its own commit. It feels like extra ceremony right up until someone tries to find out when the behavior changed and the history shows a single enormous diff. Separating the mechanical change from the interesting one costs thirty seconds and saves a very long afternoon.
There is a version of this that uses a library, and for a larger team it might be the right call. We looked at it, read the changelog and counted the dependencies. For the handful of cases we actually have, thirty lines we understand beat three thousand we would have to trust.
What we changed afterwards
type Result<T> = { ok: true; value: T } | { ok: false; error: string };
export function parse(input: unknown): Result<number> {
if (typeof input !== 'number') {
return { ok: false, error: 'not a number' };
}
return { ok: true, value: input };
}
Every layer in this stack is caching something, and each one was added by a reasonable person solving a real problem. The trouble only starts when nobody remembers all of them. Before adding another, draw the whole path on paper and write down who clears each cache and when it happens.
Performance work is mostly subtraction. Before reaching for a clever technique, find out what the page is doing that nobody asked it to do. We have never yet profiled something without finding a piece of work that could simply be removed, and removal has the best cost profile of anything.
We now write a short note after every incident, and the rule is that it has to be useful to somebody who was asleep at the time. What happened, how we noticed, what we did and what we changed. Blame is not one of the headings, and it has never needed to be.
Why it stayed broken
The number looked great on the dashboard, which should have been the first warning. It turned out we were averaging across every request, including the health checks, which are very fast and very frequent. Once they were excluded the graph told a much less flattering and far more useful story.
Choosing boring technology is mostly about choosing where your surprises will come from. The database has been around for decades and its failure modes are written up in great detail. The newer thing may well be better, but its surprises will belong entirely to you, at a time of its choosing.
Staging will lie to you, gently and often. It has less data, less traffic and a much more forgiving network. Treat a clean run there as a sign the change is not obviously broken, and keep watching the real thing for a while after it ships, with one hand near the rollback.
Did you try this with a cold cache? We saw very different numbers on the first request.
I would buy that mug.
Small correction: the default changed in a recent release, so check which version you are on.
I would buy that mug.
How long did the migration take on the full table?
The run-it-twice question should be printed on a mug.
Fair point. It depends a lot on team size.
Bookmarked for the next time someone proposes rewriting it from scratch.
I would buy that mug.
The run-it-twice question should be printed on a mug.