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; }
    }
}

Answer 5

You can go for a format and not a design-pattern in the if-else condition;

Generally, if you have lot of conditions I prefer if than lot of nested if-else's you can opt for something like this;

if(condition1){
   return x;    // or some operation
}

if(condition 2){
   return y;   // or some operation
}

return default; // if none of the case is satisfied.

Answer 6

I really like Leo Lorenzo Luis's Solution. But instead of returning the Rate, I would let the Rule do something with it. This will respect The S. from the S.O.L.I.D. Principles and the Law Of Demeter.

Also, when a class "asks" for a value that is contained into another class, you can identify it as a smell called the data class. You should try to avoid this.

That being said: I would do two things to polish Leo Lorenzo's solution:

1. Call the right Rule without the for loop.
2. Executing the behavior requested inside it's associated Rule class.

In order to do this, we have to map the rule classes with their time range, so they can be accessed directly instead of having to iterate through a loop. You'll need to implement your own Map Object (or List Object, or Collection), overloading the [] operator and it's add function

so you can add your rules in your map like this for example:

ranges.Add(0,500).AddRule(rule1);
ranges.Add(500,1000).AddRule(rule2);
etc.. 

You can see above, there is an Object Range that can have an Object Rule associated to it. So you could eventually add more than one rule for the same Range.

Then, you call it like this:

ranges[Object.time].Execute(Object);

Answer 7

If you have a huge amount of "if" or if you want to put this information in a settings file then I would suggest you create a class to store this information.

Class
    FromTime
    ToTime
    Value

values.Add(New Class(0, 499, .75));
values.Add(New Class(500, 999, .85));
values.Add(New Class(1000, 9999, 1));

Then you loop each items in the collection

if(object.Time >= curItem.FromTime && object.Time <= curItem.ToTime)
    rate = curItem.Value;

You could always have nullable values or set -1 as infinite.

values.Add(New Class(-1, 0, 0));
values.Add(New Class(0, 499, .75));
values.Add(New Class(500, 999, .85));
values.Add(New Class(1000, -1, 1));

if((object.Time >= curItem.FromTime || curItem.FromTime == -1) && (object.Time <= curItem.ToTime || curItem.ToTime == -1))
    rate = curItem.Value;

Answer 8

I do not think this is a anti pattern Problem and for code metrics is also oki. The If is not nested and not very complex!. But you might make better for example using Switch or you make own class which contains properties IsAgeBiggerThanMax() etc.

---

Switch update:

        var range = (time - 1) / 499;
        switch (range)
        {
            case 0:  // 1..499
                rate = 0.75;
                break;
            case 1: // 500.. 999 
                rate = 0.85;
                break;
            default:
                rate = 0;
                if (time == 1000)
                {
                    rate = 1.0;
                }
                break;
        }

Testing is a philosophy question we do not know what is this function and what is doing. maybe it can be tested 100% from outside!

Answer 9

Just do one comparison in each if, and go top-to-bottom with the values:

if (Object.Time >= 1000)
    rate = 1.0;
else
    if (Object.Time >= 500)
        rate = 0.85;
    else
        if (Object.Time > 0)
            rate = 0.75;
        else
            rate = 0;