Replace State-Altering Conditionals with State

• Download source - 7.1 Kb

Introduction

I guess there have been said a lot of good things about design patterns and refactoring as well. The primary point of the article is not to repeat them again. But it’s usually difficult for newcomers (to the world of design patterns or/and refactoring) to get comfortable with them. At least it took me awhile to properly use design patterns, and to finally exclaim that I refactor rather than stupidly rewrite code. This example is based on my speech at our local “User Group” monthly meeting on Feb. 19th 2007.

The main purpose of the example is that using patterns to improve an existing design is better than using patterns early in a new design. This is true whether code is years old or minutes old. We improve designs with patterns by applying sequences of low-level design transformations, known as refactorings Again I repeat the example is simple, so please don’t blame me right away :).

Demo

To try the example you need VS 2005 or 2008, ReSharper 3 (or greater), and NUnit (I got 2.4.3) of course.

User Stories:

1. We want to conduct a membership system to our store.
2. Each customer in the system will be given a Membership Number.
3. We’ll track each customer’s name and balance in the database.
4. Our system will deposit money to increase the balance.
5. Each person using our membership system will get 5% discount for his every buy, if the charge exceeds 150 pounds.

Developer: Since the purpose is demonstration we’ll focus on domain model, and testing, i.e. no UI and database. Let’s start by creating fresh solution with two projects: MyDemo.sln, MyDemo.Core .csproj, and MyDemo.Testing.csproj. And surely, testing project must add reference to NUnit. And your solution must look something like this:

Don’t worry about the other references like System.Core, System.Xml.Linq and etc. They are because I use 3.5 Framework. Let’s start with basic tests about the business object of our membership system, the customer.

CustomerTest.cs in the testing project.

using MyDemo.Core;
using NUnit.Framework;

namespace MyDemo.Testing
{
    [TestFixture]
    public class CustomerTest
    {
        [Test]
        public void TestBasics()
        {
            const int num = 1342;
            Customer c = new Customer(num);
            c.Name = "Khamza Davletov";

            Assert.AreEqual(c.ID, num);    // membership number (ID)

            Assert.AreEqual(c.Balance, 0);    // no balance at first

            c.Deposit(100);
            Assert.AreEqual(c.Balance, 100);    // see if deposit works
        }

        [Test]
        public void TestPurchase()
        {
            Customer c = new Customer(3453);
            
            Assert.AreEqual(c.Balance, .0);
            c.Deposit(1000);
            Assert.AreEqual(c.Balance, 1000);

            c.Purchase(100);
            Assert.AreEqual(c.Balance, 900);

            c.Purchase(200);
            Assert.AreEqual(c.Balance, 710);    // 5% discount
        }
    }
}

Now let’s try unit test the code. We’ll use ReSharper’s NUnit testing support features. Just go to ReSharper->Unit Testing->Run Unit Tests.

Gray, you can easily figure out the reason. It’s because the code doesn’t even compile. Let’s create them and link core project to the testing.

Customer.cs class in the core project.

namespace MyDemo.Core
{
    public class Customer
    {
        public int ID { get; private set; }
        public string Name { get; set; }
        public double Balance { get; private set; }

        public Customer(int membershipNumber)
        {
            Balance = .0;
            ID = membershipNumber;
        }

        public void Deposit(double money)
        {
            Balance += money;
        }
        public void Purchase(double charge)
        {
            double discount = (charge < 150) ? .0 : charge * .05;
            Balance -= charge + discount;
        }
    }
} 

Now, let’s try our tests,

Beautiful! Everything works, the user is happy and pays you for the job. Now, he wants some more features.

New User Stories:

1. We want to add customer privileges like Gold, Silver and no privileged.
2. A customer having more than 5000 pounds in his balance is gold customer, and the one having more than 1000 pounds is silver one, otherwise no privilege.
3. A Gold customer will get 20% discount for his every buy.
4. A Silver customer will get 10% discount for a buying between 100 pounds and 200 pounds, and 15% discount for more than 200 pounds.

Developer: Let’s start with tests for them in CustomerTest class,

[Test]
public void TestGoldCustomerDiscount()
{
    Customer c = new Customer(2363);
    c.Deposit(6000);    // gold customer;

    c.Purchase(1478);
    Assert.AreEqual(c.Balance, 6000 - 1478 * (1 - .2));
}

