A Refactoring Breakdown

I thought it would be interesting and potentially useful to others, to work my way through a refactor. It's not a topic I've run across very often and, especially early on in one's career, it can be challenging to understand exactly how to refactor something. Once code is written and working, trying to see areas where it can be improved can be tricky.

In addition to showing how a refactor can work, this is giving me a chance to see how much I've been learning. For the purposes of brevity, I'm going to limit this to a single file on a fairly basic project.

I set up a link directory about a year ago, based off of my Firefox bookmarks. There were plenty of challenges in building the app, not the least of which was that I was relatively new to React, though not to JavaScript or web development in general. In building the app, I focused on just getting the job done and working, even if it meant sacrificing a lot of the more DRY or frankly better ways I could have written it.

A year on, I've decided to go back to the project and clean it up, give it better structure, and implement my more refined development process. Let's dive into one of these files and see where it goes!

Our starting point

The file I'll be using as an example is the list of links that appears under each topic. When I opened the file up, this is what I found:

import React from "react";
import { Link } from "@reach/router";
import { LinkListProps } from "../../interfaces";

 class LinkList extends React.Component<LinkListProps, {}> {
  linkElement(linkID,linkURI, linkTitle) {
    if (linkURI) {
      return (<li key={linkID}><a href={linkURI}>{linkTitle}</a></li>);
    } else {
      return ""
    }
  }
  render() {
    return (
      <ul>
        {this.props.links.map(link => {
          return this.linkElement(link.id link.uri, link.title)
        })}
      </ul>
    );
  }
}

 export default LinkList;

Okay, this is not so bad, but I can see some immediate places this can be improved. There's a lot of extra syntax in general, for example. Let me break down the changes I'm making.

Swapping Typescript for PropTypes

I've stopped using Typescript in my personal projects over the last year. I think it's awesome and for projects that have a team larger than one and a project significantly more complex than this one, I would argue it might be essential. However, at this level of complexity, Typescript is heavy-handed and adds more to the maintenance time for the project. I would swap it out for more simplified PropTypes.

import React from "react";
import { Link } from "@reach/router";
import { PropTypes } from "prop-types";

 class LinkList extends React.Component{
  linkElement(linkID,linkURI, linkTitle) {
    if (linkURI) {
      return (<li key={linkID}><a href={linkURI}>{linkTitle}</a></li>);
    } else {
      return ""
    }
  }
  render() {
    return (
      <ul>
        {this.props.links.map(link => {
          return this.linkElement(link.id link.uri, link.title)
        })}
      </ul>
    );
  }
}

 export default LinkList;

 LinkList.propTypes = {
     links: shape({
         id: PropTypes.string.isRequired,
         uri: PropTypes.string.isRequired,
         title: PropTypes.string.isRequired
     })
 }

Logic Streamlining

Time to look at the logic itself. In this case, the changes I make aren't going to improve performance in any measurable or meaningful way. It will make the file a little easier to read, which is important. It also reduces the lines of code, which also reduces the tests we'll need to write.

In retrospect it looks very verbose to have the linkElement function there when all it actually does is process some simple logic. Originally this had been set up with the anticipation of needing to add more functions to the component and this would separate concerns a bit. However, I've moved toward avoiding premature optimization in my code so this separation is now overkill.

I'll also note that there's nothing inherently wrong with this code. It compiles properly and the browser does what I would expect it to do with the logic as it's written. With the changes I'm making, what I'm looking to do is improve the readability. I want other developers to be able to read my code as easily as the computer does. So, the changes are about style, more than about substance.

Before I delete the function entirely, let's see what it would look like cleaned up a bit. Here's our current version:

linkElement(linkID,linkURI, linkTitle) {
    if (linkURI) {
        return (<li key={linkID}><a href={linkURI}>{linkTitle}</a></li>);
    } else {
        return ""
    }
}

I would handle this with a ternary as the "else" of the statement is an empty string and even the "if" result is contained on a single line. Let's see what that looks like:

linkElement(linkID,linkURI, linkTitle) {
        return linkURI ? (<li key={linkID}><a href={linkURI}>{linkTitle}</a></li>) : "";
}

That definitely looks even more unnecessary now, right? Okay, now I'll clean up the rest of the file to reflect that change and remove the function. Also note that I don't need to do an explicit return anymore either since I'm using the ternary.

import React from "react";
import { Link } from "@reach/router";
import { PropTypes } from "prop-types";

 class LinkList extends React.Component{
  render() {
    return (
      <ul>
        {this.props.links.map(link => linkURI ? (<li key={linkID}><a href={linkURI}>{linkTitle}</a></li>) : "" )}
      </ul>
    );
  }
}

 export default LinkList;

 LinkList.propTypes = {
     links: shape({
         id: PropTypes.string.isRequired,
         uri: PropTypes.string.isRequired,
         title: PropTypes.string.isRequired
     })
 }

Wrap Up

The end result of this refactor accomplishes two major goals:

• Brings the file inline with how I construct my projects now.
• Reduces the amount of code in the file, making it more readable, easier to refactor further as needed, and less prone to bugs.

The logic itself is all still there, the functionality of the code didn't change. To the end user, everything will appear exactly the same. The biggest benefit is that the next time a developer (including my future self) opens the file, the level of complexity is much lower, hopefully making it also easier to understand at a glance.

What makes this a refactor and not just a modification of code? I read this quote from Martin Fowler a while back that sums it up:

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior.

So in this case, though the file was small to begin with, I've drastically restructured it while maintaining the external behavior either in how it interacts with other components or in how the user receives the rendered component in the browser.

With my code cleaned up, I can move on with confidence that my code is as clean as I can make it. I hope this brief overview helps clear away some of the mystery around what a refactor looks like in real life.