Refactoring With Confidence

Last week while I was making changes to our constants.js file, I spotted that we could improve its maintainability and readability. But before I could put on my refactoring glasses, I first had to make sure I would not introduce any new bugs into the codebase.

Because the list of constants that I would need to change was quite long, there was a high chance I would make a mistake. Our automated tests would most likely not catch the bug as our constants.js file is not explicitly tested as it does not contain any logic. So to improve my confidence in making the changes, I first created a (throwaway) unit test. Although I generally avoid using Jest snapshots, I think this was an ideal use case for them.

Inspecting the Problems

Let’s start by taking a look at our initial constants object:

const constants = {
  CHANNEL_BLOG: 1,
  CHANNEL_WORDPRESS: 2,
  CHANNEL_SHOPIFY: 3,
  CHANNEL_FACEBOOK: 5,
  CHANNEL_TWITTER: 6,
  
  CHANNEL_GROUP_PERSONAL: [1, 2, 3, 4],
  CHANNEL_GROUP_SOCIAL: [5, 6],
  CHANNEL_NAMES: {
    1: 'Blog',
    2: 'WordPress',
    3: 'Shopify',
    5: 'Facebook',
    6: 'Twitter',
  },
  CHANNEL_ROUTES: {
    1: 'channels/blog',
    2: 'channels/wordpress',
    3: 'channels/shopify',
    5: 'channels/facebook',
    6: 'channels/twitter',
  },
};

The above object is a smaller version of the real one so it more easily illustrates all the issues that I wanted to fix:

• The value of each channel is located in three or more locations. This means if we would need to change the value of a channel, we would need to make sure to do so in all locations. Spoiler alert, you will most likely miss changing one of them.
• The values can become out of sync as the values are not linked with each other. You may have spotted such an issue in the above example. At some point, someone removed the CHANNEL_ARTICLE with the value of 4. They removed its definition, name, and route; but forgot to exclude it from all the channel groups (see CHANNEL_GROUP_PERSONAL).
• The channel groups are hard to read as you need to manually/mentally map a value to the corresponding channel name. This means if you want to remove the Facebook channel from the CHANNEL_GROUP_SOCIAL, you first need to find out which value (5 or 6) is referring to Facebook.

Making the Improvements

An object with all the above issues fixed would look something like this:

const CHANNEL_BLOG = 1;
const CHANNEL_WORDPRESS = 2;
const CHANNEL_SHOPIFY = 3;
const CHANNEL_FACEBOOK = 5;
const CHANNEL_TWITTER = 6;

const constants = {
  CHANNEL_BLOG,
  CHANNEL_WORDPRESS,
  CHANNEL_SHOPIFY,
  CHANNEL_FACEBOOK,
  CHANNEL_TWITTER,
  
  CHANNEL_GROUP_PERSONAL: [CHANNEL_BLOG, CHANNEL_WORDPRESS, CHANNEL_SHOPIFY],
  CHANNEL_GROUP_SOCIAL: [CHANNEL_FACEBOOK, CHANNEL_TWITTER],

  CHANNEL_NAMES: {
    [CHANNEL_BLOG]: 'Blog',
    [CHANNEL_WORDPRESS]: 'WordPress',
    [CHANNEL_SHOPIFY]: 'Shopify',
    [CHANNEL_FACEBOOK]: 'Facebook',
    [CHANNEL_TWITTER]: 'Twitter',
  },
  CHANNEL_ROUTES: {
    [CHANNEL_BLOG]: 'channels/blog',
    [CHANNEL_WORDPRESS]: 'channels/wordpress',
    [CHANNEL_SHOPIFY]: 'channels/shopify',
    [CHANNEL_FACEBOOK]: 'channels/facebook',
    [CHANNEL_TWITTER]: 'channels/twitter',
  },
};

Let’s look at the new object more closely:

1. We refactored the channel definitions into variables. This allows us to reuse them as values for other constants.
2. We replaced the fixed values inside the channel groups. This makes it easy for someone to read what channels are in each group. The mistake mentioned above where someone forgot to remove all values of a constant also won’t be possible to repeat in the future. This is because as soon as you remove a constant and don’t remove all its usages, you will start seeing the ReferenceError: “x” is not defined error.
3. We replaced the fixed property keys of CHANNEL_NAMES and CHANNEL_ROUTES with the channel variables. We did so by using computed property names.

As you can see, there are multiple changes that we need to do for each constant. In the case of our app where we have 30+ channels, there is a good chance that I would make a mistake while refactoring. I hope at this point you agree that the upfront investment of writing a unit test is well worth it.

Creating the Unit Test

For this task, the unit test needs to check the original object and compare it to the modified version. In our app we use Jest to write our unit and integration tests.

We could include the original object inside the test and use toMatchObject matcher but I instead opted to create a snapshot:

import constants from '../constants';

it('should match the original object', () => {
  expect(constants).toMatchSnapshot();
});

When we run the test for the first time, Jest will remember the object value by storing it as a snapshot. It will then use the snapshot for comparison when we run the test again.

To run the test I used the following Jest CLI command:

jest contants.test.js --watch

The --watch mode makes sure to automatically run the test each time we make a change.

Once I had the unit test in place, I went step-by-step making small changes. If I made a mistake after a change, the unit test would let me know and point to the issue. Fixing a mistake was fast and simple. After I had made all the changes, I removed the unit test and opened a pull request.

Conclusion

We need to be confident that we will not introduce any issues in our codebase when making refactorings.

If the change is small and isolated (changing a variable name, fixing a typo, etc.), checking the change manually can be a valid approach.

But in a lot of cases making sure a unit test is watching over our changes is the practical way to go. This is especially true in situations when making:

• large changes,
• changes that affect multiple parts,
• changes that require you to test many cases.

When we are refactoring a part of the codebase that is not yet covered by an automated test, we should check to see if introducing a test to the codebase will provide value. If so, then we need to make sure to write a test that is not going to be a maintenance burden. Jest snapshots were a great use case for this throwaway test but in a lot of cases can be a maintenance nightmare as your tests can report false negatives.