Refactoring if-else if - else

Question

I have the following code example

if(object.Time > 0 && <= 499)
{
     rate = .75m
}
else if(object.Time >= 500 && <= 999)
{
     rate = .85m
}
else if(object.Time >= 1000)
{
     rate = 1.00m
}
else
{
     rate = 0m;
}

My question is what design pattern can I use to make this better?

Edit: Just to clarify a little better, the code you see here is something that currently exists within a Strategy Pattern Implementation. We have 3 types of calculations 2 of which has 3 different "rates" that could be used based off of the time you see below. I thought about creating a strategy implementation for each rate, but then I would be moving the logic for determining the strategy to use and make that a mess as well.

Thanks!

Answer 1

If you're really looking for a design pattern, I'd go for the Chain of Responsibility pattern.

Basically your "link" tries to handle the input. If it is unable to handle it, it's passed down the chain until an other link can handle it. You can also define an interface for easy mocking in your unit tests, if you have some.

So you have this abstract class that every link will inherit :

public abstract class Link
{
    private Link nextLink;

    public void SetSuccessor(Link next)
    {
        nextLink = next;
    }

    public virtual decimal Execute(int time)
    {
        if (nextLink != null)
        {
            return nextLink.Execute(time);
        }
        return 0;
    }
}

And then you create each links with your rules :

public class FirstLink : Link
{
    public override decimal Execute(int time)
    {
        if (time > 0 && time <= 499)
        {
            return .75m;
        }

        return base.Execute(time);
    }
}

public class SecondLink : Link
{
    public override decimal Execute(int time)
    {
        if (time > 500 && time <= 999)
        {
            return .85m;
        }

        return base.Execute(time);
    }
}

public class ThirdLink : Link
{
    public override decimal Execute(int time)
    {
        if (time >= 1000)
        {
            return 1.00m;
        }

        return base.Execute(time);
    }
}

Finally, to use it, just set every successor and call it :

Link chain = new FirstLink();
Link secondLink = new SecondLink();
Link thirdLink = new ThirdLink();


chain.SetSuccessor(secondLink);
secondLink.SetSuccessor(thirdLink);

and all you have to do, is call the chain with one clean call:

var result = chain.Execute(object.Time);

Answer 2

There is a not so famous pattern called 'Rules Pattern'

The idea is that everything is extracted into an object and let it handle its own job. You will have each class for each rule that you defined which is your condition statement, e.g. (object.Time > 0 && <= 499)

public class RuleNumberOne : IRules
{
   public decimal Execute(Oobject date)
   {
      if(date.Time > 0 && date.Something <= 499)
         return .75m;
      return 0;
   } 
} 

public class RuleNumberTwo : IRules
{
    public decimal Execute(Oobject date)
    {
        if(date.Time >= 500 && date.Something <= 999)
            return .85m;
        return 0;
    } 
} 

public interface IRules
{ 
  decimal Execute(Oobject date);
}

Therefore, on your class that used to look like this

if(object.Time > 0 && <= 499)
{
     rate = .75m
}
else if(object.Time >= 500 && <= 999)
{
     rate = .85m
}
else if(object.Time >= 1000)
{
     rate = 1.00m
}
else
{
     rate = 0m;
}

Will now be,

private List<IRules>_rules = new List<IRules>();
public SomeConstructor()
{
    _rules.Add(new RuleNumberOne());
    _rules.Add(new RuleNumberTwo());
}

public void DoSomething()
{

    Oobject date = new Oobject();

    foreach(var rule in this._rules)
    {
        Decimal rate = rule.Execute(date);
    }
}

The idea here is that once you get nested if conditions, it would be harder to read the condition statements and its hard for the developer to make any changes. Thus, it separates the logic of each individual rule and its effect into its own class which follows the rule Single Responsibility Pattern.

Some considerations are 1.) Read only 2.) Explicit order 3.) Dependencies 4.) Priority 5.) Persistence

Again, consider using the Rules Pattern when you have a growing amount of conditional complexity and your application's requirements warrants it.

You can customize it if you want don't want it to return decimal or something but the idea is here.

Answer 3

You only need to check one endpoint of the range. The other one is implied by your actually being at that point in the code, since the earlier conditions were false.

if (obj.Time <= 0) {
    rate = 0.00m;
}

// At this point, obj.Time already must be >= 0, because the test
// to see if it was <= 0 returned false.
else if (obj.Time < 500) {
    rate = 0.75m;
}

// And at this point, obj.Time already must be >= 500.
else if (obj.Time < 1000) { 
    rate = 0.85m;
}

else {
    rate = 1.00m;
}

It would be better to make the more common end of the scale the one you check first, for readability and performance reasons. But it'll work either way.

Answer 4

Using a map:

var map = new[]
{
    new { Rule = (Func<Oobject, bool>) ( x => x.Time > 0 && x.Something <= 499 ), 
          Value = .75m },
    new { Rule = (Func<Oobject, bool>) ( x => x.Time >= 500 && x.Something <= 999 ), 
          Value = .85m },
    new { Rule = (Func<Oobject, bool>) ( x => true ), 
          Value = 0m }
};

var date = new Oobject { Time = 1, Something = 1 };
var rate = map.First(x => x.Rule(date) ).Value;

Assert.That( rate, Is.EqualTo(.75m));

---

I like the idea of @lll's Rules Pattern answer but it has a flaw.

Consider the following test (NUnit):

[Test]
public void TestRulesUsingList()
    var rules = new IRules[]{ new RuleNumberOne(), new RuleNumberTwo() };

    var date = new Oobject { Time = 1, Something = 1 };
    var rate = 0m;

    foreach(var rule in rules)
        rate = rule.Execute(date);

    Assert.That( rate, Is.EqualTo(.75m));
}

The test fails. Although RuleNumberOne was called and returned a non-zero value, RuleNumberTwo was subsequently called and returned zero to overwrite the correct value.

In order to replicate the if..else..else logic, it need to be able to short circuit.

Here's a quick fix: change the interface's Execute method to return a bool to indicate whether to rule should fire and add a Value property to get the rule's decimal value. Also, add a defulat rule that alwasys evaluates true and returns zero. Then change the implementation (test) to get the value of the first rule to evaluate true:

[Test]
public void TestRulesUsingList2()
{
    var rules = new IRules[]{ new RuleNumberOne(), new RuleNumberTwo(), 
        new DefaultRule() };

    var date = new Oobject { Time = 1, Something = 1 };
    var rate = rules.First(x => x.Execute(date)).Value;

    Assert.That( rate, Is.EqualTo(.75m));
}

public class Oobject
{
    public int Time { get; set; }
    public int Something { get; set; }
}

public interface IRules
{ 
    bool Execute(Oobject date);
    decimal Value { get; }
}

public class RuleNumberOne : IRules
{
    public bool Execute(Oobject date)
    {
        return date.Time > 0 && date.Something <= 499;
    }

    public decimal Value
    {
        get { return .75m; }
    }
} 

public class RuleNumberTwo : IRules
{
    public bool Execute(Oobject date)
    {
        return date.Time >= 500 && date.Something <= 999;
    }

    public decimal Value
    {
        get { return .85m; }
    }
} 

public class DefaultRule : IRules
{
    public bool Execute(Oobject date)
    {
        return true;
    }

    public decimal Value
    {
        get { return 0; }
    }
}