The noble art of refactoring - Part 1

#beginners #codenewbie #webdev #refactoring

If you are interested in reading this article in Spanish, check out my blog The Developer's Dungeon

Hey, how you been? I hope you were waiting to see how we do this refactoring. If you haven't checked the previous article you definitely should as today we are gonna do the refactoring and add that pending feature.

The Refactoring

So without any further ado, let's start.
First, I don't know about you, but I don't like that for loop and accessing the items by i so let's get rid of that by using a forEach to make the code clearer.

    this.items.forEach(item => {
      if (item.name != 'Sulfuras, Hand of Ragnaros') {
        item.sellIn = item.sellIn - 1;
      }
      if (item.name != 'Aged Brie' && item.name != 'Backstage passes to a TAFKAL80ETC concert') {
        if (item.quality > 0) {
          if (item.name != 'Sulfuras, Hand of Ragnaros') {
            item.quality = item.quality - 1;
          }
        }
      } else {
        if (item.quality < 50) {
          item.quality = item.quality + 1;
          if (item.name == 'Backstage passes to a TAFKAL80ETC concert') {
            if (item.sellIn < 11) {
              if (item.quality < 50) {
                item.quality = item.quality + 1;
              }
            }
            if (item.sellIn < 6) {
              if (item.quality < 50) {
                item.quality = item.quality + 1;
              }
            }
          }
        }
      }
      if (item.sellIn < 0) {
        if (item.name != 'Aged Brie') {
          if (item.name != 'Backstage passes to a TAFKAL80ETC concert') {
            if (item.quality > 0) {
              if (item.name != 'Sulfuras, Hand of Ragnaros') {
                item.quality = item.quality - 1;
              }
            }
          } else {
            item.quality = item.quality - item.quality;
          }
        } else {
          if (item.quality < 50) {
            item.quality = item.quality + 1;
          }
        }
      }
    });

I would also like to extract every boolean condition into its own explicit variable, this is called Extract Variable.
Why you might ask? well to avoid having to carry all that complexity inside my head.
By giving meaningful names to boolean expressions the code automatically becomes clearer, if you are interested in clean code I recommend that you read my previous article about the subject, you can find it here.
I would also like to change the increases and decreases for something like ++ and --.

     this.items.forEach(item => {
      const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";

      if (!isSulfuras) {
        item.sellIn--;
      }

      const isAgedBrie = item.name == "Aged Brie";
      const isBackstagePasses =
        item.name == "Backstage passes to a TAFKAL80ETC concert";
      const isQualityBiggerThan0 = item.quality > 0;
      const isQualityLessThan50 = item.quality < 50;
      // notice how I changed this 2 properties so
      // they look more to what we are expecting from them
      const tenDaysOrLessToSell = item.sellIn <= 10;
      const fiveDaysOrLessToSell = item.sellIn <= 5;
      const areNoMoreDaysToSell = item.sellIn < 0;

      if (!isAgedBrie && !isBackstagePasses) {
        if (isQualityBiggerThan0) {
          if (!isSulfuras) {
            item.quality--;
          }
        }
      } else {
        if (isQualityLessThan50) {
          item.quality++;
          if (isBackstagePasses) {
            if (tenDaysOrLessToSell) {
              if (isQualityLessThan50) {
                item.quality++;
              }
            }
            if (fiveDaysOrLessToSell) {
              if (isQualityLessThan50) {
                item.quality++;
              }
            }
          }
        }
      }

      if (areNoMoreDaysToSell) {
        if (!isAgedBrie) {
          if (!isBackstagePasses) {
            if (isQualityBiggerThan0) {
              if (!isSulfuras) {
                item.quality--;
              }
            }
          } else {
            item.quality = 0;
          }
        } else {
          if (isQualityBiggerThan0) {
            item.quality++;
          }
        }
      }
    });

Much better, although the logic is still super ugly, we already start to see some expressiveness in the code. All tests on "green" so let us move on.
Now, I would like to start unifying ifs, this is part of Simplifying Conditional Expressions, as you can already see we have a few lines of code like the next one:

   if (tenDaysOrLessToSell) {
        if (isQualityLessThan50) {
           item.quality++;
        }
   }

Since there is only one path for the code to follow when entering this block, we can unify those ifs into something like this:

  if (tenDaysOrLessToSell && isQualityLessThan50) {
      item.quality++;
  }

