Reflections on the Bad Code I've Written

Published on |阅读中文原文

I created the draft for this post in October 2017. Every time I got some of it written and considered publishing, it felt like something was still missing. Having now written code for four or five years across projects ranging from simple campaign pages to genuinely complicated business logic, it is time to reflect on the bad code I have written.

I have written a fair amount of code by now, and every time I look back at what I wrote one or two months ago it seems terrible. I used to think this meant I had not mastered the techniques of efficient programming — unit testing, breakpoint debugging, that sort of thing. After reading a few books and sitting with it, I came to a different view: avoiding bad code has little to do with how many techniques you know or how many language features you understand. It starts with how you think about the work of writing code.

What is bad code ​

Everyone's temperament, skill level and work history differ, so the standard for "good code versus bad code" differs too. There is a well-known image about judging code quality in review that captures the problem:

Muddled logic, needlessly complex ​

Poor readability, mostly:

  • Magic numbers and bizarre variable names.
  • No comments, or comments so vague they say the wrong thing — and when you ask, it turns out someone forgot to update them.
  • Chaotic structure: piles of conditionals, spaghetti flow, nested callbacks.
  • Redundant, convoluted implementations. Showing off four ways to write the same thing does not improve the system.
  • Verbosity born of limited skill, or of not knowing the libraries and frameworks already in the project and hand-rolling a more complicated version of something that already existed.

That was essentially my first year. jQuery was still popular and pages were full of $xxx.parent.parent.find(xx).nextSiblings. Code exists to describe logic, and muddled code is where bugs come from.

With modern IDEs and linters, most of the truly bizarre constructions can be prevented and style can at least be made consistent.

Not robust, hard to test ​

We cannot predict how a product will change or how users will use it, and wherever the program fails to cover a case, a bug can appear:

  • Unconsidered edge cases and external-dependency failures, producing errors and crashes every few days.
  • Fix one bug, introduce ten.
  • No error handling, so bugs are hard to locate.

The most basic form of error handling is try...catch, and I have seen it misused plenty of times in legacy projects.

try...catch is not for swallowing errors. It is for keeping the system from collapsing on a case you failed to cover. No visible error does not mean no error — quite the opposite. Silently catching an error means you never learn what is actually wrong with the system.

One measure of robustness is unit test coverage: if you are afraid of bugs, make them appear early, and the simplest way to do that is to test.

The benefits of unit testing are well known, but across the projects I have inherited — apart from a few utility and component libraries — test cases were rare. Even on projects I own today, tests tend to get backfilled afterwards, and coverage is embarrassing.

Partly that is schedule pressure: no time to write tests up front.

Partly it is that a lot of legacy code depends on globals and external state, which makes it hard to test at all:

js
// To test this you also have to mock the external `name`
function test() {
  return `hello ${name}`
}

// Whereas a function whose logic depends only on its parameters is trivial to test
function test2(name) {
  return `hello ${name}`
}

The simplest heuristic: the more dependencies, the harder the test. Which requires good module boundaries and no circular dependencies — a different topic, namely how to layer a codebase.

Hard to maintain ​

"Maintenance" covers both fixing old code and adding new features, so "hard to maintain" means both are hard. Every characteristic of bad code ultimately expresses itself as difficulty of maintenance. Code that is easy to maintain has to satisfy roughly:

  • A clear flow, so that a maintainer can follow it either backwards from the result (say, the rendered view) or forwards from the entry point.
  • Easy to find the place to change. Hunting for one if inside a thousand-line function is painful just to think about.
  • The maintainer can clearly assess the scope of the change, with nothing missed.
  • The maintainer can clearly assess the impact of the change, with no surprises.
  • Consistent style, preserved after each modification.
  • Easy to extend.

Most code is clear in its first version. As features iterate and requirements shift, it drifts from the original design and ends up bad. Why does my code gradually rot? That is the question this post is really about.

There is a joke about product managers that I heard early on: "this requirement is simple, how you implement it is your problem."

From the developer's side: the PM does not understand the code, does not know that this part was not designed that way, and thinks the feature only needs X while I have to change a dozen places and regression-test all of it.

From the PM's side: I know this product better than anyone, and given the existing design this change is logically consistent and should be quick.

So where is the actual problem?

Recently I finished a feature and demoed the whole flow to the PM on my local environment, and something struck me: why did development take a day when the demo takes two or three minutes? This was not one of those features where the demo is two seconds ("type a URL, see the page") and the implementation is enormous.

