.Net 4.0 Optimized code for refactoring existing "if" conditions and "is" operator

Question

I have following C# code. It works fine; but the GetDestination() method is cluttered with multiple if conditions by using is operator.

In .Net 4.0 (or greater) what is the best way to avoid these “if” conditions?

EDIT: Role is part of the business model, and the destination is purely an artifact of one particular application using that business model.

CODE

public class Role { }
public class Manager : Role { }
public class Accountant : Role { }
public class Attender : Role { }
public class Cleaner : Role { }
public class Security : Role { }

class Program
{
    static string GetDestination(Role x)
    {
        string destination = @"\Home";

        if (x is Manager)
        {
            destination = @"\ManagerHomeA";
        }

        if (x is Accountant)
        {
            destination = @"\AccountantHomeC";
        }

        if (x is Cleaner)
        {
            destination = @"\Cleaner";
        }

        return destination;

    }

    static void Main(string[] args)
    {
        string destination = GetDestination(new Accountant());
        Console.WriteLine(destination);
        Console.ReadLine();
    }
}

REFERENCES

1. Dictionary<T,Delegate> with Delegates of different types: Cleaner, non string method names?
2. Jon Skeet: Making reflection fly and exploring delegates
3. if-else vs. switch vs. Dictionary of delegates
4. Dictionary with delegate or switch?
5. Expression and delegate in c#

Cardboard

You could replace this with string.Format(@"\{0}Home", x.GetType().Name). If that's a good idea is another question which depends on your design. – Allier

Why not make GetDestination() a method of Role and override it? – Ause

Is there any possibility that any of the classes you've shown might be further subclassed? – Biltong

@Allier No, it is not a simple string manipulation for the type. I have updated the question to reflect it. – Cardboard

@Rik disagree. It's clearly just a sample of code, shown for the purpose of asking about an OOP technique. – Idolater

@JonSkeet In my scenario, there is no chance for getting another level of subclass. However I am curious to know why that information is useful. – Cardboard

@Lijo: Well if you're going to have a Type to string dictionary, you need to know there'll be an exact match for the type of the object... see my answer for an alternative in the case where there might be subclassing though. – Biltong

Answer 1

Approach 1 (Selected): Using dynamic keyword to implement multimethods / double dispatch

Approach 2: Use a dictionary to avoid if blocks as mentioned in Jon Skeet’s answer below.

Approach 3: Use a HashList with delegates if there is condition other than equality (For example, if input < 25). Refer how to refactor a set of <= , >= if...else statements into a dictionary or something like that

Apporach 4: Virtual Functions as mentioned in MarcinJuraszek’s answer below.

MultiMethods / Double Dispatch approach using dynamic keyword

Rationale: Here the algorithm changes based on the type. That is, if the input is Accountant, the function to be executed is different than for Manager.

    public static class DestinationHelper
    {
        public static string GetDestinationSepcificImplm(Manager x)
        {
            return @"\ManagerHome";
        }

        public static string GetDestinationSepcificImplm(Accountant x)
        {
            return @"\AccountantHome";
        }

        public static string GetDestinationSepcificImplm(Cleaner x)
        {
            return @"\CleanerHome";
        }
    }

   class Program
    {
        static string GetDestination(Role x)
        {

            #region Other Common Works
            //Do logging
            //Other Business Activities
            #endregion

            string destination = String.Empty;
            dynamic inputRole = x;
            destination = DestinationHelper.GetDestinationSepcificImplm(inputRole);
            return destination;
        }

        static void Main(string[] args)
        {
            string destination = GetDestination(new Security());
            Console.WriteLine(destination);
            Console.WriteLine("....");
            Console.ReadLine();
        }

    }

Cardboard

Refernce: Change Functionality without changing function – Cardboard

Answer 2

Having virtual property which would be overriden in derived classes should do the trick:

class Role
{
    public virtual string Destination { get { return "Home"; } }
}
class Manager : Role
{
    public override string Destination { get { return "ManagerHome;"; } }
}
class Accountant : Role
{
    public override string Destination { get { return "AccountantHome;"; } }
}
class Attender : Role
{
    public override string Destination { get { return "AttenderHome;"; } }
}
class Cleaner : Role
{
    public override string Destination { get { return "CleanerHome;"; } }
}
class Security : Role { }

I didn't make the property abstract, to provide default Home value when it's not overriden in derived class.

Usage:

string destination = (new Accountant()).Destination;
Console.WriteLine(destination);
Console.ReadLine();

Rely

Thanks.. However this won't work in the following scenario - Role is part of the business model, and the destination is purely an artifact of one particular application using that business model. – Cardboard

@Lijo You should go with Jon's answer then! – Rely

Answer 3

Here's one option:

private static readonly Dictionary<Type, string> DestinationsByType =
    new Dictionary<Type, string> 
{
    { typeof(Manager), @"\ManagerHome" },
    { typeof(Accountant), @"\AccountantHome" },
    // etc
};

private static string GetDestination(Role x)
{
    string destination;
    return DestinationsByType.TryGetValue(x.GetType(), out destination)
        ? destination : @"\Home";
}

Note:

• This doesn't cope with null parameters. It's not clear whether or not you actually need it to. You can easily add null handling though.
• This doesn't copy with inheritance (e.g. class Foo : Manager); you could do that by going up the inheritance hierarchy if necessary

Here's a version which does deal with both of those points, at the cost of complexity:

private static string GetDestination(Role x)
{
    Type type = x == null ? null : x.GetType();
    while (type != null)
    {
        string destination;
        if (DestinationsByType.TryGetValue(x.GetType(), out destination))
        {
            return destination;
        }
        type = type.BaseType;
    }
    return @"\Home";
}

EDIT: It would be cleaner if Role itself had a Destination property. This could either be virtual, or provided by the Rolebase class.