so let us unify all the ifs we can:

    this.items.forEach(item => {
      const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";

      if (!isSulfuras) {
        item.sellIn--;
      }

      const isAgedBrie = item.name == "Aged Brie";
      const isBackstagePasses =
        item.name == "Backstage passes to a TAFKAL80ETC concert";
      const isQualityBiggerThan0 = item.quality > 0;
      const isQualityLessThan50 = item.quality < 50;
      // notice how I changed this 2 properties so
      // they look more to what we are expecting from them
      const tenDaysOrLessToSell = item.sellIn <= 10;
      const fiveDaysOrLessToSell = item.sellIn <= 5;
      const areNoMoreDaysToSell = item.sellIn < 0;
      const isNormalItem = !isAgedBrie && !isBackstagePasses && !isSulfuras;

      if (isNormalItem) {
        if (isQualityBiggerThan0) {
          item.quality--;
        }
      } else if (isQualityLessThan50) {
        item.quality++;
        if (isBackstagePasses) {
          if (tenDaysOrLessToSell) {
            item.quality++;
          }

          if (fiveDaysOrLessToSell) {
            item.quality++;
          }
        }
      }

      if (areNoMoreDaysToSell) {
        if (!isAgedBrie) {
          if (!isBackstagePasses) {
            if (isQualityBiggerThan0 && !isSulfuras) {
              item.quality--;
            }
          } else {
            item.quality = 0;
          }
        } else if (isQualityBiggerThan0) {
          item.quality++;
        }
      }
    });

As you can see, after unifying all the ifs, some common patterns started to appear and it makes you notice that some logic for boolean expressions was duplicated in many parts of this method, so we extract them.

      const isNormalItem = !isAgedBrie && !isBackstagePasses && !isSulfuras;

