The noble art of refactoring - Part 1
Tags: beginners, codenewbie, webdev, refactoring
Published: 2020-03-18 16:22:45

If you are interested in reading this article in Spanish, check out my blog [The Developer's Dungeon](https://www.patferraggi.dev/blog/2020/mar/noble-refactoring-1/)

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.

```javascript
    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](https://refactoring.guru/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](https://dev.to/patferraggi/5-clean-code-techniques-you-can-start-doing-today-2ifh).
I would also like to change the increases and decreases for something like ++ and --.

```javascript
     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](https://refactoring.guru/refactoring/techniques/simplifying-conditional-expressions), as you can already see we have a few lines of code like the next one:

```javascript
   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:

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

so let us unify all the `ifs` we can:

```javascript
    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.

```javascript
      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](https://refactoring.guru/consolidate-conditional-expression). This is gonna be a big move, but it is gonna make our code incredibly easier to read.

```javascript
    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](https://refactoring.guru/replace-conditional-with-polymorphism) but I thought that since we are using JavaScript let's keep running functions.

```javascript
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:

```javascript
  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:

```javascript
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. 

```javascript
  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:

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

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

```javascript
  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:

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

* We assemble everything together in the method that calculates quantity:

```javascript
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

![tests green](https://dev-to-uploads.s3.amazonaws.com/i/quibo81aolj60j9yu2zt.png)

## WE ARE DONE! :raised_hands:

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](https://www.amazon.com/-/es/Martin-Fowler/dp/0134757599/ref=pd_sbs_14_t_0/133-9591133-9013653?_encoding=UTF8&pd_rd_i=0134757599&pd_rd_r=6d5cfc6d-3bc8-44ef-a4df-4926f04f8adf&pd_rd_w=iZeJC&pd_rd_wg=582cX&pf_rd_p=5cfcfe89-300f-47d2-b1ad-a4e27203a02a&pf_rd_r=3VA8TRMT2V5QYYG5JJ3N&psc=1&refRID=3VA8TRMT2V5QYYG5JJ3N) by Martin Fowler or if you prefer something more interactive check [Refactoring Guru](https://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 :smiley:.