[Test]
public void TestSilverCustomerDiscount()
{
    Customer c = new Customer(84673);
    c.Deposit(4000);    // silver customer

    c.Purchase(150);    // 10% discount
    Assert.AreEqual(c.Balance, 4000 - 150 * (1 - .1));

    double oldBalance = c.Balance;
    c.Purchase(1240);    // 15% discount
    Assert.AreEqual(c.Balance, oldBalance - 1240 * (1 - .15));
}

If we run tests, we’ll get

We got red light, so let’s make our code to pass the test. We need only rewrite Customer’s Purchase method’s code,

public void Purchase(double charge)
{
    double discountPercentage = .0;
    if (Balance >= 5000)
    {
        // gold customer
        discountPercentage = .2;
    }
    else if (Balance >= 1000)
    {
        // silver one
        if (charge >= 200)
        {
            discountPercentage = .15;
        }
        else if (charge >= 100)
        {
            discountPercentage = .1;
        }
    }
    else
    {
        // no privelege
        if (charge > 150) discountPercentage = .05;
    }
    Balance -= charge * (1 - discountPercentage);
}

Run tests, and we have

TestPurchase is red. This is because it tests overwritten feature that purchase performs. I would like to update this test, to see if no privilege discount works fine and rename it too TestNoPrivelegePurchase.

[Test]
public void TestNoPrivelegePurchase()
{
    Customer c = new Customer(3453);

    Assert.AreEqual(c.Balance, .0);
    c.Deposit(999);
    Assert.AreEqual(c.Balance, 999);

    c.Purchase(100);
    Assert.AreEqual(c.Balance, 899);

    c.Purchase(200);
    Assert.AreEqual(c.Balance, 899 - 200 * (1 - .05));    // 5% discount
}

Now tests succeed.

Although the store becomes more popular and popular, no customer is depositing enough money to become gold one. So user makes a marketing move.

New User Stories: Gold customers are so rare, so we want to increase the amount of money by 10% for each their deposit.

Developer: Here is the test,

[Test]
public void TestGoldCustomerDeposit()
{
    Customer c = new Customer(7245);
    c.Deposit(5400);
    Assert.AreEqual(c.Balance, 5400);

    c.Deposit(430);
    Assert.AreEqual(c.Balance, 5400 + 430 * (1 + .1));
}

Of course, test fails. Rewrite Deposit method of Customer to pass the test,

public void Deposit(double money)
{
    double depositBonus = Balance >= 5000 ? money*.1 : .0;
    Balance += money + depositBonus;
}

And everything becomes ok, and the store begins having gold customers too.

After awhile, the user asks for some reasonable changes.

New User Stories:

1. Now Gold customers will get only additional 6% money for their each deposit.
2. And Silver customers must get plus 5% money for each deposit larger than 600 pounds.

Developer: We’ll update TestGoldCustomerDeposit() test, and add one more for the 2^nd feature,

[Test]
public void TestGoldCustomerDeposit()
{
    Customer c = new Customer(7245);
    c.Deposit(5400);
    Assert.AreEqual(c.Balance, 5400);

    c.Deposit(430);
    Assert.AreEqual(c.Balance, 5400 + 430 * (1 + .06));
}

[Test]
public void TestSilverCustomerDeposit()
{
    Customer c = new Customer(98367);
    c.Deposit(1700);
    Assert.AreEqual(c.Balance, 1700);

    c.Deposit(300);
    Assert.AreEqual(c.Balance, 2000);

    c.Deposit(650);
    Assert.AreEqual(c.Balance, 2000 + 650 * (1 + .05));
}

No surprise they both fail. Let’s pass the tests by modifying Deposit method,

public void Deposit(double money)
{
    double depositBonusPercentage = .0;
    if (Balance >= 5000)
    {
        depositBonusPercentage = .06;
    }
    else if (Balance >= 1000)
    {
        if (money > 600) depositBonusPercentage = .05;
    }
    Balance += money * (1 + depositBonusPercentage);
}

Now all tests pass. But as time goes, the requirements of the user will grow and to prevent the code from becoming “messy” we must take care of some code smells. The most obvious code smell is duplication of if-else clause on privilege condition in two methods. Since percentage calculations of deposit bonus and discount depend on logical privilege state, we can use State Pattern to remove the duplication. We can have base “CustomerPrivelege” class to inherit privilege classes for gold, silver and simple customers. We’ll somehow move the calculation methods to the new class and call them in a polymorphic way.

