1

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.

Improve this question
1
  • 2
    If you are going to do something different based on each type, then I would consider putting that functionality in the derrived class implementations themselves. Commented Nov 30, 2012 at 14:09

7 Answers 7

Reset to default
4

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

2 and 3 are effectively the same from a design point of view; its two slight variations on the same general approach of checking the type. As for the importance of the difference; it's one type check. If that's not a microoptimization I don't know what is. It is trivially easy for a computer to do such a type check, especially considering we know the object's state is already in the cache. As for proper design, proper design is "none of the above". He should be using polymorphism and defining the differences in behavior within each derived class and calling it through a method in the base.
When you say, "And I quote" you should follow it with an exact quote, rather than something the person didn't actually say. His actual words are: "I have the following example code. Waht is the best practice?" He asked what is the best practice, and the best practice is to not use any of the three options list; it's to use polymorphism.
And yet his question didn't constrain you to those choices. It's clearly implied, I'll grant you that, but it's clear that those were simply the OP's attempts at solving the problem, and it's most helpful to them to provide the solution that best solves that problem, regardless of whether or not it's a solution he's tried. If you wanted to include which of the three is best as well as what you consider the best in general, that'd be fine, but as it is you've already said you know what's best, and it's not what's in your answer. That's not helpful to the OP in the end.
3

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.

Improve this answer

Comments

1

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.

Improve this answer

Comments

0

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.

Improve this answer

Comments

0

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

Improve this answer

4 Comments

Can you elaborate more about why 2nd option save a cast? Perhaps I'm wrong but is is a type operator so where's the cast?
Nevermind. I got this other SO question: stackoverflow.com/questions/686412/…
If using as vs is is making a significant performance difference to your application you have major problems already. Doing a type check is not a slow operation; it's in fact very fast.
@Servy I know that. The point is that option 2 and 3 are equivalent - the only distinction is that option 3 is infintessimaly slower, so we can eliminate it. This cuts his options by 33%, simplifying his question - really, he has asked a question about 2 approaches, not 3.
0

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

Improve this answer

1 Comment

Equals can be used to compare one Category instance to another. But he only has one Category object, customer.Category. Also implementing Equals at a base level is a bad idea as can easily break the Equals contract, rules such as i.e. if A=B=C then A=C. A=B then B=A etc.
0

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");
    }
}
Improve this answer

15 Comments

It's the best practice if you're using something like active record. What about POCO?
Method overloading like in Visitor pattern.
what does visitor pattern to do with POCO? I mean that if you want to leave your entities "as is" without garbage (properties only!), this wouldn't be a "good practice".
@MatíasFidemraizer Actually the opposite is true. It's not "garbage". The entire design of classes in OOP is to combine methods with the data they act on into a single entity. In more procedural programming they were divided; you had data holders and separately you had methods. OOP came about because of the problems with that type of design. The Visitor pattern is a mess; it's a very high maintenance pattern to use, and should only be used when you have few options. Using polymorphism here, rather than reversing it, is much better design.
@MatíasFidemraizer Yet those domain object don't actually have methods for interacting with the database, such operations are performed on them, not by them. In either case, the edit of this post is pretty bad; if you have DoSomething overloads each accepting a different type it effectively forces you to switch on the type to figure out which to call. That's poor design. If you find it imperative to remove code from the actual data holder then you should use the adapter pattern; provide a wrapper type for each domain object which uses inheritance/polymorphism to solve this problem.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.