I saw this tweet from David Pine earlier and it hit home with me. I’ve always learnt from experienced developers during my career and tried to pass on that knowledge to others as I have progressed. Due to this I thought I would take up the challenge and show my workings on how I would work through improving the code.

Original

Let’s take a look at the original code and determine what the requirements are. These would normally come from your Product Owner or the PBI you have been assigned but we don’t have that, so let’s work it out.

It takes in a decimal total value and wants to output the string representation of the range of monetary value it falls into. Working from this assumption we can determine that total is a money value. Now decimal is fine, however as soon as you start working with exchange rates etc. then a different representation maybe required. This is out of the scope of this.

public class Original
{
    public static string ToStringRangeCurrency(decimal total)
    {
        int number = (int) total;

        if (Enumerable.Range(0, 1000).Contains(number))
        {
            return "$0 - $1000";
        }

        if (Enumerable.Range(1001, 2000).Contains(number))
        {
            return "$1001 - $2000";
        }

        if (Enumerable.Range(2001, 3000).Contains(number))
        {
            return "$2001 - $3000";
        }

        if (Enumerable.Range(3001, 4000).Contains(number))
        {
            return "$3001 - $4000";
        }

        if (Enumerable.Range(4001, 5000).Contains(number))
        {
            return "$4001 - $5000";
        }

        if (number > 5000)
        {
            return "> $5000";
        }

        return "--";
    }
}

Now looking at the code you can see that it wants to work in $1000 ranges and using the Enumerable.Range initially seems a reasonable idea, but as we move through we’ll see it’s not.

Thought Process

Let’s work through the steps I would take to refactor the code.

Unit Tests

Now if the code doesn’t have unit tests we need to make sure the different scenarios are covered so that when we refactor the code we know we’ve not broken the functionality.

[Theory]
[InlineData(1, "$0 - $1000")]
[InlineData(0, "$0 - $1000")]
[InlineData(654, "$0 - $1000")]
[InlineData(1000, "$0 - $1000")]
[InlineData(1001, "$1001 - $2000")]
[InlineData(2510, "$2001 - $3000")]
[InlineData(2999, "$2001 - $3000")]
[InlineData(3698, "$3001 - $4000")]
[InlineData(4125, "$4001 - $5000")]
[InlineData(5001, "> $5000")]
public void Single_Value_In_Returns_Expected_Range(decimal value, string expectedRangeValue)
{
    // Arrange

    // Act
    var result = Original.ToStringRangeCurrency(value);

    // Assert
    result.Should().Be(expectedRangeValue);
}

The above test using the xUnit [Theory] attribute to allow for multiple runs of the unit test with different starting data values. This allows for writing large number of unit tests without repeating the test code. We are also using the FluentAssertions library for the Assert step.

Do they run?

Now we have a suite of tests to check the functionality we need to make sure they run.

Unit tests failing

As we can see, they do not. Now we need to make sure the functionality is working as expected before refactoring otherwise we won’t know if we’ve kept the functionality or not during our refactor.

Make the tests pass

First we need to determine why they are failing. Are the assumptions wrong? Does a certain construct we are using not work as we expect?

Actually in this case it’s the later.

Using Enumerable.Range is a good way of creating a range of values however the usage in this instance is not what the developer is looking for.

If we take a look at the signature of the method we can see why …

public static IEnumerable<int> Range(int start, int count) { ... }

To use this method we specify the starting value and how many to create; not the upper and lower bounds of the range. It’s also worth bearing in mind that it will be zero indexed which is why the test for 1000 fails. As the range created was 0 to 999.

Let’s look at an updated version of the original, using the same constructs, which passes all the tests.

public static string ToStringRangeCurrency(decimal total)
{
    int number = (int) total;

    if (Enumerable.Range(0, 1001).Contains(number))
    {
        return "$0 - $1000";
    }

    if (Enumerable.Range(1001, 1000).Contains(number))
    {
        return "$1001 - $2000";
    }

    if (Enumerable.Range(2001, 1000).Contains(number))
    {
        return "$2001 - $3000";
    }

    if (Enumerable.Range(3001, 1000).Contains(number))
    {
        return "$3001 - $4000";
    }

    if (Enumerable.Range(4001, 1000).Contains(number))
    {
        return "$4001 - $5000";
    }

    if (number > 5000)
    {
        return "> $5000";
    }

    return "--";
}

We start with the value we want and work with $1000 ranges.

We now have the functionality working as we want and all the unit tests pass. Now time to improve with the good old; Red, Green, Refactor mantra.

Refactor

Now as I said before Enumerable.Range is great for creating ranges of values however if you want to check a value is between a minimum and maximum you don’t need the whole range to check that.

If you do a quick google you come across the following answer on Stack Overflow - https://stackoverflow.com/a/3188768/33116 - and it looks like the original developer may have taken the first part of the accepted answer (at the time of writing) and went for that!

int x = 30;
if (Enumerable.Range(1,100).Contains(x))
    //true

Not the second part of the answer which would have been better.

if (x >= 1 && x <= 100)
    //true

As we know from “greater than, less than” maths this is a way we can check if something is between 2 values and hence in the required range.

Due to this you might want to try something like …

public static string ToStringRangeCurrency(decimal total)
{
    int number = (int) total;

    if (number > 5000)
    {
        return "> $5000";
    }

    if (number.IsInBetween(0, 1000))
    {
        return "$0 - $1000";
    }

    if (number.IsInBetween(1001, 2000))
    {
        return "$1001 - $2000";
    }

    if (number.IsInBetween(2001, 3000))
    {
        return "$2001 - $3000";
    }

    if (number.IsInBetween(3001, 4000))
    {
        return "$3001 - $4000";
    }

    if (number.IsInBetween(4001, 5000))
    {
        return "$4001 - $5000";
    }

    return "--";
}

public static bool IsInBetween(this int value, int min, int max) => (value >= min && value <= max);

Don’t worry about abstracting out constructs into different methods and classes as required. Code needs to be readable and maintainable at the end of the day. Show intent through code and then comment if required.

And all the tests pass!

We have now taken a simple constuct we all learnt at school and applied it to the code and we’re not creating a large number of enumerables of ints. I’ve not proven this but I would expect this updated version to perform a bit better due to less itertions going on behind the scenes.

Conclusion

In this post I have run through the basic premise of how I would go about working with a less experienced developer during a code review or pair programming session. Both to improve the code but also help teach and share knowledge.

Now I’m not saying the solution I’ve come up with is the best and you could code golf it down I’m sure but the important thing is maintainablity especially as code bases increase in size and complexity.

Would you do something similar? Would you do something different? I’d be interested in hearing your ideas please contact me on Twitter @WestDiscGolf

Original#

Thought Process#

Unit Tests#

Do they run?#

Make the tests pass#

Refactor#

Conclusion#