Refactoring to maintainability

Services About Me Reading Videos Blog

Introducing F# onto your systems

Published in Functional Programming on

The setup

So, it is my turn to write something for the F# Advent. It is the first time I do it, and well, there are lots of good people I know there. Scary. On top of that, I think I have ever only written a single blog post of F#. So yeah, this will be interesting.

I thought to myself, let’s start from the basics. Maybe we can explore one of the recommendations of Scott Waschlin on F# for fun and profit on how to start using F# through tests. So the audience for this post is .Net people that haven’t yet tried F#, or have an inkling and want to look at options on how to use it at work.

My initial idea was to compare how the tests looked liked using a kata like the Mars Rover. Then I decided to use the Gilded Rose as the code was already written (you can find the original code here at NotMyself’s Github repo). And once I started, things went further than I originally intended in size, although I left a couple of things out that I had an idea of talking about (maybe something for the future). So strap in because there is a lot of code below.

First steps

I recommend you to see the original code, to know what is going on here. Also, there is a lot of C# code on the examples. I even maintained the C# test till the very end.

So the first step was to write tests, both on C# and F#, to see how much different they actually were. Below you can see part of the C# test suite.

namespace GildedRose.Tests
{
    #pragma warning disable CS8321, xUnit1013
    public class BaseRules
    {
        public const string Brie = "Aged Brie";
        public const string Vest = "+5 Dexterity Vest";
        public const string Sulfuras = "Sulfuras, Hand of Ragnaros";
        public const string Backstage = "Backstage passes to a TAFKAL80ETC concert";
        
        public Item FindItemByName(IList<Item> items, string name)
        {
            return items.First(x => x.Name.Equals(name));
        }

        public void StandardItemRules()
        {
            [Fact]
            void VestItemDegradesQualityBeforeSellIn()
            {
                var app = new Program() { Items = new List<Item> {new Item {Name = Vest, SellIn = 2, Quality = 20}}};
                var item = FindItemByName(app.Items, Vest);
                Assert.Equal(20, item.Quality);
                app.UpdateQuality();
                Assert.Equal(19, item.Quality);
            }

            [Fact]
            void AnItemSellInGetsReduced()
            {
                var app = new Program() { Items = new List<Item> {new Item {Name = Vest, SellIn = 2, Quality = 3}}};
                var item = FindItemByName(app.Items, Vest);
                Assert.Equal(2, item.SellIn);
                app.UpdateQuality();
                Assert.Equal(1, item.SellIn);
            }

            [Fact]
            void ItemDegradesQualityTwiceAfterSellIn()
            {
                var app = new Program() { Items = new List<Item> {new Item {Name = Vest, SellIn = 0, Quality = 3}}};
                var item = FindItemByName(app.Items, Vest);
                Assert.Equal(3, item.Quality);
                app.UpdateQuality();
                Assert.Equal(1, item.Quality);
            }

            [Fact]
            void AnItemDontGoBelowZeroQuality()
            {
                var app = new Program() { Items = new List<Item> {new Item {Name = "Test", SellIn = 1, Quality = 0}}};
                var item = FindItemByName(app.Items, "Test");
                Assert.Equal(0, item.Quality);
                app.UpdateQuality();
                Assert.Equal(0, item.Quality);
            }
        }

There are a couple things that I want to point out:

  • I think the ability to have methods within methods in C# is relatively new. This allowed me to follow a setup that I originally saw with … Mocha? (can’t remember). Even more interesting, I could mark the internal methods with [Fact].
  • I have to add the #pragma warning disable to take care of two warnings relating to the fact that the container method didn’t have the annotation, and that the internal methods were not being called (xUnit is taking care of executing them)

Now the first version of the F# tests follows quite closely the C# counterpart

module Tests =

    let findItemByName (app: Program) name =
        app.Items |> Seq.toList |> List.find (fun x -> x.Name = name)
        
    module StandardItem =
        let Vest = "+5 Dexterity Vest"

        [<Fact>]
        let ``Standard item degrades quality before "sell in"`` () =
            let app = new Program()
            app.Items <- ResizeArray<Item>([new Item (Name = Vest, SellIn = 2, Quality = 20)])
            let item = findItemByName app Vest

            Assert.Equal(20, item.Quality)
            app.UpdateQuality()
            Assert.Equal(19, item.Quality)


