Get Even More Visitors To Your Blog, Upgrade To A Business Listing >>

Refactoring taken too far

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:

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:

  • Don’t let cleaning go overboard (by Ralf Westpahl)
  • An F# rewrite (by Roman Bassart)
  • Using C# 7 and Succin to give F# a run for its money (by David Arno)

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

enum Status { NotRegistered = 0, Simple = 1, Valuable = 3, MostValuable= 5 }
class DiscountManager 
  decimal applyDiscount(decimal price, Status status, int yearsAsCustomer)
    price = price - (int)status * price/100; 
    return price - Math.Min(yearsAsCustomer, 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

public void Tests()
    applyDiscount(100m, Status.MostValuable, 1).Should().Be(94.05000m);
    applyDiscount(100m, Status.Valuable, 6).Should().Be(92.15000m);
    applyDiscount(100m, Status.Simple, 1).Should().Be(98.01000m);
    applyDiscount(100m, Status.NotRegistered, 0).Should().Be(100.0m);

Bonus 2:

Want a UI with that? Here you go!

The post Refactoring taken too far appeared first on Kenneth Truyers.

This post first appeared on Kenneth Truyers Development, please read the originial post: here

Share the post

Refactoring taken too far


Subscribe to Kenneth Truyers Development

Get updates delivered right to your inbox!

Thank you for your subscription