However, it could be that the destination is really not something the Role should concern itself with - it could be that Role is part of the business model, and the destination is purely an artifact of one particular application using that business model. In that sort of situation, you shouldn't put it into Role, as that breaks separation of concerns.

Basically, we can't tell which solution is going to be most suitable without knowing more context - as is so often the way in matters of design.

Biltong

It's so not OO. May I ask why do you offer such solution if better had been already suggested? – Merozoite

@MichalB.: Which better one were you thinking of? While it's nicer if Role can provide this itself, it may be inappropriate. I'll edit my answer to explain what I mean. – Biltong

@MichalB. "better" is a subjective term. We don't know what is actually better in this case. – Kamilahkamillah

I think that MarcinJuraszek's answer is the cleanest. It may be inappropriate when Role class should not provide the destination. But in this case some explanation should be given and a solution suggested (e.g. DestinationProvider) – Merozoite

@MichalB.: I've now updated my answer with explanation - but there's no indication that a whole separate "destination provider" interface is required at all. That sounds like overengineering when we don't have any requirements which suggest it. – Biltong

@JonSkeet Good and useful explanation about Role and Destination - Role is part of the business model, and the destination is purely an artifact of one particular application using that business model.. I really liked it. – Cardboard

@Lijo: Well that's an explanation of why you might not want Role to know about Destination - whether that's actually the case in your situation is a different matter ;) – Biltong

@JonSkeet Though your post answers this question, my real scenario is bit more complex than simply returning a string. It has some logic to be performed based on each role. How can we do that? – Cardboard

@Lijo: It's hard to say without knowing more about the real requirements, to be honest. You might want to put delegates in the dictionary... Or you might not :( – Biltong

@JonSkeet I have posted an answer dynamic keyword. Do you think that is a better approach (at least in some scenario)? – Cardboard

@Lijo: It feels like overkill to me. It doesn't scream "simplicity" - but we don't really enough about your real scenario. If it works for you and you prefer it to the dictionary approach then that's fine, but I'd personally steer clear unless there was a really compelling reason to use it. – Biltong

Answer 4

This is a strongly typed, imperative language so if statements and type checking are going to happen.

Having said that, have you considered a virtual method on Role that can be overridden to provide a destination string?

A further alternative, a lookup table!

Dictionary<Type, string> paths = new Dictionary<TYpe, string>()
{
    { typeof(Manager),  @"\ManagerHomeA" }
    { typeof(Accountant),  @"\AccountantHomeC" }
    { typeof(Cleaner),  "Cleaner" }
}

string path = @"\Home";
if(paths.ContainsKey(x.GetType())
    path = paths[x];

Kamilahkamillah

was typing up the same thing, this seems more OO + 1 – Durkee

your edit/addition made your answer worse imo - why did you not go with what you suggested? – Durkee

@Durkee turns out Mr Skeet was putting in the same answer. Why did i add it? Destination may be a more specialised concept than Role needs to know about. Therefore, separate it. – Kamilahkamillah

@Durkee you can't say this is worse, you don't know the program design and the purpose of this code. If the destinations have anything to do with routing as in a UI framework, you might not want to solve this in the Role or derived classes themselves, because they look like business objects which should be UI-agnostic. If you want it "more OO", stick this code in a RouteEvaluator class or the likes. – Bosun

@Gusdor: I was just adding the bit about it being something that Role might not want to know about into my answer, too :) – Biltong

Answer 5

Role should have a virtual function that would return destination:

public virtual string GetDestination()
{
     return "Home";
}

And all the classes should override this function and return the correct string. Then in the code you would have:

var role = new Accountant();
string destination = role.GetDestination();

I hope that helps. There may be typos, I am writing from head.

Merozoite

Oh yes, I didn't see that he also wanted to return something if the role is unknown. Abstract is not a good choice then. I would go for virtual and indeed - property would be nicer. – Merozoite

Answer 6

One way to do it would be to use a map instead of an if:

//(psuedocode)
private Dictionary<Type, string> RoleMap;

void SomeInitializationCodeThatRunsOnce()
{
  RoleMap.Add(typeof(Manager), @"\ManagerHome");
  RollMap.Add(typeof(Accountant), @"\AccountantHome");
  // ect...
}

string GetDestination(Role x)
{
  string destination;
  if(!RoleMap.TryGet(x.GetType(), out destination))
    destination = @"\Home";
  return destination;
}

Further reading: http://www.hanselman.com/blog/BackToBasicsMovingBeyondForIfAndSwitch.aspx

Sulphide

Answer 7

you can either use an interface definition or an abstract method / property

with interface:

public interface IDestinationProvider
{
    sting Destination { get; }
}

string GetDestination(Role role)
{
    var provider = role as IDestinationProvider;
    if (provider != null)
        return provider.Destination;
    return "Default";
}

with an abstract base class

abstract class Role 
{ 
    public abstract string GetDestination();
}

class Manager : Role
{
    public virtual string GetDestination() { return "ManagerHomeA"; }
}

string GetDestination(Role role)
{
    return @"\" + role.GetDestination();
}

or with attributes:

[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
public class DestinationAttribute : Attribute
{
    public DestinationAttribute() { this.Path = @"\Home"; }
    public string Path { get; set; }
}

[Destination(Path = @"\ManagerHome")]
public class Manager : Role { }

string GetDestination(Role role)
{
    var destination = role.GetType().GetCustomAttributes(typeof(DestinationAttribute), true).FirstOrDefault();
    if (destination != null)
        return destination.Path;

    return @"\Home";
}

Transponder

There seems to be a default destination, so method should be virtual not abstract. – Allier

You are right, didn't notice that. Another way would be to stick with abstract and have somethine like return Role.DefaultDestination for every implementation. – Karachi