Refactoring: Writing Competent Code

In “The Way of Refactoring,” I took a shortcut: I discussed when refactoring deserves consideration, along with tools and methods that encourage it, but I said little about how to refactor code itself—or, more precisely, how to turn bad code into good code, or at least into competent code. This article takes up that topic.

We first define “competent code”:

• It satisfies the lint rules defined by the project.
• The code is clear and concise, with no logic that defies human intuition or deliberately clever tricks.
• Side effects are controlled as much as possible.
• Comments are present in the right amount—neither too many nor too few.
• The code’s interfaces are defined sensibly, making them hard to misuse or abuse.

The first point goes without saying. We have already described a strict set of lint rules in an earlier article. Lint rules are not universal; each team can choose rules it is comfortable with and then follow them on every commit. Note that once such rules have been tuned and stabilized, you must hold the line on them as if they were a matter of faith. Do not compromise, or your bar will keep dropping and your code quality will keep deteriorating.

Let us look at the remaining points.

Clear and Concise Code

Some readers may ask: if a function’s length is strictly limited by lint to a few dozen lines, isn’t that clear and concise enough? Can readability still be poor? Yes, it can. Consider a real example—one I encountered personally during code review (lightly modified to remove irrelevant details):

function clean(tags) {
  var dedupe = {};
  for (var i=0; i<tags.length; i++){
    tags[i] = tags[i].trim();
    var aTag = tags[i];

    if(dedupe[aTag]){
      var t =tags.splice(i,1);
      i--;
    } else {
      dedupe[aTag] = true;
    }
  }
  return tags;
}

Read this code carefully twice and see whether you can infer what it is trying to do. If you cannot, work through it on paper and see what it is trying to express.

It is hard to understand at a glance. This code has two things that defy human intuition:

• It mutates tags, a variable tied to the loop, inside the loop.
• Because tags is changed, the loop’s termination condition changes as well, so the loop index is also adjusted with i-- (this change was surely added during debugging after something went wrong, to patch the problem).

Through these two counterintuitive moves, this ten-odd-line function succeeds in making itself thoroughly rotten. The only way to deal with such code is to understand (or test out) the logic it is meant to express, and then rewrite it:

const uniq = R.pipe(R.map(R.trim), R.uniq);

This example is typical of a large class of code that is not clear or easy to understand: such code has a common trait—the programmer started writing before thinking through how to write it, then got busy debugging when problems arose, and finally delivered code that basically runs correctly but cannot be read. Programmers who produce this kind of code are generally using the Debugger-Driven Development method I mentioned in “The Way of Refactoring.”

Another kind of code is clear enough but not concise enough.

const ALPHABET = ['1', '2', '3', '4', '5', '6', '7', '8', '9',
'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S',
'T', 'U', 'V', 'W', 'X', 'Y', 'Z'];

function randomCode(length) {
  const code = [];
  for (let i = 0; i < length; i++) {
    const rand = Math.floor(Math.random() * 33);
    code.push(ALPHABET[rand]);
  }
  return code.join('');
}

This code could of course be simplified further, but there is essentially no more meaningful refactoring to do. Yet do we really need to write code like this ourselves? Someone has surely solved such a basic problem already—for example, the randomstring module.

const randomstring = require('randomstring');
const randomCode = length => randomstring.generate({
  length, capitalization: 'uppercase',
});

Some people may object to this kind of refactoring—my code has no problem; it is just a few extra lines; is it really necessary to bring in an external module? I think everyone still remembers the left-pad incident that caused a stir in the open-source world not long ago: a programmer, exhausted and furious, deleted all the modules he had published on npm. One of them, a very simple implementation of leftpad, was depended on by many well-known modules (including React, Babel, and others), eventually triggering a chain of problems. Many people wrote articles mocking today’s programmers for having degenerated to the point where they are too lazy to write even a dozen lines of code and are too dependent. To this, Sindre Sorhus replied in issue #10 of his AMA (Ask Me Anything) repo:

People get way too easily caught up in the LOC (Lines Of Code). LOC is pretty much irrelevant. It doesn’t matter if the module is one line or hundreds. It’s all about containing complexity. Think of node modules as lego blocks. You don’t necessarily care about the details of how it’s made. All you need to know is how to use the lego blocks to build your lego castle. By making small focused modules you can easily build large complex systems without having to know every single detail of how everything works. Our short term memory is finite. In addition, by having these modules as modules other people can reuse them and when a module is improved or a bug is fixed, every consumer benefits.

