Refactoring taken too far

LinkedIn16
Google+0

I came across a tweet today about refactoring badly written code. I’m always interested in that, so I saw a few fellow devs had taken some badly written code and then applied refactoring to it, following good software design principles.

It all started with this article on CodeProject from April last year: https://www.codeproject.com/articles/1083348/csharp-bad-practices-learn-how-to-make-a-good-code

The author shows a piece of badly written and then goes through a slew of refactorings following Liskov, strategy patterms, dependency injection and many other well-known principles.

While I’m a big fan of good software practices, the number 1 principle I like to adhere to is KISS: (Keep it simple, stupid). If you have been reading my blog before, you’ll certainly have come across the theme. I’m not the only though, there were a few follow up posts as well:

I like many of the ideas expressed in the above posts, but I couldn’t stop to think about how this code could be made much simpler. Looking at the posts I still see things that are complicating matters much more than necessary.

Code before refactoring

For reference, this is the initial code from the first post:

public class Class1
{
  public decimal Calculate(decimal amount, int type, int years)
  {
    decimal result = 0;
    decimal disc = (years > 5) ? (decimal)5/100 : (decimal)years/100; 
    if (type == 1)
    {
      result = amount;
    }
    else if (type == 2)
    {
      result = (amount - (0.1m * amount)) - disc * (amount - (0.1m * amount));
    }
    else if (type == 3)
    {
      result = (0.7m * amount) - disc * (0.7m * amount);
    }
    else if (type == 4)
    {
      result = (amount - (0.5m * amount)) - disc * (amount - (0.5m * amount));
    }
    return result;
  }
}

This is indeed not very nice code. What I do like about it though, is that it’s compact. When I read this, I can probably figure out relatively quickly what this code does. It has many problems though (as discussed in the original post).

The “issue” I have with the refactorings in the other posts is that they try to cater for use cases that simply aren’t specified. From what I can tell, these are the requirements:

  • Give a discount based on what type of customer it is
  • Give a loyalty discount which equals the amount of years the customer has been active, with a maximum of 5
  • Both discounts are cumulable

The simplest possible solution

UPDATE: after comments on twitter / reddit, I noticed that the tests were incorrect. I had taken them from one of the refactorings and assumed they were correct. I have modified the data and updated the tests to reflect what the original code does.

UPDATE 2: I went against my own adage: going to far. Smuggling the discount info into the enum was too much. I have refactored it to a dictionary, which is easier to maintain.

static readonly Dictionary<Status, int> Discounts = new Dictionary<Status, int>
{
    {Status.NotRegistered, 0 },
    {Status.SimpleCustomer, 10 },
    {Status.ValuableCustomer, 30 },
    {Status.MostValuableCustomer, 50 }
};
decimal applyDiscount(decimal price, Status accountStatus, int timeOfHavingAccountInYears)
{
    
    price = price - Discounts[accountStatus] * price/100;
    return price - Math.Min(timeOfHavingAccountInYears, 5)* price/100;
}

It’s a simple calculation, so why not express is at as a simple calculation? While I see the power of functional programming, sometimes discriminated unions, partial application and the likes are just overkill for simple problems.

For a coding kata like this, I understand that people want to provide a most elegant solution for future needs, but unfortunately I see these patterns arise far too often, complicating already complex problems even more.

Bonus 1: Tests

[Fact]
public void Tests()
{
    applyDiscount(100m, Status.MostValuableCustomer, 1).Should().Be(49.5000m);
    applyDiscount(100m, Status.ValuableCustomer, 6).Should().Be(66.5000m);
    applyDiscount(100m, Status.SimpleCustomer, 1).Should().Be(89.1000m);
    applyDiscount(100m, Status.NotRegistered, 0).Should().Be(100.0m);
}

Bonus 2:

Want a UI with that? Here you go! :-)