5

I have read that doing a select case is often a code smell. There are cases however where an interface cannot solve my problem.

For instance, I have a set of date filter objects (last 7 days, last year, ect.) that implement a IDateFilter interface. In another part of the code, I need to determine the type of date filter so that the chart properties can be set. I may want to use a different chart tick interval for instance depending on the date filter.

The problem is I do not want to store this information in the date filter object as it is interface related and I do not want to couple the two.

How could a select case be avoided in this instance? Other than using select case on gettype or creating some enumeration for all the types and then exposing the type and doing a select case on that, I do not see a clean way to do this?

The decorator pattern comes to mind but even then I would still need a select case.

Any ideas?

if (type = last 7 days)
 set this property 
else if (type = last year)
 set this property to something different

I'm using asp net c# in case that makes a difference.

5 Answers 5

3

Create a data structure, such as a dictionary, which uses the type as the key and the value is the chart property. If you have multiple chart properties associated with a given type, then the value could be a ChartProperties object. So you have something like this:

ChartProperties chartProp;
if (propertiesMap.containsKey(currentFilter.getType()){
  chartProp = propertiesMap(currentFilter.getType());
}

I would make the properties map a subclass of a dictionary and then put the above code in a method on that class.

2
  • This is a neat approach (and I've used it before), but it doesn't account for subclasses of types stored in propertiesMap. Plus you deal with the overhead of getType() (depending on the language), and the hashing overhead of the Dictionary. It certainly isn't a bad approach, of course, it just needs to be used with care :-) Commented Apr 23, 2012 at 21:06
  • Thanks, I do use this approach for mapping text keys that come from the client with commands and date filters. I will give this some thought. :) Commented Apr 24, 2012 at 13:47
3

I would make the IDateFilter interface include a method/property for querying the timespan or something. You should be able to ask the object when the start and end dates are, or the number of days back, etc.

8
  • Not sure how that solves my problem. I still end up with a select case statement. The start and end dates are already exposed. It is really the type that is important since it can have the same amount of days as another type but be handled differently. I could expose an enumeration that would expose the type without using gettype() but I still have a select case. Ultimately there is a specific handling or value for each type that needs to be determined (so it can be used), but this handling or value can't be put in (i.e.: doesn't belong in) the object itself. Commented Apr 23, 2012 at 13:57
  • I agree. You may not like it, but it's the proper way to do this. Simply return an enumerator with a weighted integer value and apply logic using the "minimum" or "maximum" preferred timespan.
    – Neil
    Commented Apr 23, 2012 at 13:57
  • @neil. Nothing to do with the time span. The number of days doesn't matter. Commented Apr 23, 2012 at 14:00
  • @KingOfHypocrites, if the number of days is not what's relevant, then I think I need to ask why is this information relevant, and why can't you move this logic to the class itself?
    – Neil
    Commented Apr 23, 2012 at 14:03
  • 1
    @KingOfHypocrites: You're completely missing the point of select as a code smell. Nobody has ever said that it should be avoided completely, only that it should be isolated to its own method and kept to one line per case. This is generally done using a factory method which returns a strategy, but if your situation isn't complex enough to justify that then don't.
    – pdr
    Commented Apr 23, 2012 at 15:38
1

First of all - why aren't you computing the tick interval from the actual data returned by the filter? But okay, I suppose you have your reasons.

Select case statements are a code smell because you might end up having more than one if these statements selecting on the same type. If you add another case branch to one of them, you must add one to all of these statements. This is more error-prone than the alternative - adding an abstract method to an interface. In the latter case the compiler will help you find all the relevant sites in your code.

If there is no chance to integrate the computation in the IDateFilter by a method like getIntervalSize, you could split the client of the IDateFilter interface into a class hierarchy. Whether this is a viable solution depends on the client its creator and the creator of the concrete filter. However, I should warn you that you might run into a different code smell, the duplicated class hierarchy. But this might be either preferable or (more likely) an indication of a bigger design flaw.

1
  • Thanks, I will give this some thought. The main reason I didn't wan to put the tick interval in the date filter is because the interval will be different for different devices/interfaces. While it is business logic in a sense, it is specific to a UI and not a general business rule. I guess it could go either way. Commented Apr 24, 2012 at 13:45
1

After giving it some further thought, if you want to do this in the cleanest way possible, you could use a combination of the Strategy and Service Locator pattern to accomplish your goal in an object-oriented manner.

