What is the name of this bad practice / anti-patte

Question

I'm trying to explain to my team why this is bad practice, and am looking for an anti-pattern reference to help in my explanation. This is a very large enterprise app, so here's a simple example to illustrate what was implemented:

public void ControlStuff()
    {
        var listOfThings = LoadThings();
        var listOfThingsThatSupportX = new string[] {"ThingA","ThingB", "ThingC"};
        foreach (var thing in listOfThings)
        {
            if(listOfThingsThatSupportX.Contains(thing.Name))
            {
                DoSomething();
            }
        }
    }

I'm suggesting that we add a property to the 'Things' base class to tell us if it supports X, since the Thing subclass will need to implement the functionality in question. Something like this:

public void ControlStuff()
    {
        var listOfThings = LoadThings();
        foreach (var thing in listOfThings)
        {
            if (thing.SupportsX)
            {
                DoSomething();
            }
        }
    }
class ThingBase
{
    public virtual bool SupportsX { get { return false; } }
}
class ThingA : ThingBase
{
    public override bool SupportsX { get { return true; } }
}
class ThingB : ThingBase
{
}

So, it's pretty obvious why the first approach is bad practice, but what's this called? Also, is there a pattern better suited to this problem than the one I'm suggesting?

Answer 1

I would call it a Failure to Encapsulate. It's a made up term, but it is real and seen quite often

A lot of people forget that encasulation is not just the hiding of data withing an object, it is also the hiding of behavior within that object, or more specifically, the hiding of how the behavior of an object is implemented.

By having an external DoSomething(), which is required for the correct program operation, you create a lot of issues. You cannot reasonably use inheritence in your list of things. If you change the signature of the "thing", in this case the string, the behavior doesn't follow. You need to modify this external class to add it's behaviour (invoking DoSomething() back to the derived thing.

I would offer the "improved" solution, which is to have a list of Thing objects, with a method that implements DoSomething(), which acts as a NOOP for the things that do nothing. This localizes the behavior of the thing within itself, and the maintenance of a special matching list becomes unnecessary.

Answer 2

It is just a bad code, it does not have a name for it (it doesn't even have an OO design). But the argument could be that the first code does not fallow Open Close Principle. What happens when list of supported things change? You have to rewrite the method you're using.

But the same thing happens when you use the second code snippet. Lets say the supporting rule changes, you'd have to go to the each of the methods and rewrite them. I'd suggest you to have an abstract Support Class and pass different support rules when they change.

Answer 3

In order to me the best is to explain that in term of computational complexity. Draw two chart showing the number of operation required in term of count(listOfThingsThatSupportX ) and count(listOfThings ) and compare with the solution you propose.

Answer 4

The Writing Too Much Code Anti-Pattern. It makes it harder to read and understand.

As has been pointed out already it would be better to use an interface.

Basically the programmers are not taking advantage of Object-Oriented Principles and instead doing things using procedural code. Every time we reach for the 'if' statement we should ask ourselves if we shouldn't be using an OO concept instead of writing more procedural code.

Answer 5

If it were one string, I might call it a "magic string". In this case, I would consider "magic string array".

Answer 6

Since you don't show what the code really is for it's hard to give you a robust sulotion. Here is one that doesn't use any if clauses at all.

// invoked to map different kinds of items to different features
public void BootStrap
{
    featureService.Register(typeof(MyItem), new CustomFeature());
}

// your code without any ifs.
public void ControlStuff()
{
    var listOfThings = LoadThings();
    foreach (var thing in listOfThings)
    {
        thing.InvokeFeatures();
    }
}

// your object
interface IItem
{
    public ICollection<IFeature> Features {get;set;}

    public void InvokeFeatues()
    {
        foreach (var feature in Features)
            feature.Invoke(this);
    }
}

// a feature that can be invoked on an item
interface IFeature
{
    void Invoke(IItem container);
}

// the "glue"
public class FeatureService
{

    void Register(Type itemType, IFeature feature)
    {
        _features.Add(itemType, feature);
    }

    void ApplyFeatures<T>(T item) where T : IItem
    {
        item.Features = _features.FindFor(typof(T));
    }
}