1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ What is the rationale for storing anonymous functions rather than the objects themselves? \$\endgroup\$ Jun 21 '13 at 9:34
  • \$\begingroup\$ Your solution is concise, but it hurts my eyes to read it. Replacing the if/else if with a switch statement would simplify the code slightly but it would be very readable and easy to understand even for a novice programmer. \$\endgroup\$
    – Tim Long
    Jun 21 '13 at 18:46
3
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Switch is not possible because Region is a reference type. \$\endgroup\$
    – Alberto
    Jun 20 '13 at 14:20
  • \$\begingroup\$ You haven't provided the code for Region, and it looked like enum... Now that you mentioned it I see that you actually use Region.Current. So the only good option is your second variant \$\endgroup\$
    – almaz
    Jun 20 '13 at 14:23
  • \$\begingroup\$ Have just updated the question to reflect that. \$\endgroup\$
    – Alberto
    Jun 20 '13 at 14:27
  • \$\begingroup\$ "Second option is definitely a preferred way over first one." Could you elaborate on this statement? \$\endgroup\$
    – MattDavey
    Jun 21 '13 at 8:22
  • \$\begingroup\$ @MattDavey updated my answer \$\endgroup\$
    – almaz
    Jun 21 '13 at 8:34
3
\$\begingroup\$

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).

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ That would make region aware of things it shouldn't be, and a possible violation of SRP. He could get away with it IF the Region and the Price were ALWAYS together. In this case it does not appear to be that way. \$\endgroup\$ Jun 21 '13 at 11:37
  • \$\begingroup\$ @Robert Snyder, thats why i'm asking :) For me its hard to tell from the code provided, what Region should and should not "know". Your explanation makes sense tho, this is probably the case. \$\endgroup\$
    – Nikita B
    Jun 21 '13 at 12:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.