Don’t Just Refactor, Make it Maintainable

Safely Refactor Old Code: Part 2

--

In this series, we go over the conceptual design patterns of safely refactoring old code step-by-step. I’ve written this guide to accommodate any kind of refactoring scenario, and the example project is Node.js with RxJS.

This is the second article in a 3-part series on safely refactoring code:

Our Story So Far

In writing tests, we were completely unable to get unit, integration, or even end-to-end (E2E) tests written without major code changes. Instead, we ended up creating E2E tests where a human is required in the process to verify functionality. Now that we’ve got something to ensure our app isn’t broken, we can begin refactoring the code so it’s actually testable and presentable for an npm package.

Where to Start Refactoring?

Now that we’ve finally squared away a method for verifying breakage, where do we start? Do we refactor or rewrite? This article is about safely refactoring so we’re going to avoid a rewrite. That means we have to ensure every minor change leaves the app in a working state, but it also means we can refactor module-by-module.

Disclaimer: While I noted we wrote tests to ensure we don’t break functionality, at some point, we will have to upgrade the RxJS version which includes breaking changes. Our unit tests won’t function correctly unless we update imports so we’ll have to test the tests again when that time comes.

In the last article, we did a little bit of rework just to ensure we could get some repeatable testing done. We’re going to continue that today, but I want to start smart.

Start With Libraries

Upgrading RxJS

Since the biggest change to our codebase will be the RxJS upgrade, let’s do that and get it out of the way. You wouldn’t think to handle the big fish first, but this upgrade is going to change some of the fundamentals of our design. Not upgrading RxJS, after refactoring, means we have more places to update including unit tests.

We’re going to utilize the rxjs-compat library to retain our existing RxJS v5 code along-side the newer RxJS v6. This way, we don’t have to change everything all-at-once, we can do it in chunks. It’s important to remove rxjs-compat when you’re done refactoring. I can foresee larger projects installing a crutch like that and then continuing to have 2 separate methods of using RxJS for the lifetime of the product.

The safest route is keeping us in a box of one small change at a time, not a wide-scale architectural change at the last minute. From experience, upgrading RxJS last means it’ll never get updated because you’ll have too much ingrained in the old way of doing it to move. We’re already going to have to update our one unit test when RxJS upgrades, so why write a bunch of extra unit tests on top of that which will all need potentially-breaking changes as well?

On the other hand, you shouldn’t upgrade RxJS first-thing if you’re code is written in a way where it’s easier to rewrite than refactor. In our case, we’ll be taking a little bit of code and moving it around to separate unit-testable modules. This significantly changes its structure which makes it important to upgrade sooner than later.

Simply updating rxjs and adding rxjs-compat was enough to keep the code working as before:

yarn add rxjs rxjs-compat

This tiny little upgrade was a super safe change so far. We don’t know about performance, memory usage, etc; but I’m going to assume if you don’t already have tests for those things, it’s not worthwhile for your project.

Other Libraries

I know my other libraries have newer versions, but I’m only going to update the ones without breaking changes. I want to continue using RxJS so I upgraded it. On the other hand, I don’t want to continue using moment.js, so I’ll leave it as-is until I’m ready.

In this project, some safe upgrades are nodemon, node-fetch, and as we’ve already seen, rxjs. Upgrading app-module-path and moment.js are not safe because of possible breakage in the existing API. If the project is well-maintained, I could at least update the patch version (the last decimal in semantic versioning: 2.19.1) especially if my current version has known security flaws. You’re supposed to also be able to upgrade the minor version (2.19.1) with most npm packages, but if you don’t already have unit tests, it’s not safe.

Avoid Changing Libraries

In the initial refactor, you’ll want to avoid changing libraries. That way, you can ensure the old code still works before swapping it with something new.

For instance, I’d really like to use date-fns instead of moment.js, but instead of going to all the places using moment.js and swapping them out, I should first refactor those spots to allow for unit testing against moment.js and then later swap to date-fns after I’ve got working unit tests.

Is it Halloween?

Let’s take a look at our isHalloween example from before:

We need to give it a date instead of hard-coding moment() so we can unit test it no matter what day it is. We also need to take currentYear and pass it in as an argument or change it into a function and pass a date to it. Both of these changes can be easily unit tested to ensure nothing broke.