        [<Fact>]
        let ``Standard item sell in gets reduced`` () =
            let app = new Program()
            app.Items <- ResizeArray<Item>([new Item (Name = Vest, SellIn = 2, Quality = 20)])
            let item = findItemByName app Vest

            Assert.Equal(2, item.SellIn)
            app.UpdateQuality()
            Assert.Equal(1, item.SellIn)

        [<Fact>]
        let ``Standard item degrades in quality after "sell in"`` () =
            let app = new Program()
            app.Items <- ResizeArray<Item>([new Item (Name = Vest, SellIn = 0, Quality = 20)])
            let item = findItemByName app Vest

            Assert.Equal(20, item.Quality)
            app.UpdateQuality()
            Assert.Equal(18, item.Quality)

        [<Fact>]
        let ``Standard item don't go below 0 quality`` () =
            let app = new Program()
            app.Items <- ResizeArray<Item>([new Item (Name = Vest, SellIn = 0, Quality = 0)])
            let item = findItemByName app Vest
            
            Assert.Equal(0, item.Quality)
            app.UpdateQuality()
            Assert.Equal(0, item.Quality)

First thing that you will notice is the name of the functions. Using the two backticks allows you to use actual sentences. This makes it immediately more readable than the equivalent C# method names.

The <- operator is the assignment operator. Unlike nearly most languages there is a separation between the assignment operator and the initialization operator. You will see as well ResizeArray and Seq.toList. These two are needed because F# lists are not C# lists. Markedly, F# lists (like nearly everything in F#, by default) are immutable. But the list that we have in the code is the mutable C# list. So we have to do a bit of jiggling to get it to match as we want.

I found that, above all, this contrast between immutability and mutability is what truly distinguish F# from C#. And the advantages of immutability over mutability in terms of correctness of the code and the easiness to develop similar functionality are great enough that I think it should be the default approach. Of course, there are always trade offs, the way that you code must change (you know, the functional style), and performance could be an issue (not that performance cannot be an issue at times on C#, anyway).

The |> operator is just a pipe. The result of the left operation gets passed as the last parameter of the right operation.

I am using module similarly to how I was doing the method within method on the C# code. F# has been doing function within function since the beginning, but because is not exactly the same as the method within method of C#, it cannot be annotated (pity).

They final result looks similar between both languages, and is not really taking advantage of F# due to the fact that the gilded rose code is actually a single method, very procedural. So you can say that is not really taking advantage either of C#.

And then here is when I decided to start refactoring the original code (which is, in fact, the point of the Gilded Rose exercise) and start moving towards a full F# version. Because.

The beginning of the refactoring

So first lets add some constants, to avoid using as much magic strings. I could have done the same with some of the magic numbers, really (50 and 0 being the ones that definitely would do if I change this code again).

public const string Brie = "Aged Brie";
public const string Vest = "+5 Dexterity Vest";
public const string Elixir = "Elixir of the Mongoose";
public const string Sulfuras = "Sulfuras, Hand of Ragnaros";
public const string Backstage = "Backstage passes to a TAFKAL80ETC concert";
public const string ConjuredCake = "Conjured Mana Cake";

And then we go and extract all the code onto three separate methods, making the main method a better description of what is happening.

public void UpdateQuality()
{
    for (var i = 0; i < Items.Count; i++)
    {
        UpdateQualityBeforeSellIn(Items[i]);;
        UpdateSellIn(Items[i]);
        UpdateQualityAfterSellIn(Items[i]);
    }
}

In fact, LinQ was a superb addition to C#, so let’s use it. The for loop is just unneeded noise here.

public void UpdateQuality()
{
    Items.ToList().ForEach(item => {
        UpdateQualityBeforeSellIn(item);;
        UpdateSellIn(item);
        UpdateQualityAfterSellIn(item);
    });
}

The ToList() call in there is needed because ForEach() works on IEnumerable not IList.

Next, we do a change based on the Transformation Priority Premise (level 14), in which we change the multiple nested if for switches. This change will be quite handy later on, but also makes the code more readable right now. It does involve moving around a bit what gets tested first. But is worthy of the change.

private void UpdateQualityAfterSellIn(Item item)
{
    if (item.SellIn >= 0) return;

    switch (item.Name)
    {
        case Brie:
            if (item.Quality < 50)
            {
                item.Quality = item.Quality + 1;
            }
            return;
        case Sulfuras:
            return;
        case Backstage:
            item.Quality = item.Quality - item.Quality;
            return;
        default:
            if (item.Quality > 0)
            {
                item.Quality = item.Quality - 1;
            }
            return;
    }
}

The first F# structure

Ok, if you look at the changes, and you have experience with OOP you can see an interface screaming to get out and a strategy pattern forming if we go the route of amplifying the structure Item (it is a class on C# but right now if functions as a data structure).

But what if we do changes that will be more suitable for F# (which can do OOP anyway).

So lets create and FSharp library that we are going to reference on the C# console app. And on it we will put an F# Record type. This is our data structure.

module Data =
    type Item =
        { mutable Name : string
          mutable SellIn : int
          mutable Quality : int}

The mutable keyword is added because by default everything is immutable on F#, but we want to take the steps slowly and we need the elements to be mutable for now on the C# version of the code.

We can reference this library on the Console app.

    public class Program
    {
        public IList<Data.Item> Items;
        static void Main(string[] args)
        {
            System.Console.WriteLine("OMGHAI!");

            var app = new Program()
            {
                Items = new List<Data.Item>
                {
                    new Data.Item(Data.Vest, 10, 20),
                    new Data.Item(Data.Brie, 2, 0),
                    new Data.Item(Data.Elixir, 5, 7),
                    new Data.Item(Data.Sulfuras, 0, 80),
                    new Data.Item(Data.Backstage, 15, 20),
                    new Data.Item(Data.ConjuredCake, 3, 6)
                }
            };

            app.UpdateQuality();

            System.Console.ReadKey();

        }

Of course, you should have changed first the test to indicate the direction you were going.

[Fact]
void VestItemDegradesQualityBeforeSellIn()
{
    var app = new Program() { Items = new List<Data.Item> {new Data.Item (Vest, 2, 20)}};
    var item = FindItemByName(app.Items, Vest);
    Assert.Equal(20, item.Quality);
    app.UpdateQuality();
    Assert.Equal(19, item.Quality);
}

The Record type creates a constructor for you to use on C# code, where the elements appear in the same order.

And this is the F# version of the test:

[<Fact>]
let ``Standard item degrades quality before "sell in"`` () =
    let app = new Program()
    app.Items <- ResizeArray<Item>([ {Name = Vest; SellIn = 2; Quality = 20}])
    let item = findItemByName app Vest

    Assert.Equal(20, item.Quality)
    app.UpdateQuality()
    Assert.Equal(19, item.Quality)

Look at F#, not declaring the type when using the record {Name = Vest; SellIn = 2; Quality = 20}. It knows that the only record type that has that exact format is my new Item, therefore it knows what it is. I can’t stress enough how great is to have a compiler that is statically strongly typed and yet doesn’t force you to write every single type all over (check about Hindley-Milner type systems if you are interested).

Initial logic translations

Next step is so start moving some functionality from the C# console app to the F# library. This is a string refactoring in terms of the usage being exactly the same as a C# method.

let changeQualityMut item amount =
    item.Quality <- item.Quality + amount
        
let reduceSellInMut item =
    item.SellIn <- item.SellIn - 1

You can see again the <- operator being used to assign the new values.

Using them within the C# code is quite easy.

private void UpdateSellIn(Data.Item item)
{
    switch (item.Name)
    {
        case Data.Sulfuras:
            return;
        default:
            Data.reduceSellInMut(item);
    }
}

Ok, the next part requires a small lateral (even backward) move. Do you know how in resolving maths equation sometimes you introduce some elements that doesn’t change the final value of the equation so you can do reductions not available before? (like adding a *2 /2 … wasn’t that good at that back in school). So look at the introduction below of return values from the extracted methods …

private Data.Item UpdateQualityAfterSellIn(Data.Item item)
{
    if (item.SellIn >= 0) return item;

    switch (item.Name)
    {
        case Brie:
            if (item.Quality < 50)
            {
                Data.changeQualityMut(item, 1);
            }
            return item;
        case Sulfuras:
            return item;
        case Backstage:
            Data.changeQualityMut(item, -item.Quality);
            return item;
        default:
            if (item.Quality > 0)
            {
                Data.changeQualityMut(item, -1);
            }
            return item;
    }
}

…and their use on the UpdateQuality method, where the result is being passed to the next element, successively.

public void UpdateQuality()
{
    Items = Items.ToList().Select(item => {
        item = UpdateQualityBeforeSellIn(item);;
        item = UpdateSellIn(item);
        item = UpdateQualityAfterSellIn(item);
        return item;
    }).ToList();
}

The above is interesting because now we can go ahead and, instead of using a mutable version of the functions, we can use a non-mutable version on F#

    let changeQuality item amount =
        { item with Quality = item.Quality + amount }

    let reduceSellIn item =
        { item with SellIn = item.SellIn - 1 }

The above is how you create a new Record instance using an existing record. There is no much to change on the C# methods, only making sure that we are assigning the new values to the item variables.

        private Data.Item UpdateQualityAfterSellIn(Data.Item item)
        {
            if (item.SellIn >= 0) return item;

            switch (item.Name)
            {
                case Brie:
                    if (item.Quality < 50)
                    {
                        item = Data.changeQuality(item, 1);
                    }
                    return item;
                case Sulfuras:
                    return item;
                case Backstage:
                        item = Data.changeQuality(item, -item.Quality);
                    return item;
                default:
                    if (item.Quality > 0)
                    {
                        item = Data.changeQuality(item, -1);
                    }
                    return item;
            }
        }

The tests need to be fixed, as until now they were relying on the fact that items where mutable:

[Fact]
void AgedBrieQualityDoesntIncreaseAfter50()
{
    var app = new Program() { Items = new List<Data.Item> {new Data.Item (Brie, 2, 51)}};
    var item = FindItemByName(app.Items, Brie);
    Assert.Equal(51, item.Quality);
    app.UpdateQuality();
    item = FindItemByName(app.Items, Brie);
    Assert.Equal(51, item.Quality);
}

[<Fact>]
let ``Aged brie quality doesn't increase if already over 50`` () =
    let app = new Program()
    app.Items <- ResizeArray<Item>([ {Name = Brie; SellIn = 2; Quality = 51}])
    let item = findItemByName app Brie

    Assert.Equal(51, item.Quality)
    app.UpdateQuality()
    let itemChanged = findItemByName app Brie
    Assert.Equal(51, itemChanged.Quality)

Now we can do an additional refactor, still based on the fact that there is no mutability.

[Fact]
void VestItemDegradesQualityBeforeSellIn()
{
    var original = new Data.Item (Vest, 2, 20);
    var app = new Program() { Items = new List<Data.Item> {original}};

    app.UpdateQuality();

    var result = FindItemByName(app.Items, Vest);
    Assert.Equal(original.Quality - 1, result.Quality);
}

[<Fact>]
let ``Standard item degrades quality before "sell in"`` () =
    let original = {Name = Vest; SellIn = 2; Quality = 20}
    let app = new Program()
    app.Items <- ResizeArray<Item>([ original ])

    app.UpdateQuality()

    let result = findItemByName app Vest
    Assert.Equal(original.Quality - 1, result.Quality)

There are no longer before and after checks.

Ok, maybe that refactoring could have been done earlier with additional temporal variables holding the int values. But adding complication because of the language limitations is … the life of any developer.

Moving the rest of the logic

Time to move the constants that we created early to their F# counterparts

[<Literal>]
let Brie = "Aged Brie"
[<Literal>]
let Vest = "+5 Dexterity Vest"
[<Literal>]
let Elixir = "Elixir of the Mongoose"
[<Literal>]
let Sulfuras = "Sulfuras, Hand of Ragnaros"
[<Literal>]
let Backstage = "Backstage passes to a TAFKAL80ETC concert"
[<Literal>]
let ConjuredCake = "Conjured Mana Cake"

We have added the [<Literal>] annotation because otherwise C# cannot use them on switch statements. Once we move away from the switches we can remove the annotation.

Nothing much changes on the C# version of the below method

private Data.Item UpdateQualityAfterSellIn(Data.Item item)
{
    if (item.SellIn >= 0) return item;

    switch (item.Name)
    {
        case Data.Brie:
            if (item.Quality < 50)
            {
                item = Data.ChangeQuality(item, 1);
            }
            return item;
        case Data.Sulfuras:
            return item;
        case Data.Backstage:
            item = Data.ChangeQuality(item, -item.Quality);
            return item;
        default:
            if (item.Quality > 0)
            {
                item = Data.ChangeQuality(item, -1);
            }
            return item;
    }
}

But then we can create an F# version that uses match expressions, a turbo-charged version of switchs statements.

    let updateQualityAfterSellIn item =
        if item.SellIn >= 0
        then item
        else
            match item.Name with
            | Brie ->
                if item.Quality < 50
                then changeQuality item 1
                else item
            | Sulfuras -> item
            | Backstage -> changeQuality item -item.Quality
            | _ ->
                if item.Quality > 0
                then changeQuality item -1
                else item

One thing quite noticeable above is that the F# version ends being less verbose, making the whole thing far more readable.

Ok. We have changed the three helping methods into F# methods following the same pattern. What about the main UpdateQuality method? How the equivalent F# function will look like?

    let updateQuality (items) =
        items
        |> List.map updateQualityBeforeSellIn
        |> List.map updateSellIn
        |> List.map updateQualityAfterSellIn

The thing I love the most about the pipe (|>) operator (also on Elixir, Clojure, Haskell …) is that it removes the temporal variables, which are just noise, while keeping a very readable code.

We could go even further and use composition to avoid multiple calls to List.map

let updateQuality items =
    items |> List.map (updateQualityBeforeSellIn >> updateSellIn >> updateQualityAfterSellIn)

To be used on C# we need to do the hoky-poky changes between IList and FSharpList (now from the perspective of C#)

public void UpdateQuality()
{
    Items = Data.UpdateQuality(ListModule.OfSeq(Items)).ToList();
}

But why not then use directly the immutable FSharpList?

public FSharpList<Data.Item> Items;
static void Main(string[] args)
{
    System.Console.WriteLine("OMGHAI!");

    var app = new Program()
    {
        Items = ListModule.OfSeq(new List<Data.Item>
        {
            new Data.Item(Data.Vest, 10, 20),
            new Data.Item(Data.Brie, 2, 0),
            new Data.Item(Data.Elixir, 5, 7),
            new Data.Item(Data.Sulfuras, 0, 80),
            new Data.Item(Data.Backstage, 15, 20),
            new Data.Item(Data.ConjuredCake, 3, 6)
        })
    };

    app.UpdateQuality();

    System.Console.ReadKey();

    }

    public void UpdateQuality()
    {
        Items = Data.updateQuality(Items);
    }

This means we are close to the end!!!

The full transition

There is not much left to transition completely to an F# only system. First let’s first refactor slightly the F# version of the tests to completely avoid the C# code (after all the UpdateQuality method now just calls the updateQuality function).

let findItemByName items name =
    items |> List.find (fun x -> x.Name = name)
        
module StandardItem =
    let Vest = "+5 Dexterity Vest"

    [<Fact>]
    let ``Standard item degrades quality before "sell in"`` () =
        let original = {Name = Vest; SellIn = 2; Quality = 20}

        let actual = updateQuality [original]

        let result = findItemByName actual Vest
        Assert.Equal(original.Quality - 1, result.Quality)

And finally let’s create the equivalent Console application using F#

[<EntryPoint>]
let main argv =
    printfn "We are starting"
    let items = Data.UpdateQuality [{Name = Data.Vest; SellIn = 10; Quality = 20}
                                    {Name = Data.Brie; SellIn = 2; Quality = 0}
                                    {Name = Data.Elixir; SellIn = 5; Quality = 7}
                                    {Name = Data.Sulfuras; SellIn = 0; Quality = 80}
                                    {Name = Data.Backstage; SellIn = 15; Quality = 20}
                                    {Name = Data.ConjuredCake; SellIn = 3; Quality = 6}]
    printfn "%A" items
    
    Console.ReadKey() |> ignore
    0 // return an integer exit code

Not much different at this point.

Well, what a trip. We started with some C# code, and we transitioned, bit by bit to an F# version. There are still things that can be improved, both on the quality of the code, and the use of F# tooling (like FsUnit and Fantomas). Maybe I will do some follow ups.

Epilogue

I have experience in multiple languages (not only hobby wise, but commercially), having used C#, Java, Javascript, Clojure, PHP and others in the past. I do find FP, in general (there are always exceptions), to produce more maintainable and readable code. F# brings an non-intrusive statically compiled system to .Net that is a delight to work with.

You can find the “final” version of the code on my SourceHut repo (using Mercurial, rather than Git)