Here is the overall strategy (no pun intended):

  1. Put your implementation for each strategy in a separate class.
  2. Register each strategy in a static list.
  3. When you need to handle an object of an unknown type, iterate through the registered strategies until you find one that works.

The neat part about this approach is that you can make the condition of whether or not to follow a specific strategy based on criteria more custom than just Type. Let's say you have one interface that you actually want to handle in two different ways depending on the value of a property, or if you want to apply the same strategy to multiple interfaces. It's all about how you implement it.

This is definitely overkill for classes like factories, etc., where the if/else if pattern should suffice and is in fact quite appropriate. Assuming you've explored other options that are simpler or more oo-friendly, here's a thoroughly over-engineered example with the pure purpose of avoiding a switch on Type, resulting in Ultimately Extensible Code (TM).

Let's say you are dealing with two external, non-related interfaces, IBlinger and ISnapper:

interface IBlinger
{
    public string BlingName { get; }
}

interface ISnapper
{
    public string SnapName { get; }
}

And you are trying to write a class that handles writing each interface to the console in a custom way. You would first define an interface IWriteHandler as follows:

interface IWriteHandler
{
    bool Write(object target);
}

And then provide implementations:

class BlingWriter : IWriteHandler
{
    public bool Write(object target)
    {
        var bling = target as IBlinger;
        if (bling == null)
            return false;

        Console.WriteLine("BLING: " + bling.BlingName);

        return true;
    }
}

class SnapWriter : IWriteHandler
{
    public bool Write(object target)
    {
        var snap = target as ISnapper;
        if (snap == null)
            return false;

        Console.WriteLine("SNAP: " + snap.SnapName);

        return true;
    }
}

And here's an example usage:

class Writer
{
    static List<IWriteHandler> _handlers = new List<IWriteHandler>();

    static Writer()
    {
        //add them to the list in order of preference, in case the types overlap
        //due to inheritance.
        _handlers.Add(new BlingWriter());
        _handlers.Add(new SnapWriter());
    }

    public void Write(object unknown)
    {
        foreach (var handler in _handlers)
        {
            if (handler.Write(unknown))
                return;
        }

        //default
        Console.WriteLine("UNKNOWN: " + unknown.ToString());
    }
}

And if that seems too "boilerplatey", you could make it more generic:

abstract class GenericWriteHandler<T> : IWriteHandler where T : class
{
    public bool Write(object target)
    {
        var asT = target as T;
        if (asT == null)
            return false;

        WriteInternal(asT);

        return true;
    }

    protected abstract void WriteInternal(T target);
}

Which could be implemented as:

class SnapWriter : GenericWriteHandler<ISnapper>
{
    protected override void WriteInternal(ISnapper target)
    {
        Console.WriteLine("SNAP: " + target.SnapName);
    }
}

OR, if you like lambdas/don't like creating lots of classes, you could make:

class ActionWriteHandler<T> : IWriteHandler where T : class
{
    Action<T> _a;

    public ActionWriteHandler(Action<T> action)
    {
        _a = action;
    }

    public bool Write(object target)
    {
        var asT = target as T;
        if (asT == null)
            return false;

        _a(asT);

        return true;
    }

    protected abstract void WriteInternal(T target);
}

Where the Writer class is initialized like:

static Writer()
{
    _handlers.Add(new ActionWriteHandler<IBlinger>(b => Console.WriteLine(b.BlingName)));
    _handlers.Add(new ActionWriteHandler<ISnapper>(s => Console.WriteLine(s.SnapName)));
}

In fact, you could make the entire Writer class generic too, but that is for another day...

Related question (which happens to have a similar answer): https://stackoverflow.com/questions/3834091/strategy-pattern-with-no-switch-statements

0
1

The reason that select statements are consider a code smell is aresult of the Zero, One, Infinity rule aka there is no three.

Basically switch statements do two things which are "bad" -- first and foremost, they are an explicit list in your code, with the potential problems associated with that (maintaining sync, partial use, duplicated list for different properties of the same thing [i.e sometimes you do case x: b=y; and sometimes you do case x: c=z;]).

Secondly, if you add a new value or take it away, your code needs to be modified, and then deployed.

Both can be summed up with: code is a poor data structure.

OTOH, sometimes the worst is good enough (sort an array which always contains exactly two elements). So, as with all code smells it is a possible problem, and you need to examine the actual usage to see whether it is a problem. The things to look for are size, varability, frequency of usage, and variations.

Your Answer

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

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