C# If...Else compare String value vs Type

Question

I have the following example code. Waht is the best practice? Compare Values or compare Types to perform a certain business logic:

public Customer
{
    public Category {get;set;}
    public CategoryName {get;set;}  //e.g. "Category A" or "Category B"
}

public Category{}

public CategoryA : Category {}

public CategoryB : Category {}

public void Main()
{
    Customer customer = new Customer();

// Option 1:
if(customer.CategoryName == "Category A")
{
    CategoryA catA= customer.Category as CategoryA;     
    DoSomething(catA)
}

// Option 2:
CategoryA catA= customer.Category as CategoryA;
if(catA != null)
{
    DoSomething(catA)
}

// Option 3:
if(customer.Catgeory is Category A)
{
    CatgeoryA catA= customer.Category as CategoryA;
    DoSomething(catA)
}
}

The Code is just for illustration.

Answer 1

Given those 3 options, I'd go with Option 2.
e.g. - Try to make a conversion and check if it isn't Null.

The reason it is better then the last option is that Option 3 causes you to make 2 conversions, one for the is and one for the as in the next line.

Finally, Option 1 is the worst IMO - it requires you to have some kind of logic that you can't really be sure will stick later on (No one is preventing someone from creating a customer with a Category of CategoryA and a CategoryName of "Category B"). Also, this is additional logic which can be done in a much clearer way via Option 2.

I should mention, as pointed out in the other comments/answers, there are a few more options that can be taken into account. Using Polymorphism is probably the best design strategy.

Answer 2

Edit 3:

I think that in this question author did not mean that objects are POCO's, because if Category is a POCO then it can not be polymorphic. And because of that there is no sense to inherit CategoryA and CategoryB from Category - why we would want to have the same interface without polymorphism?! So conclusion is that author of this question made a design mistake or he should think about my Original answer.

Edit 2:

I just noticed that in Customer we do not know concrete type of Category. So we can not use CategoryService as:

new CategoryService().DoSomething(customer.Category); // compile-time error

So Edit 1 was not valid for this case.

Edit 1:

For POCO you can use service class with method overloading:

public class Customer {
    public Category Category { get; set; }
}

public abstract class Category {
}

public class CategoryA : Category {
}

public class CategoryB : Category {
}

public class CategoryService {
    public void DoSomething(CategoryA c) {
        Console.WriteLine("A");
    }

    public void DoSomething(CategoryB c) {
        Console.WriteLine("B");
    }
}

Original:

Best practice is to use polymorphism. When you need to compare types it is a sign that you did something wrong or it is very special situation.

public class Customer {
    public Category Category { get; set; }
}

public abstract class Category {
    public abstract void DoSomething();
}

public class CategoryA : Category {
    public override void DoSomething()
    {
        Console.WriteLine("A");
    }
}

public class CategoryB : Category {
    public override void DoSomething()
    {
        Console.WriteLine("B");
    }
}

Answer 3

Why not implement an override method in the parent class as such:

public override bool Equals(System.Object obj)
{
    bool equal=true;

    //any kind of business logic here on the object values andor types

    return equal;
}

We do it all the time, works great :)

Answer 4

I think there is something wrong with your design. It is a smelly design IMO to have multiple checks and casts.

CategoryA catA = customer.Category as CategoryA;
if(catA != null)
{
    DoSomething(catA)
}

So presumably you have a DoSomething(CategoryA cat), DoSomething(CategoryB cat) etc.

In this case I would strongly recommend you consider moving the DoSomething to the category class.

customer.Category.DoSomething();

It can then be implemented in different ways by CategoryA, CategoryB and CategoryC.

If there only is an implementation for CategoryA, just have an empty implementation in the base category and don't override in B and C's.

Extra recommendation: Use Interfaces

I would personally not implement basetypes. I would always opt for interfaces. It's less restrictive and easier to follow when the code is not in several layers of class hierarchy.

public interface ICategory{
  void DoSomething();
}

public class CategoryA : ICategory {...}

public class CategoryB : ICategory {...}

Common functionality between the interface implementers? No problem, just make a new object to perform that functionaility and compose it into both of the implementers. Then that functionality can be unit tested alone.

Answer 5

In addition to the other points explained by other answerers about performance or cast once or twice, or whatever, I'd like to make some considerations.

It absolutely depends on what the business would do in your particular case:

a. Category is just a discriminator

Don't use a type. It should be a string, or an identifier like a Guid or int. Your UI localization would translate the identifier into the human-readable name. This is enough to discriminate by category.

Solution: Customer would have a CategoryName property. Compare by Id.

b. Category is a discriminator and there's a limited number of possible categories

One word: enumerations. Define a Category enumeration:

public enum Category { A, B, C }

if(customer.Category == Category.A)
{
    // Whatever
}

Solution: Customer would have a Category property. Compare by enumeration value.

c. Category is an entity

I wouldn't create a class for each category. Category is the entity and there're zero or more categories having particular identifiers or names.

For that reason, Category has a Name and Id property of string and int or Guid type, respectively.

Solution: Customer would have an 1:1 association with Category and Category would have a Name property. Compare by Id.

Answer 6

Option 2 is always better than Option 3 as it saves a 2nd cast. Therefore, we can eliminate Option 3.

As for Option 1, I have to ask why are you duplicating the type name in a property? This is redundant code (as you already have the type name).

Therefore, Option 2 is the best, given the scenario you have described. However, a little more detail about what you are checking, how likely it is to change, how closely the category name is tied to the class name, etc. would probably influence the decision...

Answer 7

This is a bit subjective depending on your situation. In my opinion, it is always better to compare by type instead of by string for the single fact that you can mis-type a string but the compiler will check if you mistype a type. Between options 2 and 3, there really doesn't matter other than skipping a second cast with option 2.