Once we go to this point we realize that there are some problems with this code, basically what it is happening is that we have logic for multiple cases scattered all over the method, we are checking for Brie and Backstage passes in a lot of different places, we should arrange that logic together with a refactoring similar to Consolidate Conditional Expression. This is gonna be a big move, but it is gonna make our code incredibly easier to read.

    this.items.forEach(item => {
      const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";

      if (!isSulfuras) {
        item.sellIn--;
      }

      const isAgedBrie = item.name == "Aged Brie";
      const isBackstagePasses =
        item.name == "Backstage passes to a TAFKAL80ETC concert";
      const isQualityBiggerThan0 = item.quality > 0;
      const isQualityLessThan50 = item.quality < 50;
      const tenDaysOrLessToSell = item.sellIn <= 10;
      const fiveDaysOrLessToSell = item.sellIn <= 5;
      const areNoMoreDaysToSell = item.sellIn < 0;
      const isNormalItem = !isAgedBrie && !isBackstagePasses && !isSulfuras;

      if (isNormalItem) {
        if (isQualityBiggerThan0) {
          item.quality--;

          if (areNoMoreDaysToSell) {
            item.quality--;
          }
        }
      } else if (isBackstagePasses) {
        item.quality++;
        if (tenDaysOrLessToSell) {
          item.quality++;
        }

        if (fiveDaysOrLessToSell) {
          item.quality++;
        }

        if (areNoMoreDaysToSell) {
          item.quality = 0;
        }
      } else if (isAgedBrie && isQualityLessThan50) {
        item.quality++;
      }

As you can see now, it is pretty clear what is going on, an item can be Sulfuras which doesn't care about quality or sellin, it can be a Normal Item, it can be Backstage passes or it can be Aged Brie, each of those cases has it's logic contained in its own if statement, but there is still some work to do.

Why don't we continue by extracting the different behaviors into separate functions?, just to be clear, you could also do some classic object-orientation and separate the behaviors into an inheritance hierarchy with a factory to do a refactoring like Replace Conditional With Polymorphism but I thought that since we are using JavaScript let's keep running functions.

const calculateQualityDifferenceNormalItem = ({ sellIn, quality }) => {
  const isQualityBiggerThan0 = quality > 0;
  const areNoMoreDaysToSell = sellIn < 0;

  if (isQualityBiggerThan0 && areNoMoreDaysToSell) return -2;
  if (isQualityBiggerThan0) return -1;

  return 0;
};

const calculateQualityDifferenceBackstagePasses = ({ sellIn, quality }) => {
  const tenDaysOrLessToSell = sellIn <= 10;
  const fiveDaysOrLessToSell = sellIn <= 5;
  const areNoMoreDaysToSell = sellIn < 0;

  if (areNoMoreDaysToSell) return -quality;
  if (fiveDaysOrLessToSell) return +3;
  if (tenDaysOrLessToSell) return +2;

  return +1;
};

const calculateSellinDifference = ({ sellIn, name }) => {
  const isSulfuras = name == "Sulfuras, Hand of Ragnaros";
  return !isSulfuras ? -1 : 0;
};

Notice how I also tried to remove mutation a little by making these functions pure by just processing the update of the quality or sellIn date and returning a result without modifying the item itself. Now our updateQuality method looks like this:

  updateQuality() {
    this.items.forEach(item => {
      item.sellIn += getUpdatedSellin(item);

      const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";
      const isAgedBrie = item.name == "Aged Brie";
      const isBackstagePasses =
        item.name == "Backstage passes to a TAFKAL80ETC concert";
      const isQualityLessThan50 = item.quality < 50;
      const isNormalItem = !isAgedBrie && !isBackstagePasses && !isSulfuras;

      if (isNormalItem) {
        item.quality += getUpdatedQualityNormalItem(item);
      } else if (isBackstagePasses) {
        item.quality += getUpdatedQualityBackstagePasses(item);
      } else if (isAgedBrie && isQualityLessThan50) {
        item.quality++;
      }
    });

    return this.items;
  }

Now, let's do the same and extract the logic regarding quality, change that foreach for just a map, and let's see our final updateQuality method:

class Item {
  constructor(name, sellIn, quality) {
    this.name = name;
    this.sellIn = sellIn;
    this.quality = quality;
  }
}

const calculateQualityDifferenceNormalItem = ({ sellIn, quality }) => {
  const isQualityBiggerThan0 = quality > 0;
  const areNoMoreDaysToSell = sellIn < 0;

  if (isQualityBiggerThan0 && areNoMoreDaysToSell) return -2;
  if (isQualityBiggerThan0) return -1;

  return 0;
};

const calculateQualityDifferenceBackstagePasses = ({ sellIn, quality }) => {
  const tenDaysOrLessToSell = sellIn <= 10;
  const fiveDaysOrLessToSell = sellIn <= 5;
  const areNoMoreDaysToSell = sellIn < 0;

  if (areNoMoreDaysToSell) return -quality;
  if (fiveDaysOrLessToSell) return +3;
  if (tenDaysOrLessToSell) return +2;

  return +1;
};

const calculateSellinDifference = ({ sellIn, name }) => {
  const isSulfuras = name == "Sulfuras, Hand of Ragnaros";
  return !isSulfuras ? -1 : 0;
};

const calculateQualityDifference = item => {
  const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";
  const isAgedBrie = item.name == "Aged Brie";
  const isBackstagePasses =
    item.name == "Backstage passes to a TAFKAL80ETC concert";
  const isQualityLessThan50 = item.quality < 50;
  const isNormalItem =
    !isAgedBrie && !isBackstagePasses && !isSulfuras && !isConjuredItem;

  if (isNormalItem) return calculateQualityDifferenceNormalItem(item);
  if (isBackstagePasses) return calculateQualityDifferenceBackstagePasses(item);
  if (isAgedBrie && isQualityLessThan50) return +1;

  return 0;
};

class Shop {
  constructor(items = []) {
    this.items = items;
  }

  updateQuality() {
    return this.items.map(item => {
      item.sellIn += calculateSellinDifference(item);
      item.quality += calculateQualityDifference(item);

      return item;
    });
  }
}

That's it, now we are here and we can start working on the part of adding the new functionality.

Adding the feature

Okay, what was this feature about?

We have recently signed a supplier of conjured items. This requires an update to our system:

Conjured items degrade in Quality twice as fast as normal items

What do we change first?

** WRONG **

First, we write a test.

  it("the quality of conjured items should decrease twice as fast", () => 
  {
    const storeItems = [new Item("Conjured Mana Cake", 10, 20)];
    const expectedResult = [new Item("Conjured Mana Cake", 9, 18)];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

    expect(items).toStrictEqual(expectedResult);
  });

This will obviously fail, some Test Driven Development right there my friend. Now we can start adding functionality in a secure way.

We add a way to look for Conjured Items, those items will have "Conjured" word in their name so:

  const isConjuredItem = item.name.includes("Conjured");

We explicitly declare in our code that conjured items are not normal items by doing this:

  const isNormalItem =
    !isAgedBrie && !isBackstagePasses && !isSulfuras && !isConjuredItem;

We add the condition to calculate the difference in Quality, here we just reuse some code to make life much easier for us:

  if (isConjuredItem) return calculateQualityDifferenceNormalItem(item) * 2;

We assemble everything together in the method that calculates quantity:

const calculateQualityDifference = item => {
  const isSulfuras = item.name == "Sulfuras, Hand of Ragnaros";
  const isAgedBrie = item.name == "Aged Brie";
  const isConjuredItem = item.name.includes("Conjured");
  const isBackstagePasses =
    item.name == "Backstage passes to a TAFKAL80ETC concert";
  const isQualityLessThan50 = item.quality < 50;
  const isNormalItem =
    !isAgedBrie && !isBackstagePasses && !isSulfuras && !isConjuredItem;

  if (isNormalItem) return calculateQualityDifferenceNormalItem(item);
  if (isBackstagePasses) return calculateQualityDifferenceBackstagePasses(item);
  if (isAgedBrie && isQualityLessThan50) return +1;
  if (isConjuredItem) return calculateQualityDifferenceNormalItem(item) * 2;

  return 0;
};

We run the tests

WE ARE DONE! 🙌

Congratulations, after some long hours refactoring this code, we added the specified feature, not only that but we increased the overall quality of the software by a lot.

If you are interested in refactoring I strongly recommend Refactoring: Improving the Design of Existing Code by Martin Fowler or if you prefer something more interactive check Refactoring Guru. Both places contain some great techniques to improve existing code.

I hope you enjoyed this article, it was some hard work to refactor that mess but I think we did a great job, and I know what you are thinking, why did you use free functions instead of using class methods? shouldn't you avoid modifying instances and just return a new set of objects? all this are valid concerns, and I am not claiming in any way that this is the perfect solution, but we come a long way since our original code, and we also have some tests that verify our behavior, from here we can go anywhere and keep improving this code until it is perfect.

Please let me know in the comments if you liked this example and also if you would do something in a different way.
If you really liked this, please don't forget to share 😃.