Super Safe Refactor

A simple refactor looks like this:

Notice how all we’ve done is made moment() take a passed-in date. That’s exactly what we want. Minimal changes, but something we can unit test.

Our isHalloween function previously took no arguments so we’ve retained that functionality and made it default to new Date() only when no date is passed (the old way of calling moment() without arguments).

Renaming Functions

As a final change, I wanted to rename this function from isHalloween to isDuringHalloweenNight since that’s semantically correct:

Keeping with the idea of safe refactors, I created the new file with the renamed function and deprecated the old function. This way, the old functionality still works, but now we can notify users of the new file so they can be aware of the deprecated change.

I recommend using a deprecation library instead of writing custom deprecation code all over your codebase. For this example, I wanted to keep it simple, but I’ve actually written my own deprecation library to handle these cases.

Writing Unit Tests

I went ahead and brought in AVA as our unit testing library and updated how we’re importing files.

Here’s what our unit tests look like:

I could unit test the old isHalloween as well, but the original isHalloween wasn’t testable because it didn’t take args. Since it still doesn’t take args, it’s still not able to be tested.

Updating the Library

Now that our unit tests are in place, I upgraded to date-fns and rewrote isDuringHalloweenNight. Since the functionality didn’t change, our unit tests are unchanged. This is the result:

The underlying date library is all personal preference, but date-fns has a functional programming folder which allows you to compose new functions from their base functions. In the new isDuringHalloweenNight, we’re composing a new date function.

It would read a lot cleaner if we didn’t need to pass date into getYear, but this refactor wasn’t meant to be a complete rewrite.

Color Indexing

In flashRandomLight, we have a lot of untestable functions. To make them testable, we’ll do the same thing as we did with isDuringHalloweenNight; we want to write base functions that can be composed over and use our functions in flashRandomLight to semantically compose over those generic base functions.

Let’s look at getRandomColorSetIndex:

This refactor is different from the last one. We’re creating the base function (like we did with date-fns) and composing it in our old function. When unit testing, it makes sense to unit test generic base functions since composed functions don’t necessarily need to be unit tested.

Since getRandomColorSetIndex still doesn’t take any arguments, we can’t unit test it. That’s why it was best to make a new generic function that we could actually unit test: pluckRandomNumberFromRange.

Instead of passing dependencies as a second argument — leaving it exposed for anyone to pass in — we could also separate it out into a separate function and compose it into our random-number-plucking function:

This refactor is a lot cleaner and really dumbs down what we’re doing. At this point, someone could refactor pluckRandomNumberFromRange in the future to use a generic range function from lodash-fp or Ramda; although, nothing exists in those libraries for this particular functionality even though it’s a popular question on StackOverflow.

We could still refactor this further into the most-popular StackOverflow answer:

Math.floor(Math.random() * upperBound) + lowerBound

I’m not going to make that change for this project, but it’s definitely possible. Just pass a lowerBound of 0 and your existing functionality is unchanged.

I wrote unit tests and tweaked them after changing the API. As I do these refactors, I’m periodically checking the app to make sure it runs and the original E2E test spec to ensure the app’s still functioning properly as well.

There’s one last change I made. We need some way to throw an error if someone doesn’t pass in the number of items or their item count is 0. It makes no sense to return 0 in this case because that’s not a valid index in a list of size 0.

Either we return null or error. I wanted to try out something new in this refactor so we’re returning an error:

Composing Errors

Because I modified createRangePicker to throw an error, I created a mechanism for throwing errors using functional composition. The API is similar to how AVA throws errors. This way, if certain error criteria are met before our function executes, we can simply throw an error without having to complicate the logic of our function.

Since errorWrapper is composable, you can put one in another in another in another. In a later version of ECMAScript, you’ll even be able to use the pipeline operator to read them vertically at a single tabbing. That would greatly improve readability.

The Final Product

After a whole bunch of work refactoring and writing unit tests, I was able to make this project into something maintainable, tested, and solid. I don’t fear it suddenly not working on Halloween, and can keep adding E2E tests to verify it’s doing what I expect just for that little bit of added security.

If you want to see the AJAX refactor and others, checkout Part 3!

More Reads

If you like what you read, you should also checkout my other articles on the smart home and functional programming:

--

--