Bad Abstractions Could Be Ruining Your Code

Introduction

Let’s imagine we’re working in a very large codebase. Can you spot an issue in the following code?

const icons = {
  delete: getIconPath("delete"),
  edit: getIconPath("edit"),
  save: getIconPath("save"),
};

The code is easy to read and it runs fine — and no, the problem isn’t that it’s written in JavaScript!

The issue is that in a large codebase words like delete, edit and save are likely to appear often. Finding one of these icons could be slower than necessary, as we have to sort through needless noise; we’re making it harder for maintainers to find what they need quickly.

There’s an easier way, and that’s to get rid of our getIconPath helper, removing the abstraction layer and leaving us with this:

const icons = [
  "delete": "public/static/icons/delete.svg",
  "edit": "public/static/icons/edit.svg",
  "save": "public/static/icons/save.svg",
]

Now, we have more useful, searchable information right where you need it. If you’re looking at a particular asset, you can easily find out exactly where it’s being used. Conversely, if you’re already working in this part of the codebase and want to know where the assets are stored, it’s easy to find them too.

This may seem like a simple — even trivial — example, but extra friction created by patterns like these has a compounding effect in a large codebase. With good intentions of reducing repetition, we’ve ended up with code that’s actually harder to maintain or extend than the less abstract code it is replacing.

Take, for example, this React code:

const PasswordSettings = () => {
  const t = useTranslation();
  const getTranslation = (key) => t(`${settingsKey}.${componentKey}.${key}`);

  return (
    <>
      <h1>{getTranslation("heading")}</h1>
      <p>{getTranslation("description")}</p>
    </>
  );
};

Once again, we have replaced something unique and easy-to-find with something much less useful — something that simply isn’t worth the characters we’ve saved. We’re denying ourselves the ability to search for a unique key like settings.password.heading and immediately find out exactly the places where it is used. Instead, we may be forced to search for heading or password — terms which are unlikely to narrow things down for us. We’re spending more time wading through files and less time making meaningful changes to our code.

The situation improves when we remove the abstraction:

const PasswordSettings = () => {
  const t = useTranslation();

  return (
    <>
      <h1>{t("settings.password.heading")}</h1>
      <p>{t("settings.password.description")}</p>
    </>
  );
};

Avoiding layers of indirection like these makes our code easier to maintain — in this case, helping us to find every instance of a particular translation in our code.

I believe these patterns appear so often partly because of the popularity of the DRY principle — DRY meaning ‘don’t repeat yourself’. This is not a bad principle in programming sometimes. But as with most principles in programming, context is important. The attempt of some developers to reduce repetition in their code to a bare minimum is harming the ease with which their code can be read and maintained.

Another type of bad abstraction is abstracting too soon. If you don’t fully understand the problem you’re trying solve, you can create a pattern that makes it more difficult to work with.

I’ll demonstrate with a further React example. Let’s imagine we’re building multi-step forms. We’ve got two forms right now, but we know we’ll need more.

const Form1 = () => {
  return (
    <>
      <Step1 title="Form #1" />
      <Step2 />
      <Step3 />
    </>
  );
};

const Form2 = () => {
  return (
    <>
      <Step1 title="Form #2" />
      <Step2 />
      <Step3 />
    </>
  );
};

This code is readable, but there’s a fair bit of repetition. Seeing how similar these forms are, we might be inspired to create a generic Form component like so.

const Form = ({ title }) => {
  return (
    <>
      <Step1 title={title} />
      <Step2 />
      <Step3 />
    </>
  );
};

const Form1 = () => <Form title="Form #1" />;
const Form2 = () => <Form title="Form #2" />;

There’s nothing wrong with this pattern if our forms are always going to be this similar, since — if we’re right — we’ll save ourselves time and effort in the future.

But what if other forms are introduced, requiring additional steps or special routing logic? Let’s imagine we now need a third form which skips the first step, has a new fourth step, and requires some special logic: if step three fails it needs to return to step two.

We can try to solve this by adding more props for these new requirements, but it quickly starts to become messy. We could, for example, end up with this.

const Form = ({ title, skipStep1, hasStep4, ifStep3FailsGoBackToStep2 }) => {
  return (
    <>
      {!skipStep1 && <Step1 title={title} />}
      <Step2 />
      <Step3 onFailGoToStep2={ifStep3FailsGoBackToStep2} />
      {hasStep4 && <Step4 />}
    </>
  );
};

const Form1 = () => <Form title="Form #1" />;
const Form2 = () => <Form title="Form #2" />;
const Form3 = () => <Form skipStep1 hasStep4 ifStep3FailsGoBackToStep2 />;

Eek. Already, we can see that our abstraction is not holding up particularly well. The Form component is much less readable, and we’ve needed to introduce several new props which seem to relate to this specific use case. In a real world application, our Form component is unlikely to be this simple, so these kinds of problems will be amplified.

Instead, if we revert to using our original definitions of Form1 and Form2, and we add an non-abstracted version of Form3, we get something easier to read and maintain.

const Form1 = () => {
  return (
    <>
      <Step1 title="Form #1" />
      <Step2 />
      <Step3 />
    </>
  );
};

const Form2 = () => {
  return (
    <>
      <Step1 title="Form #2" />
      <Step2 />
      <Step3 />
    </>
  );
};

const Form3 = () => {
  return (
    <>
      <Step2 />
      <Step3 onFailGoToStep2 />
      <Step4 />
    </>
  );
};

The key problem was that we didn’t understand the problem domain enough that our abstraction made sense. Developing with the abstraction became harder, not easier.

Note that, in the example above, we do have an abstraction that seems to be working well: we have been able to re-use our step components! My point is not that abstraction and the DRY principle are bad, just that their usefulness is context-dependent.

We work with abstractions all the time. Many of them make us more productive and even capable of doing things we couldn’t do otherwise. Most programming languages are several layers of abstraction above the binary code that your computer understands, and that’s a good thing; most of us wouldn’t be very productive trying to program in ones and zeroes. So, in a sense, choosing a language or library is an exercise in trying to choose the correct abstraction for the task at hand.

This comes with risks. The potential hazards of poor abstraction can be heightened when we choose tools that don’t quite fit our needs. At my company, we use the react-admin library for a couple of internal tools. This library makes development much faster for certain use cases. Outside of those use-cases, though, it can be less efficient to work with. It’s a tradeoff. For us, react-admin was probably the right choice because most of the time, our needs fit well with what the library does. But adding custom functionality is also harder than if we were creating everything from scratch!

I’ll leave you with a quote from computer scientist David Wheeler:

All problems in computer science can be solved by another level of indirection, except for the problem of too many layers of indirection.

If we are mindful of the risk of ‘too many layers of indirection’, and choose our abstractions carefully, we’ll write better and more maintainable code.