I think the great Sindre has made it clear; let me add a few more benefits of the “borrow what exists” philosophy:

• General and highly extensible. What if one day the above code needs to support lowercase letters? Or mixed case? And so on. Every change in requirements would involve quite a few edits.
• You do not need to write test cases. Other people’s code has been thoroughly tested. If you insist on rewriting it, the cost is not just implementation—do not forget testing.

Of course, borrowing is not simple. First, you need perspective (knowing what to borrow and where to find it); second, you need the ability to select suitable modules as your Lego bricks.

Control Side Effects as Much as Possible

Side effects are the most worrying part of software; they are often the root of all evil. Side effects in a program are no friend to memoization, an enemy of concurrency, a burden on testing, a destroyer of elegance, and a hazard to your teammates. Take the clean() code above. Not only does it use two mind-bending statements to turn our fragile brains into mush, it compounds the damage by casually producing side effects. The input argument tags is completely mutated after one pass through the function, leaving callers endlessly confused.

Most of the code we write should have no side effects. Side effects should be allowed only when interacting with I/O—input/output such as displaying to a screen, reading files, using the network, accessing a database, and so on.

Comments in the Right Amount—No More, No Less

Incompetent code often has a certain smell. If, while writing, you find that you need an extra comment to explain a function name or variable name, then the name is poor; if you find that your comment is clearly enumerating several things the function does, then you have probably crammed too many responsibilities into one function; if you have to write a long comment to explain how a piece of code works, then that code is fundamentally flawed and needs refactoring; for weakly typed languages, comments also serve as type hints, whereas in strongly typed languages using comments to explain types is gilding the lily.

If the code has side effects other than logging, they must be explicitly documented in comments.

Except for extremely self-explanatory code, having no comments is also bad: readers of your code would have to read the whole thing to understand what the inputs and outputs are, whether there are side effects, and so on.

Writing comments is an art of its own; I may write a separate article about it when I get the chance.

Sensible Interface Definitions

Finally, the centerpiece of this article: sensible interface definitions.

First, what is an interface?

From a product perspective: GUI/CLI were the interfaces of the PC era; touch and gestures are the interfaces of the mobile era; AR/VR will be the interfaces of the future.

From a software development perspective: macros and functions are interfaces; classes and data structures are interfaces; libraries and packages are interfaces; environment variables are interfaces; message types and network protocols are interfaces; system calls are interfaces; software and hardware interrupts are interfaces; EABI is an interface.

So as long as we are writing code, we are constantly dealing with interfaces—and constantly creating new ones.

A necessary condition for competent code is sensible interfaces.

What makes an interface sensible?

• Sensible naming
• Sensible inputs and outputs
• Conforms to convention
• Hard to misuse or abuse

The importance of sensible naming is self-evident: it keeps people who use your code from hesitating or becoming confused.

Sensible inputs and outputs mean the interface should not expect overly complex input (for example, a function should ideally take no more than five parameters); do not depend on data outside the input unless necessary; the input parameters should carry exactly the data needed for the task, no irrelevant data; and likewise for output—do not return results that confuse the caller.

Following convention is a very important principle in interface design. For example, in some languages, by convention size() is an O(1) operation while length() is O(n). So when you provide an interface for your data structure, you should follow that convention as well, so users will not be confused or misuse it.

Hard to misuse or abuse—few people consider this when designing interfaces, especially programmers using weakly typed languages. For example, imagine you have the following JavaScript interface:

middleware(name, description, validators, action)

It expects the first argument to be an all-letter string unique among all middleware, the second to be a string, the third to be an object containing only a handful of keys, and the fourth to be a function with an arity between 1 and 3. How do you ensure your interface will not be misused?

The answer is to validate the input, just as you validate input to a REST API. Only in this way can you eliminate problems at the bud to the greatest extent, while letting users of your interface learn from error messages until they call it correctly.

Rico Mariani has a phrase for this: “the pit of success.”

In stark contrast to a summit, a peak, or a journey across a desert to find victory through many trials and surprises, we want our customers to simply fall into winning practices by using our platform and frameworks. To the extent that we make it easy to get into trouble we fail.

Scott Meyers puts it bluntly:

Make interfaces easy to use correctly, and hard to use incorrectly

Yes, you are responsible for your code and for your interfaces. Make it possible for users to do only the right thing and difficult to do the wrong thing. As programmers, we should keep writing competent, excellent code rather than adding more digital garbage to a world that is already bad enough.