Is Type Checking a Code Smell in this code?

Question

I have an interface "IPartyCountService" that counts number of customers and number of suppliers.

The implementation class "PartyCountService" makes use of type checking to check whether the party is a Customer or a Supplier.

My question is: does the implementation class PartyCountService's use of type checking give out code smell? Please ignore code smell is other classes as they are very simplified... my question is only about PartyCountService's methods.

Any feedback, comment, criticism is appreciated.

public interface IPartyCountService
{
    int GetTotalNumberOfCustomers();
    int GetTotalNumberOfSuppliers();
}

internal class PartyCountService:IPartyCountService
{
    IPartyRepository _partyRepository;

    public PartyCountService(IPartyRepository partyRepository)
    {
        _partyRepository = partyRepository;
    }

    public int GetTotalNumberOfCustomers()
    {
        var counter = 0;
        foreach(var customer in _partyRepository.GetAll())
        {
            if (customer is Customer) counter++;
        }
        return counter;
    }

    public int GetTotalNumberOfSuppliers()
    {
        var counter = 0;
        foreach (var customer in _partyRepository.GetAll())
        {
            if (customer is Supplier) counter++;
        }
        return counter;
    }
}

public interface IPartyRepository
{
    IList<IParty> GetAll();
}
internal class PartyRepository:IPartyRepository
{
    public IList<IParty> GetAll()
    {
        // put together all parties, including customers and suppliers
        return allParties;
    }
}
internal class Customer:IParty{}
internal class Supplier:IParty{}
public interface IParty{}

Answer 1

It doesn't feel right to me but a small change you could make is this.

return _partyRepository.GetAll().Count(p => p is Customer);

Answer 2

I would use the .OfType<> extension method.

return _partyRepository.GetAll().OfType<Customer>().Count();

EDIT: As stated by SP below, this makes for some cleaner code, but doesn't necessarily fix the smell.

Answer 3

Perhaps the IParty should have a method to interrogate whatever you are interested in when you are counting "customers"?

Answer 4

It's a little smelly, but not the stinkiest thing I've ever seen (or done myself). A couple of thoughts:

• Any reason to store both suppliers and customers in the same collection? If you find your code always making the distinction, refactor how you store those.
• Storing a collection of interfaces but then casting to a concrete type has an odor to it. Any time I see that sort of cast I immediately think abstraction leak or a static structure that needs a second look.
• LINQ can make this a bit more straightforward if it does indeed make sense to combine them.

like so:

return _partyRepository.GetAll().OfType<Supplier>().Count();

Answer 5

First of all, I think this is a question of choice. You must know the trade-offs involved with each possible alternative:

1. Yours.
2. Create a field on the base class that enables you to know which is the kind of the party.

The alternative (1) has the advantage of being more concise and having a friendlier approach to OO. (However, remember you are breaking polymorphism when you look at the concrete type of the object.)

The alternative (2) has the advantage that, first, you don't rely on metadata to know the object type. Secondly, an is call is more expensive then checking a variable type. So make sure that you can live with the performance hit.

EDIT: Now that I look at it, you are abstracting to an interface type, and then casting it back to it's concrete type. Why is that? If the class that consumes the collection knows about the different typing, it should access directly the types stored there.

Answer 6

I think you should not be hiding the Customer and Supplier from the public API. However, it's advisable to replace them with an ICustomer and ISupplier interfaces. Following interface-based designed on the forefront of your component (i.e. its public API) will help you achieve better design quite naturally.

Regarding the type checking and counting: I don't see explicit dynamic type checks as something bad. However, by using interfaces they'll become semantically much more natural. Also, I don't think LINQ-like statements shall be introduced just everywhere for the sake of saving an explicit foreach and a pair of { }.

Answer 7

Is there a reason why Customers and Suppliers are stored in a single heterogeneous collection in partyRepository? Seems like it would be much easier to simply hang onto a collection of Customers, and a separate collection of Suppliers; then it could expose functions like GetCustomers(), GetSuppliers() or Get<T>().

With this approach, you can still implement GetAll() - it's very easy to "union" 2 collections together. Easier (are probably better-performing) than filtering out a heterogeneous collection.