I think I see it now. For that requirement:

  • Development took a day. It genuinely was fiddly.
  • The demo took two minutes and involved no significant change. It genuinely was simple.

So why does a requirement the PM reads as simple take so long to implement? Is the code's design out of alignment with the product's design?

Turn it around and the question becomes: why is the code we write so troublesome to maintain? Is there a way of developing that fully mirrors the product design, so that features can be added and changed easily?

The habit of structured programming ​

When I was learning to code, not knowing how to approach a problem made me anxious enough that I read books like Think Like a Programmer (a decent book, for what it is worth). Eventually I learned the simplest method: work out what the code should do, write this first, then that, done.

Programming logic is a description of the concrete steps for satisfying some product requirement, with conditions and loops along the way until the goal is reached.

At one point in software history, as requirements grew more complex, structured programming was proposed: express logic using only sequence, selection and repetition, and abandon goto. It was a great advance — those three structures cover the overwhelming majority of logic.

Structured programming implies that:

  • To follow a piece of logic you read from the entry point.
  • To add or remove a feature you locate it in that structural order and modify in place.
  • Every modification affects the structure.

Real business flows can be long, spanning projects and maintained by many people — client → server → RPC service → server → client is routine. The code in front of you is the tip of an iceberg, which makes following the whole flow very hard, and leaves you unable to see the mountain for standing on it.

Here is "cooking mapo tofu" as pseudocode:

text
prepareTofuAndBeanPaste()
turnOnHeat()
addOil()
stirFry()
plateIt()

The flow looks clear. Now add some conditions — no ingredients, and seasoning adjustment:

text
prepareTofuAndBeanPaste()
+ if noIngredients then buyIngredients()
turnOnHeat()
addOil()
stirFry()

while tasteIsOff:
+ if tooBland then addSalt()
+ elif tooSalty then addTofu()
+ stirFry()

plateIt()

Still just about readable — until we start adding special cases:

text
# Special handling for Xiaoming
+ if isXiaoming then prepareTofuAndPixianBeanPaste()
+ else prepareTofuAndBeanPaste()

As those accumulate, the original linear structure drowns in branches. The code still satisfies the requirements, but each change takes longer to locate than the last, and the effect of this change compounds into the next.

That is why a feature that started out clean becomes hard to maintain after a steady drip of new features and special cases.

Accumulated technical debt ​

Every piece of code was written under specific conditions: maybe with a comfortable schedule and thorough testing; maybe as a hack under pressure to fix one urgent problem; maybe in a good mood, maybe not. I believe almost everyone in this field has basic professional ethics — in my career I have never seen someone deliberately write bugs out of spite.

But for one reason or another, there are times when you know the code is not elegant, and it satisfies the requirement, so you commit it, and at most leave a TODO for peace of mind. That is a debt taken on, and technical debt is hard to repay. The code sits quietly in some commit waiting to be refactored or retired.

For long-standing debt, if fear of breaking the system keeps bad code alive, the balance only grows.

"If I do not touch it I cannot break it — it runs fine, so why change it?" That attitude also shapes how willing we are to repay.

I have never much minded modifying old code — I feel something like shame about bad code I wrote myself and want to fix it on sight. But personal capacity is finite, and some debt costs a lot of time while changing nothing for the business.

So most business code accumulates debt over time, grows, becomes harder to extend and easier to break, until the only remaining option is "rewrite" — and rewrites are not a cure-all, and often die for reasons of headcount, time or unclear return.

I remember seeing the idea of an architecture under which code improves as it is maintained. On technical debt specifically, though, relying on architecture alone to stop engineers from accruing debt seems unlikely to work. The best available option is probably to hope your predecessors left few traps, and to hold yourself to leaving few for the next person.

Trusting old code ​

The clearest code in the world is the code you just wrote. The worst code in the world is the code you wrote a year ago.

"Who wrote this garbage?" is a routine complaint, as though authors and maintainers were natural adversaries — though it is often you-a-year-ago versus you-now.

Distrust of historical code also degrades the design; it is another kick to an already unsteady doorframe. DRY is obviously right and we treat it as doctrine and despise duplication. Yet when maintaining an inherited project we are often afraid to reuse existing code:

  • The old code is incomprehensible and unmaintainable, so write a new one.
  • Nobody knows what depends on this, so rather than risk breaking something, write a new one.

And then you add a pile of code that at least you understand. If someone maintains it after you, in all likelihood they will not trust yours either, and around it goes.

