Refactoring from if-else-if to Dictionary

Question

I have come across the following piece of code

 if (Region.Current == Region.EU)
    {
      return RegionalPriceLN;
    }
    else if (Region.Current == Region.NY)
    {
      return RegionalPriceNY;
    }
    else if (Region.Current == Region.HK)
    {
      return RegionalPriceHK;
    }
    else if (Region.Current == Region.TK)
    {
      return RegionalPriceTK;
    }

    return null;

And want to refactor it to use a Dictionary. Was thinking of something like below:

return Map.TryGetValue(Region.Current, out func) ? func() : null;

And the dictionary being initialized like this:

 Map = new Dictionary<Region, Func<double?>>
                             {
                               {Region.EU, () => RegionalPriceLN},
                               {Region.NY, () => RegionalPriceNY},
                               {Region.HK, () => RegionalPriceHK},
                               {Region.TK, () => RegionalPriceTK}
                             };

Another approach, to optimize memory usage would be to hold a static dictionary instead of an instance one:

private static readonly Dictionary<Region, Func<TickingBasket, double?>> Map = new Dictionary<Region, Func<TickingBasket,double?>>
                             {
                               {Region.EU, t_ => t_.RegionalPriceLN},
                               {Region.NY, t_ => t_.RegionalPriceNY},
                               {Region.HK, t_ => t_.RegionalPriceHK},
                               {Region.TK, t_ => t_.RegionalPriceTK}
                             };

This would require for us to pass "this" to the delegate:

return Map.TryGetValue(Region.Current, out func) ? func(this) : null;

Can you think of a more elegant way to do this?

Regards.

UPDATE: Region is a reference type.

Answer 1

Second option is definitely a preferred way over first one. Also you may consider a switch statement.

UPDATE. Since Region is a class then the only good way to avoid long if...else is the mapping like in your second variant

First option that you proposed (Map = new Dictionary<Region, Func<double?>>) requires the construction of Dictionary each time the method is executed. It is a waste of resources and time, and for this case even the if..else..if..else statements don't look that ugly :).

Second option (static Dictionary with fixed mappings) is a traditional way to branch the logic for your case. Moreover in certain cases C# compiler will compile the switch statement into static Dictionary to run lookups.

Answer 2

To me. the design as far as I get it, seems a little bit smelly. I do not know engough of your code or the domain and your concrete implementations, but I would prefer other solutions to the dictionary-lookup or switch-statements.

Perhaps you could make use of the decorator-Pattern:

class Product
{
 // ...
    decimal price;
    decimal virtual Price
    {
        get
        {
            return price;
        }
    }
}

And for the EU-Region you generate an EU-Product-Decorator like

class EUProduct:Product
{
    Product product;

    //other stuff

    public override decimal Price
    {
        get
        {
            return Decimal.Multiply(product.Price,new decimal(1.15));
        }
    }

    public Region Region
    {
        get
        {
            return Region.EU;
        }
    }

    // more stuff to go        

    public EUProduct(Product product)
    {
        this.product = product;
    }
}

And the Class Region could be refactored to a simple Enum (as it goes along with your further domainlogic).

Answer 3

Is there a reason why you cant encapsulate this logic into Region class? Calling Region.Current.GetPrice(basket) (or, to go even further, Region.Current.PricingStrategy.GetPrice(basket)) looks so much cleaner to me.

Or, at least, you can wrap your last line into an extension method.