Let’s create empty CustomerPrivelege class, and introduce its instance as a property of Customer,

public class Customer
{
    public int ID { get; private set; }
    public string Name { get; set; }
    public double Balance { get; private set; }

    private static readonly CustomerPrivelege Privelege = new CustomerPrivelege();

    ...
}

You can run the tests to ensure that nothing is broken. Let’s start refactoring from Deposit method, select code as shown below,

And go to ReSharper->Refactor->Refactor This…, or just Ctrl+Shift+R to get contextual menu on the selected code. Choose Extract Method menu item. You can learn the wizard’s preferences but just press Next button.

And you’ll get the next one. Just inline variable as shown in the picture,

Then Deposit method reduces to compact form. And we can rename depositBonusPercentage to result, since it is a result in the context of the method GetDepositBonusPercentage(…). I think it’s a good practice to rename this kind of variables to “result”.

Run the tests.

Now we have to move GetDepositBonusPercentage method to CustomerPrivelege somehow. We’ll use Martin Fowler’s suggestions about moving method, and some advantages of ReSharper to automate the process.

Let’s make GetDepositBonusPercentage static. Press just next button when wizard pops out.

Now move method using ReSharper’s context menu, or just use shortcut Ctrl+R,O. On the “Move Method” wizard type “CustomerPrivelege”, and while typing you’ll see intallisense like help to choose type from. Choose “Public” as an access right. Press Next button.

And you’ll end up with CustomerPrivelege with static method GetDepositBonusPercentage(double balance, double money). And notice how Deposit method of Customer class has been refactored.

public void Deposit(double money)
{
    Balance += money * (1 + CustomerPrivelege.GetDepositBonusPercentage(Balance, money));
}

Ensure that tests pass.

Now, we need this method to become instance (non-static) method of the class CustomerPrivelege. To do this let’s supply Privilege (CustomerPrivelege) property of Customer class as the first parameter. That’s why we first need to “Change Signature…”

And set the settings like shown in the wizard, and press Next button,

And don’t forget about the tests.

Again, press ReSharper’s “Refactor This…” context menu (Ctrl+Shift+R) and choose “Make Method Non-Static…”, learn the settings, and press Next button. Try running tests now. By now, I think you’ve already accustomed with the amenity of the automated unit tests. So think of using TDD approach in combination of Refactoring.

So, in the same way, we can extract GetDiscountPercentage method, and move it to CustomerPrivelege class. And we’ll end up with code shown below,

// Customer.cs
public class Customer
{
    public int ID { get; private set; }
    public string Name { get; set; }
    public double Balance { get; private set; }

    private static readonly CustomerPrivelege Privelege = new CustomerPrivelege();

    public Customer(int membershipNumber)
    {
        Balance = .0;
        ID = membershipNumber;
    }

    public void Deposit(double money)
    {
        Balance += money * (1 + Privelege.GetDepositBonusPercentage(Balance, money));
    }

    public void Purchase(double charge)
    {
        Balance -= charge * (1 - Privelege.GetDiscountPercentage(Balance, charge));
    }
}