Where does that missing trust come from? Mostly from no longer knowing the circumstances the old code was written under.

From the maintainer's side, if the relevant logic is unclear or forgotten, then short of studying it from scratch it is very hard to reconstruct the context. I once rewrote a chunk of old code I considered terrible, and near the end realised "oh — it was written that way because of X", and rolled the whole thing back.

Tests seem to be an effective way to sustain that trust: with tests you can change something and see what fails, which localises the impact of your change (if coverage is high enough). Unfortunately a lot of legacy code has none.

CSS is not really a programming language, but it illustrates the distrust well.

Before CSS Modules and scoped styles, all styles were global. To add a .title class, you had to search the historical stylesheets to see whether .title already existed, or risk a collision or breaking something else.

To save effort, we lean on specificity to override — add !important, or add another selector like .xxx .title. Which is a large part of why conventions like BEM emerged.

z-index has the same trust problem: to keep my modal from being affected by some style in a forgotten corner, write 9999... with as many nines as it takes and let the next person deal with it.

Encapsulation aimed at the wrong thing ​

We cannot foresee how code will change, but we can write code that is easier to maintain afterwards. How do you judge "easy to maintain" from the maintainer's point of view?

For a long time I believed: fewer places to change means easier to maintain. Acting on that, I made a lot of deliberate attempts:

  • Reduce duplicated values — manage globals through config files.
  • Reduce duplicated code — extract functions, extract modules.
  • Reduce duplicated logic — extract components.

The best way to minimise changes is to encapsulate the shared logic, and the core idea of encapsulation is to isolate what changes from what stays stable. In theory, encapsulation:

  • Physically groups code with the same responsibility, making it easy to find, so later changes touch less code.
  • Gives you "behind every elegant interface is a dirty implementation" — the maintainer writes good code without caring about the dirty part.
  • Reduces globals and free variables, which makes testing easier.

I understood encapsulation as being for reuse. What I found later was that it quietly turns against maintainability: you keep having to modify code you had already encapsulated — one more parameter on the function, a few more methods exposed from the module, a few more props on the component, an extra conditional. Eventually "few places changed" stops implying "few places affected": some code moves everything when you pull one thread, forcing changes in places you never anticipated.

So clumsy encapsulation is itself a cause of poor maintainability. Some of the problems I have run into:

Forced encapsulation ​

Say we need a product component. Two approaches:

  • Accept some query conditions, have the component fetch the product and then render it — the component owns both fetching and display.
  • Accept a product object, and let the caller fetch and pass it in — the component only displays.

For the sake of reuse I would most likely pick the first and encapsulate everything that looks general. Then, for the cases that need to pass a product directly, expose another parameter, and skip the fetch when it is present.

That instinct shaped how I practised encapsulation: forcing seemingly-repeated logic (fetch a request, handle the response) into one place while ignoring how the business varies — and then adding if...else in each scenario to accommodate its specifics.

What I eventually understood is that encapsulation is not about physically splitting code into different functions, classes or files. It is about conceptually defining good inputs and outputs.

In the example above, the display logic is what stays constant; what varies is how the product is obtained. So the variation should be lifted out of the constant part.

For any code you intend to encapsulate, you have to think about where the variation comes from — find the variation first, and only then can you decide what belongs together. But we cannot anticipate every business change, nor guarantee that today's shared logic will not vary tomorrow. So which logic should be encapsulated together?

The change is too cheap ​

When using a framework, if a feature is awkward to build, we think about how to build it — not about how to modify the framework's internals to suit us.

A concrete example. In mobile development, rem is often used for screen adaptation, and to avoid computing rem by hand the PostCSS community offers plugins like postcss-px2rem, which converts px in stylesheets to rem automatically.

Sometimes you want certain files left alone, which is what postcss-px2rem-exclude is for — it takes an exclude parameter to skip files.

But if you need most px in the same stylesheet converted and a few preserved (borders, say), exclude no longer helps. One hack is to write PX in uppercase instead of px.

Which raises another problem: quick-format in IDEs like WebStorm may normalise PX back to px, breaking the hack. A hack to preserve the hack is a Sass @function:

scss
// util.scss — do NOT run quick-format on this file!
// Return the raw pixel unit
@function PX($px) {
  @return #{$px}PX;
}

Tooling can never cover every case, but we find ways to extend it from the outside rather than thinking about modifying postcss-px2rem to add the feature.

So why, inside our own projects, do we casually modify code we already encapsulated? Add a parameter here, an if there?

