Test-driven development with legacy code

You may have probably dreamed of living in an ideal world, where you work creating master pieces of software. In that world, you always start software projects from scratch and you strive to produce beautiful, self-descriptive, reliable code. Unfortunately, the above is most of the times not granted to you, because you are often responsible for understanding not that beautiful code, created by other (unsane) programmers.

With the aim of preparing you to face such a reality, in this lab session, we will use an exercise that mimics the situation. We would like to explore the benefits of writting test cases before engaging in refactoring legacy code. To this end, we have selected one of the classic refactoring katas, which we have adapted for its use with the Elixir programming language.

Let us first introduce the scenario of the problem. Note that the text below will be also taken as our input for understanding the software requirements implemented by the code that we are going to analyze.

Gilded Rose Requirements

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

Source: https://iamnotmyself.com/2011/02/13/refactor-this-the-gilded-rose-kata/

The original code for the kata above has been written in C#. I translated that code into Elixir. You can find the current project code here. Please note that tried to keep the code as close as the original. As a consequence, one can clearly feel the imperative styled used by the original programmer.

The code is composed basically of two modules. The module Item (file lib/item.ex) contains the definition of a struct. As stated in the scenario, this module should not be touched during the refactoring. That requirement has important implications in the object oriented world, but has little impact in the functional world. The second module, called GildedRose (file lib/gilded_rose.ex includes only one function, well, only one moster to be more precise. It is there that we are going to concentrate our efforts.

In the original implementation, the code included a small fragment that exemplifies the use of the function GildedRose.update_quality/1. I removed that fragment from the code and copied it below.

arr = [
  %Item{name: "+5 Dexterity Vest", sell_in: 10, quality: 20},
  %Item{name: "Aged Brie", sell_in: 2, quality: 0},
  %Item{name: "Elixir of the Mongoose", sell_in: 5, quality: 7},
  %Item{name: "Sulfuras, Hand of Ragnaros", sell_in: 0, quality: 80},
  %Item{name: "Backstage passes to a TAFKAL80ETC concert", sell_in: 15, quality: 20},
  %Item{name: "Conjured Mana Cake", sell_in: 3, quality: 6}
]
GildedRose.update_quality(arr)

You can see that the function GildedRose.update_quality/1 takes as input a list of items. As a result, we should expect a new list with values sell_in and quality being updated for each item, according to the rules discribed in the scenario. Take some time to run the code above, e.g. using iex.

Adding test cases

Believe it or not, the function GildedRose.update_quality/1 implements the requirements described the scenario. However, the code does not include any test case. You can check that by yourself. I hope you agree with me that adding the support for the new product (i.e. the “Conjured” item) seems a mission impossible. Clearly, we need to refactor the code but we need to warranty that our refactoring does not change the behavior of the code. To that end, we will first write test cases, which we would repeatedly use to verify whether our changes are behavior-preserving or not.

Interestingly, the scenario is written in a format that clearly describe the software requirements. I believe that the first requirement from which we can derive a test case is captured in the following sentence:

  • At the end of each day our system lowers both values for every item

Such sentence can be straightforwardly translated into code. The following code snippet captures the requirement. Create a new file called test/gilded_rose_test.exs and copy there the snippet below.

defmodule GildedRoseTest do
  use ExUnit.Case

  test "At the end of each day our system lowers both values for every item" do
    item = %Item{name: "+5 Dexterity Vest", sell_in: 10, quality: 20}
    [updated_item] = GildedRose.update_quality([item])
    assert updated_item.sell_in == item.sell_in - 1
    assert updated_item.quality == item.quality - 1
  end
end

Use the command mix test to run the tests. I would expect the test above to pass without any problem. So, now it is your turn. Read carefully the scenario and write down the set of test cases for the requirements that you identify there.

Keeping track of code coverage

One requirent question a get from students in this course is how to known whether we have enough test cases for one’s code. In fact, this is a difficult question to answer which has motivated lots of research and is somehow still open. However, one way to understand if you have already a good enough test suite is the notion of code coverage. Well, there are also different ways to measure code coverage, e.g. set of lines covered by the tests, frequency of its use during testing, set of conditional branches traversed by tests, etc. Here, for the purpose of the course we are going to use the tool coverex. The tool is easy to setup and to use, and generates nice reports on HTML format that are easy to understand. To use coverex, you just need add the following lines to the file mix.exs on your project.

  def project do
    [
      app: :gilded_rose,
      version: "0.1.0",
      elixir: "~> 1.5",
      start_permanent: Mix.env == :prod,
      deps: deps(), # DO NOT FORGET TO ADD THIS COMMA
      test_coverage: [tool: Coverex.Task] # THIS IS THE LINE TO ADD!!
    ]
  end

and

  defp deps do
    [
      {:coverex, "~> 1.4.10", only: :test}  # THIS IS THE LINE TO ADD!!
    ]
  end

Now run the command mix deps.get to download the code of the new dependency. Hereinafter, you can use the command mix test --cover to launch the recollection of coverage information. The coverage reports are generated within the directory cover/. Open the file cover/Elixir.GildedRose.html in an HTML browser and take a look at the report. You will notice that most of the lines are green, meaning that they have been exercised by a test case, but several other are red. Take some time to figure out a set of test cases to cover the missing lines.

Refactoring the code

In the literature, you will find a lot of books and articles describing patterns and practices for refactoring code. Most of them, though, are specific to refactoring object oriented code. Surprisingly, there is not much written about refactoring functional code with few exceptions. At the bottom of this document, you will find some pointers. However, for this exercise we are going to introduce some strategies that could be useful not only in this exercise but also in other contexts.

  • Identify for duplicate code and extract a function out of it

You will notice that the following snippet is repeated several times through out the code

if itemp.quality < 50 do
  %{itemp | quality: itemp.quality + 1}
else
  itemp
end

Clearly, we can create a function out of it and replace this fragment by a call to the newly added function.

def increase_quality(item) do
  if item.quality < 50 do
    %{item | quality: item.quality + 1}
  else
    item
  end
end

Do you see other opportunities for refactoring on the resulting code that are similar to the one above?

  • Transform negative conditions into positive ones

It is commonly accepted (and most likely proved by research somewhere) that the use of negative conditions makes the code more difficult to understand. That is why, it is a good practice to identify the negative conditions and replace them by their equivalent positive ones. In this context, you will have to recall the well-known De Morgan laws. Anyway, here is an example fragment where we can apply this transformation.

The rule above implies that the following code:

if item.name != "Sulfuras, Hand of Ragnaros" do
  %{item | sell_in: item.sell_in - 1}
else
  item
end

should be replaced by:

if item.name == "Sulfuras, Hand of Ragnaros" do
  item
else
  %{item | sell_in: item.sell_in - 1}
end

Please, pay attention to how the code has been redistributed as as consequence of this change. Now, with this new version we can clearly see that the code captures the following two complementary requirements:

  • At the end of each day our system lowers both values for every item
  • “Sulfuras”, being a legendary item, never has to be sold or decreases in Quality

I think that it now clear that the purpose of this fragment of code is to update the value of sell_in. Let us extract a function with the code above as follows:

def update_sell_in(item) do
  if item.name == "Sulfuras, Hand of Ragnaros" do
    item
  else
    %{item | sell_in: item.sell_in - 1}
  end
end
  • Relocate code fragments to allow code merging

As we progress with the refactoring, we gain some understanding of the code, particularly when it starts to shrink. At this point in time I expect that you have already see that the original code is composed of three major blocks: The first block (a big nested if/else) updates the quality of items indistinctingly, the second block updates the sell_in value, and the final block updates once again the quality of items if the sell_in value has already passed.

In this context, I propose you to pull up the second block. This action would bring together the two blocks that update the quality of items. My intention is that you could eventually merge together the two blocks of code.

Note that pulling up the second block would have an impact on the first block. Hence, I will ask you to update the code accordingly. I will let you do this refactorings by yourselves.

  • Transform the if/else, case and cond blocks into function cases

This transformation is definitely optional. But let me give you an example. On the second refactoring, we extracted the following function:

def update_sell_in(item) do
  if item.name == "Sulfuras, Hand of Ragnaros" do
    item
  else
    %{item | sell_in: item.sell_in - 1}
  end
end

If we use some pattern matching, we can derive the following function cases:

def update_sell_in(%{name: "Sulfuras"<>_} = item), do: item
def update_sell_in(item), do: %{item | sell_in: item.sell_in - 1}

In my opinion, the code above is more intuitive.

  1. If the item has a name that starts with the prefix “Sulfuras” (take into consideration that I am using string concatenation as part of the pattern matching - awesome, is it?) we do not update anything.
  2. In all the other cases, we decrease the value of sell_in for a given item.

I think we still have some other strategies to follow. For instance, we can split the composite conditions on if/else blocks and redistribute their corresponding bodies. The latter would result in some code duplication. However, you will see that this strategy would enable the extraction of functions with separation of cases.

The redistribution of if/else blocks will also enable the code merging and extraction of functions. But, I will leave the rest to you. Good luck!