What is the name of this bad practice / anti-pattern?

I'm trying to explain to my team why this is bad practice, and am looking for an anti-patt开发者_开发技巧ern 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?

Normally a better approach (IMHO) would be to use interfaces instead of inheritance

then it is just a matter of checking whether the object has implemented the interface or not.

I think the anti-pattern name is hard-coding :)

Whether there should be a ThingBase.supportsX depends at least somewhat on what X is. In rare cases that knowledge might be in ControlStuff() only.

More usually though, X might be one of set of things in which case ThingBase might need to expose its capabilities using ThingBase.supports(ThingBaseProperty) or some such.

IMO the fundamental design principle at play here is encapsulation. In your proposed solution you have encapsulated the logic inside of the Thing class, where as in the original code the logic leaks out into the callers.

It also violates the Open-Closed principle, since if you want to add new subclasses that support X you now need to go and modify anywhere that contains that hard-coded list. With your solution you just add the new class, override the method and you're done.

Don't know about a name (doubt such exists) but think of each "Thing" as a car - some cars have Cruise Control system and others do not have.

Now you have fleet of cars you manage and want to know which have cruise control.

Using the first approach is like finding list of all car models which have cruise control, then go car by car and search for each in that list - if there it means the car has cruise control, otherwise it doesn't have. Cumbersome, right?

Using the second approach means that each car that has cruise control come with a sticker saying "I has cruise control" and you just have to look for that sticker, without relying on external source to bring you information.

Not very technical explanation, but simple and to the point.

There is a perfectly reasonable situation where this coding practice makes sense. It might not be an issue of which things actually support X (where of course an interface on each thing would be better), but rather which things that support X are ones that you want to enable. The label for what you see is then simply configuration, presently hard-coded, and the improvement on this is to move it eventually to a configuration file or otherwise. Before you persuade your team to change it I would check this is not the intention of the code you have paraphrased.

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.

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.

I don't think it has a name but maybe check the master list at http://en.wikipedia.org/wiki/Anti-pattern knows? http://en.wikipedia.org/wiki/Hard_code probably looks the closer.

I think that your example probably doesn't have a name - whereas your proposed solution does it is called Composite.

http://www.dofactory.com/Patterns/PatternComposite.aspx

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));
    }
}

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.

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

I don't know if there is a 'pattern' for writing code that is not maintainable or reusable. Why can't you just give them the reason?

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.

Instead of using interfaces, you could use attributes. They would probably describe that the object should be 'tagged' as this sort of object, even if tagging it as such doesn't introduce any additional functionality. I.e. an object being described as 'Thing A' doesn't mean that all 'Thing A's have a specific interface, it's just important that they are a 'Thing A'. That seems like the job of attributes more than interfaces.