Partly for the reason above: we encapsulated business logic that was going to vary, which invites us to modify the encapsulated code.

Partly because we wrote it ourselves. Unlike framework or library code, which has natural isolation (in frontend projects, it lives in node_modules), our own encapsulated code carries no such barrier, and the linear habit from structured programming makes reaching in and editing it the reflexive move. Encapsulation is easier to break as a result.

Is there any way to constrain ourselves, or raise the cost of the change?

The simplest is single responsibility. If a piece of code has no reason to change, we are not going to keep wanting to change it.

The Single Responsibility Principle requires that code have exactly one reason to change. A class with multiple reasons to change has multiple responsibilities. If there are two or more reasons to modify one place — a class, an object, a method — that code violates single responsibility.

Breaking the encapsulation ​

The point of encapsulation is to isolate what varies and wrap up what does not, which gives us the ability to change things quickly: modify one place and every dependent is affected. That is very tempting when adding a general feature, and during fast iteration the temptation usually wins.

Under that pressure we are likely to break what the encapsulation was for, dragging odd extra features inside until the encapsulated logic is no longer general.

Fundamentally, we failed to separate responsibilities clearly and encapsulated possible variation along with everything else, which is exactly what invites us to modify the encapsulated part.

Suppose there is a pure UI component. It takes a specific config structure and renders it. Ten pages use it.

A new requirement arrives: clicking the component must report analytics. Two options:

  • Register a click handler on each page that uses the component and handle reporting there.
  • Changing ten pages is a pain — luckily it is a shared component, so handle reporting inside it.

Think for a moment about which you would choose.

Say we take the second. Obviously this requirement is trivial: a day was estimated, half an hour was spent, the rest is free time.

Change: we added analytics reporting inside a UI component.

The component now has two responsibilities, UI and analytics. Anyone who wants it purely for display silently gets reporting too, which may not be what they want.

We could add another prop, needReport, to control it. Clearly not a great answer.

Change: we added a needReport prop to control reporting.

Now suppose fifteen places use the component: ten need reporting, five only need display.

Under the current design we pass :needReport="true" in ten places and :needReport="false" in five. Default values can omit some of them, but there is no doubt: we have broken the component's generality. It is no longer a general component — a user now has to know which features it has and which parameters control them.

The original intent was a component that takes a config and renders it. How did it get here?

Setting aside the chronological accident that the UI component existed before the analytics requirement: we forced apparently-repeated code into one place, and then, to satisfy each site's specifics, added more and more parameters and conditionals.

When modifying old code, understand first why the change arose. Only then can you decide where the change belongs and whether it is reasonable at all. Adding a new feature inside old code affects the old code and constrains the new.

Suppose we could rewind and choose the first option — do not touch the old code, write the reporting separately at each of the call sites. That is clearly not right either.

For dynamically attaching behaviour unrelated to a component's own logic, a decorator may be the answer. What about a higher-order component that combines display and reporting?

There is no problem that cannot be solved by adding a layer of indirection. If there is, add another layer.

Ways to extend behaviour without modifying the original:

  • Inheritance — extend a subclass without modifying the parent.
  • Mixins — extend an object's methods directly, but with several mixins in play it becomes hard to trace where a method came from; like mixing water and ink, separating them again is difficult.
  • Decorators or interceptors — invoked before or after the logic, easy to attach and detach, but unable to modify the logic in the middle.

Over-abstraction has its own costs, of course: you have to descend layer by layer before you know what a component actually does.

Closing ​

At this point I no longer write the truly beginner-grade bad code — globals defined at random, large blocks of duplication — and my throughput is acceptable enough that requirements do not usually slip. Over the past few years I have worked through SOLID and read Code Complete, Design Patterns and Refactoring, trying to apply them. And still, looking back at my own old code, there is a trace of embarrassment and some worry about what others will say about it. I know elegant code matters, but real development is full of trade-offs. Does genuinely elegant code exist?

I have come to feel the weight of "no silver bullet" and no longer chase perfect code. Code exists to serve the business, and meeting the business need matters far more than writing something "elegant". But for anyone with some standards, writing a little less code that others will look down on is worth the effort — writing code is a genuinely interesting thing to do.

The goal, as far as I can manage it: write a little less bad code.

References ​

你要请我喝一杯奶茶？

版权声明：自由转载-非商用-保持署名和原文链接。

本站文章均为本人原创，参考文章我都会在文中进行声明，也请您转载时附上署名。