The noble art of refactoring - Part 0
Tags: beginners, codenewbie, webdev, refactoring
Published: 2020-03-14 17:57:35

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-0/)

Usually, when we study programming or we develop our own projects we never realize that this is not the regular flow we are gonna follow when we work as part of a team on a project that has been developed over the years, 90% of the time we are gonna be working on code that is already there, the original developer might no be in the team anymore, maybe there is no documentation, maybe there are no tests, a lot of different possibilities here, but since we will be involved in this environment it is crucial to know how to improve existing code, this process is called _Refactoring_. 

## Introduction

First, let's start with a definition:

> A change made to the internal structure of software to make it easier to understand and cheaper to modify without changing its observable behavior. -- Martin Fowler

In short, refactoring means restructuring the existing code without changing the output. You may have heard the term “refactoring” used vaguely to refer to any modification of the existing code. However, refactoring is actually a technique based on transformations that improve your code without affecting the behavior.

One important consideration is that refactoring doesn’t intentionally fix bugs, alter any functionality or directly improve performance. The code is assumed to be in a working state before you start.

---

This is gonna be a purely practical series of articles, we are gonna dive deep into existing code, we are gonna refactor it and then add a new feature.

## The example

For our practice, we are gonna use the [Gilded Rose Refactoring Kata](https://github.com/NotMyself/GildedRose). In our case, we are not gonna use the original implementation written in C#, since I am a C# dev moving to the frontend I decided to try something different and use the _JavaScript_ version from [this repository](https://github.com/emilybache/GildedRose-Refactoring-Kata/tree/master/js-jest), as a testing framework we are gonna use [Jest](https://jestjs.io/), so let's get on with it.

## Requirements

Luckily this time there is some documentation of what our code should do.

># Gilded Rose Refactoring Kata

>Hi and welcome to team Gilded Rose. As you know, we are a small inn with a 
prime location in a prominent city ran by a friendly innkeeper named 
Allison. We also buy and sell only the finest goods. Unfortunately, our 
goods are constantly degrading in quality as they approach their sell by 
date. We have a system in place that updates our inventory for us. It was 
developed by a no-nonsense type named Leeroy, who has moved on to new 
adventures. Your task is to add the new feature to our system so that we 
can begin selling a new category of items. First an introduction to our 
system:

> - All items have a SellIn value which denotes the number of days we have 
to sell the item
> - All items have a Quality value which denotes how valuable the item is
> - At the end of each day our system lowers both values for every item

> Pretty simple, right? Well, this is where it gets interesting:

> - Once the sell-by date has passed, Quality degrades twice as fast
> - The Quality of an item is never negative
> - "Aged Brie" actually increases in Quality the older it gets
> - The Quality of an item is never more than 50
> - "Sulfuras", being a legendary item, never has to be sold or decreases 
in Quality
> - "Backstage passes", like aged brie, increases in Quality as it's SellIn 
value approaches; Quality increases by 2 when there are 10 days or less 
and by 3 when there are 5 days or less but Quality drops to 0 after the 
concert

> 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

> Feel free to make any changes to the UpdateQuality method and add any 
new code as long as everything still works correctly. However, do not 
alter the Item class or Items property as those belong to the goblin 
in the corner who will insta-rage and one-shot you as he doesn't 
believe in shared code ownership (you can make the UpdateQuality 
method and Items property static if you like, we'll cover for you).

>Just for clarification, an item can never have its Quality increase 
above 50, however "Sulfuras" is a legendary item and as such its 
Quality is 80 and it never alters.

## The code

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

class Shop {
  constructor(items=[]){
    this.items = items;
  }
  updateQuality() {
    for (let i = 0; i < this.items.length; i++) {
      if (this.items[i].name != 'Aged Brie' && this.items[i].name != 'Backstage passes to a TAFKAL80ETC concert') {
        if (this.items[i].quality > 0) {
          if (this.items[i].name != 'Sulfuras, Hand of Ragnaros') {
            this.items[i].quality = this.items[i].quality - 1;
          }
        }
      } else {
        if (this.items[i].quality < 50) {
          this.items[i].quality = this.items[i].quality + 1;
          if (this.items[i].name == 'Backstage passes to a TAFKAL80ETC concert') {
            if (this.items[i].sellIn < 11) {
              if (this.items[i].quality < 50) {
                this.items[i].quality = this.items[i].quality + 1;
              }
            }
            if (this.items[i].sellIn < 6) {
              if (this.items[i].quality < 50) {
                this.items[i].quality = this.items[i].quality + 1;
              }
            }
          }
        }
      }
      if (this.items[i].name != 'Sulfuras, Hand of Ragnaros') {
        this.items[i].sellIn = this.items[i].sellIn - 1;
      }
      if (this.items[i].sellIn < 0) {
        if (this.items[i].name != 'Aged Brie') {
          if (this.items[i].name != 'Backstage passes to a TAFKAL80ETC concert') {
            if (this.items[i].quality > 0) {
              if (this.items[i].name != 'Sulfuras, Hand of Ragnaros') {
                this.items[i].quality = this.items[i].quality - 1;
              }
            }
          } else {
            this.items[i].quality = this.items[i].quality - this.items[i].quality;
          }
        } else {
          if (this.items[i].quality < 50) {
            this.items[i].quality = this.items[i].quality + 1;
          }
        }
      }
    }

    return this.items;
  }
}

```

I am not religious but this calls for a **JESUS CHRIST!**. After the initial shock, what do we modify first?.

**WRONNNNGGGG!** 

We don't modify anything, when we are in a situation like this, modifying the code directly is one the worst things we can do since it will be very hard for us to actually know if we kept the original behavior, it is gonna turn into a gigantic ball of bugs. So first we write some tests that capture the working behavior and then we proceed with the modifications.

## Writing the tests

Here we have two approaches, we can start writing down a test for every single expected behavior, only possible because we have the documentation, or we can do a Text-Based test. This type of test is nothing more than capturing the existing output of the code and using this to check against every time we do a modification, this is a term called _Golden Master_.

So let's start with that one:

```javascript
  it("should return correct result", () => {
    // The original items passed to the method
    const storeItems = [
      new Item("+5 Dexterity Vest", 10, 20),
      new Item("Aged Brie", 2, 0),
      new Item("Elixir of the Mongoose", 5, 7),
      new Item("Sulfuras, Hand of Ragnaros", 0, 80),
      new Item("Backstage passes to a TAFKAL80ETC concert", 15, 20)
    ];
    // The result the code returned
    const expectedResult = [
      new Item("+5 Dexterity Vest", 9, 19),
      new Item("Aged Brie", 1, 1),
      new Item("Elixir of the Mongoose", 4, 6),
      new Item("Sulfuras, Hand of Ragnaros", 0, 80),
      new Item("Backstage passes to a TAFKAL80ETC concert", 14, 21)
    ];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

    expect(items).toStrictEqual(expectedResult);
  });
```
This test compares the original items that were passed down to the working code, against the result that the code gave, with this test we can always verify that the existing behavior is maintained.
To be extra confident that my changes won't alter the behavior I am gonna write tests for the cases that are not properly represented the current input of the method, so I go ahead and add few more tests that represent those conditions.

```javascript
 it("for normal items quality should never be below 0", () => {
    const storeItems = [new Item("+5 Dexterity Vest", 10, 0)];
    const expectedResult = [new Item("+5 Dexterity Vest", 9, 0)];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

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

  it("when the sellIn date passes, quality should degrade twice as fast", () => {
    const storeItems = [new Item("+5 Dexterity Vest", 0, 4)];
    const expectedResult = [new Item("+5 Dexterity Vest", -1, 2)];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

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

  it("the quality of an item can never be more than 50", () => {
    const storeItems = [new Item("Aged Brie", 1, 50)];
    const expectedResult = [new Item("Aged Brie", 0, 50)];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

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

  it("the quality of an aged brie should increase by 1", () => {
    const storeItems = [new Item("Aged Brie", 1, 0)];
    const expectedResult = [new Item("Aged Brie", 0, 1)];
    const gildedRose = new Shop(storeItems);
    const items = gildedRose.updateQuality();

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

  describe("Backstage passes", () => {
    it("increases in Quality as it's SellIn value approaches", () => {
      const storeItems = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 14, 0)
      ];
      const expectedResult = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 13, 1)
      ];
      const gildedRose = new Shop(storeItems);
      const items = gildedRose.updateQuality();

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

    it("Quality increases by 2 when there are 10 days or less", () => {
      const storeItems = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 10, 0)
      ];
      const expectedResult = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 9, 2)
      ];
      const gildedRose = new Shop(storeItems);
      const items = gildedRose.updateQuality();

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

    it("Quality increases by 3 when there are 5 days or less", () => {
      const storeItems = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 5, 0)
      ];
      const expectedResult = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 4, 3)
      ];
      const gildedRose = new Shop(storeItems);
      const items = gildedRose.updateQuality();

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

    it("Quality drops to 0 after concert", () => {
      const storeItems = [
        new Item("Backstage passes to a TAFKAL80ETC concert", 0, 30)
      ];
      const expectedResult = [
        new Item("Backstage passes to a TAFKAL80ETC concert", -1, 0)
      ];
      const gildedRose = new Shop(storeItems);
      const items = gildedRose.updateQuality();

      expect(items).toStrictEqual(expectedResult);
    });
  });
```
This might seem like a lot of preparation for changing some code but by taking our time and properly preparing before actually making the changes we can have certainty that we are not gonna introduce bugs with our refactoring.

---

I know you might be wanting to start refactoring right away, but it would be too much to do in one single article, in a few days we will start fresh and refactor all the code from scratch, in the meantime, I suggest that you take this code and the tests we made today for a spin, do your own refactoring and let's compare with my solution in the next article :smiley:.

After we finish with the refactoring, we are gonna add the feature they requested in the specification, it is gonna be a piece of cake, I promise.

If you liked this please let me know in the comments and please don't forget to share, if you have some comments or suggestions on how to take on this example please also don't forget to comment.