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

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. 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, as a testing framework we are gonna use Jest, 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

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:

  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.

 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 😃.

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.