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
andcond
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.
- 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.
- 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!