Junior Refactors: Example 1.

(Introduction to this series here)

Let’s start with a small one.

I like to look at a code review in the following order:

  1. Does it work?
  2. Is it doing what it should?
  3. Is it doing things it shouldn't?
  4. Is it readable (and consistent with the codebase)?
  5. Is it prone to exploding?

For our first example, we start with this code:

const ellipseTitle = (title, long) => {
    const maxTitleLength = [25, 10]
    return long ? title.length < maxTitleLength[1]
        ? title
        : `${title.slice(0, maxTitleLength[1] - 1)}...`
        : title.length < maxTitleLength[0]
        ? title
        : `${title.slice(0, maxTitleLength[0] - 1)}...`
}

The first thing that stands out is that this hasn’t been formatted correctly. Let’s auto format.

const ellipseTitle = (title, long) => {
    const maxTitleLength = [25, 10]
    return long 
        ? title.length < maxTitleLength[1]
            ? title
            : `${title.slice(0, maxTitleLength[1] - 1)}...`
        : title.length < maxTitleLength[0]
            ? title
            : `${title.slice(0, maxTitleLength[0] - 1)}...`
}

Ok! With that out of the way, lets find out what we’re trying to do.

Luckily, the name of the function is prescriptive enough, and the function body isn’t too long.

We take two parameters: A title, and a boolean value long. Based on the value of long, we return title ellipsed to a length specificed in the function body.

Does it work?

Javascript doesn’t have a REPL, so we can’t experiment there.
But, this merge request came with unit tests:

const title = "Design patterns are bug reports " +
              "against your programming language."

test("ellipseTitle short", () => {
    const wanted = "Design pa..."
    const actual = ellipseTitle(title, false)
    expect(wanted).toBe(actual);
})

test("ellipseTitle long", () => {
    const wanted = "Design patterns are bug ..."
    const actual = ellipseTitle(title, true)
    expect(wanted).toBe(actual);
})

And those tests are passing. So far so good.

Is it doing what it should?

Yes.

Based on the work task, the name and tests producing the desired result.

Is it doing things it shouldn't?

Yes, it’s a single function that should be two functions.
Let’s break out the actual ellipsing.

const ellipseTitle = (title, long) => {
    const maxTitleLength = [25, 10]
    return long 
        ? ellipse(title, maxTitleLength[0])
        : ellipse(title, maxTitleLength[1])
}

const ellipse = (text, max) => {
    if (text.length < max) {
        return text
    }
    return `${text.slice(0, max - 1)}...`
}

We’ve now separated the logic for deciding on how to ellipse a title from the logic of ellipsing, regardless of what we’re ellipsing.

This enables us to reuse ellipse later, and we run less risk of accidentally breaking something in ellipseTitle if modifying it in the future, as there is less logic in there now.

Let’s simplify the return statement by moving the if-else to maxTitleLength instead.

const ellipseTitle = (title, long) => {
    const amount = long ? 25 : 10
    return ellipse(title, amount)
}

Now the ternary operator is used in a way where it makes more sense: a short one-liner. Ternaries spanning multiple lines are potentially more confusing than if statements and are best avoided in those cases.

Is it readable?

Much more than before.

Is it prone to exploding?

We’re not checking the presence of title, which could be null or undefined. This shouldn’t be a problem however, as input should be sanizited and checked before calling this method. Otherwise we end up in a situation where every function in the entire codebase must be guarded, which increases both code to maintain and potential bugs and errors as there are now more paths to branch from.

Anything else?

Not sure if those numbers should be hard coded into the function itself, but we’ll let them slide for now. Perhaps in the future, these should be part of a config and/or injected at application start. This would prevent us from scattering business logic all over the application.

Until next time!

< Previous

Next >