// CustomerPrivelege.cs
public class CustomerPrivelege
{
    public double GetDiscountPercentage(double balance, double charge)
    {
        double result = .0;
        if (balance >= 5000)
        {
        ...
        return result;
    }

    public double GetDepositBonusPercentage(double balance, double money)
    {
        double result = .0;
        if (balance >= 5000)
        {
        ...
        return result;
    }
}

If you couldn’t get here, try several times again. And of course don’t forget run tests periodically to make sure that you haven’t broken anything.

This how we have centralized the duplicated if-else logic depending on customer’s privilege state. Now we need to perform “Replace Conditional with Polymorphism”.

First, let’s inherit 3 different empty subclasses from CustomerPrivelege class, GoldCustomer, SilverCustomer, and SimpleCustomer, and make the two methods virtual. And we must use proper instance for the privilege state of the customer.

public class CustomerPrivelege
{
    public virtual double GetDiscountPercentage(double balance, double charge)
    {...}

    public virtual double GetDepositBonusPercentage(double balance, double money)
    {...}
}

public class GoldCustomer : CustomerPrivelege
{
}

public class SilverCustomer: CustomerPrivelege
{
}

public class SimpleCustomer: CustomerPrivelege
{
}

See if the tests pass. Indeed, we did a tiny step, so it’s obvious that everything will be fine. Actually, we can skip running tests in very tiny, or in automated steps. But it’s better to spend not much time running automated tests, instead of losing more than hours for debugging the code.

Now, let’s create Creation Method in CustomerPrivelege class to correctly create proper instance of its subclass according to the balance parameter,

public class CustomerPrivelege
{
    public static CustomerPrivelege GetByBalance(double balance)
    {
        if (balance >= 5000) return new GoldCustomer();
        if (balance >= 1000) return new SilverCustomer();
        return new SimpleCustomer();
    }
...

Then we can replace static readonly field of Customer class in the following property,

// Old "Privelege" field, you can delete it
// private static readonly CustomerPrivelege Privelege = new CustomerPrivelege();
private CustomerPrivelege Privelege
{
    get { return CustomerPrivelege.GetByBalance(Balance); }
}

Here we definitely need run the tests.

So, we have dealt with the proper instance for the state, and the next step will be actually replacing conditions (if-else clauses) with polymorphism.

Let’s start with GetDiscountPercentage method. In GoldCustomer class, override this method and just copy and paste the code from the code belonging to the gold customer’s condition in the virtual method of the base class.

public class GoldCustomer : CustomerPrivelege
{
    public override double GetDiscountPercentage(double balance, double charge)
    {
        return .2;
    }
}

And for the conviction we can rewrite the condition in the virtual method as follows,

public virtual double GetDiscountPercentage(double balance, double charge)
{
    double result = .0;
    if (balance >= 5000)
    {
        // return .2;
        throw new ApplicationException("Never reaches here!");
    }
    else if (balance >= 1000)
    {
        // silver one
        if (charge >= 200)
        {
            result = .15;
        }
        else if (charge >= 100)
        {
            result = .1;
        }
    }
    else
    {
        // no privelege
        if (charge > 150) result = .05;
    }
    return result;
}

Run the tests.

Let’s move to the next clause in the method. We need copy the code, override the method in SilverCustomer class, and paste the code there, and make some proper improvements as follows,

// CustomerPrivelege
public virtual double GetDiscountPercentage(double balance, double charge)
{
    double result = .0;
    if (balance >= 5000)
    {
        // return .2;
        throw new ApplicationException("Never reaches here!");
    }
    else if (balance >= 1000)
    {
        // silver one
        // if (charge >= 200)
        // {
        //     result = .15;
        // }
        // else if (charge >= 100)
        // {
        //     result = .1;
        // }
        throw new ApplicationException("Never reaches here too!");
    }
    else
    {
        // no privelege
        if (charge > 150) result = .05;
    }
    return result;
}

...
public class SilverCustomer: CustomerPrivelege
{
    public override double GetDiscountPercentage(double balance, double charge)
    {
        double result = .0;
        if (charge >= 200)
        {
            result = .15;
        }
        else if (charge >= 100)
        {
            result = .1;
        }
        return result;
    }
}

Run tests, and for the next clause do the same thing. And you’ll get, virtual method throwing exceptions in all cases. But actually they will never be thrown because of polymorphic calls. To be convinced, let’s remove implementation of the virtual method, and make it abstract method. Of course, the class becomes abstract too, and we can explicitly make protected constructor for the class now. And don’t forget to run the tests periodically.

The second method GetDepositBonusPercentage is left for the reader as an exercise. Now, we have performed the main refactoring here called “Replace State-Altering Conditionals with State”. Let’s continue with other minor refactorings to finish improving the code.

Can you notice that the “balance” parameter is never being used in all override of the both abstract methods? Using ReSharper, we can check it easily, just go to CustomerPrivelege class, place cursor on the parameter, you can even select the parameter, and go to ReSharper->Go To->Usage (Shift+Alt+F12).

And the same for the second method. Now, select the name of the method and perform “Change Signature…”

Remove first parameter in the wizard,

And you’ll notice that the parameter was removed, in subclasses too, automatically! See if tests pass. For the second method the same process.

Points of Interest

That’s all I wanted to present in my article. I guess it would be nice to perform some additional improvements to the code, like “Replace Magic Number with Symbolic Constant” and etc.

History

July 05, 2008 - Posted