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

Question

I am trying to explain to my team why this is bad practice, and I am looking for a link to the anti-pattern to help in my explanations. This is a very large enterprise application, so here is 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 suggest adding a property to the Things base class to let us know if it supports X, since the Thing subclass will have to implement this functionality. Something like that:

 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 is it called? Also, is there a sample more suitable for this problem than the one I offer?

Answer 1

Usually a better approach (IMHO) would be to use interfaces instead of inheriting

then it’s just a matter of checking whether the object implemented the interface or not.

Answer 2

I think the name of the anti-template is hard coding :)

Should ThingBase.supportsX be at least slightly dependent on what X . In rare cases, this knowledge can only be in ControlStuff() .

More commonly, X may be one of many things, in which case ThingBase may need to reveal its capabilities with ThingBase.supports(ThingBaseProperty) or some of these.

Answer 3

IMO, the fundamental design principle here is encapsulation. In the proposed solution, you encapsulated the logic inside the Thing class, where, as in the source code, the logic seeps into the callers.

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

Answer 4

I don’t know about the name (such a doubt exists), but think of each “Things” as a car - some cars have a cruise control system, while others do not.

Now you have a fleet of cars that you drive and want to know what cruise control they have.

Using the first approach is to search for a list of all car models that have cruise control, then drive a car and look for everyone in this list - if this means that the car has cruise control, otherwise it does not. Cumbersome, right?

Using the second approach means that every car that has cruise control has a sticker that says “I have cruise control,” and you just need to look for that sticker without relying on an external source to provide you with information.

Not a very technical explanation, but simple and accurate.

Answer 5

There is a perfectly reasonable situation when this coding practice makes sense. Perhaps not the problem is that things actually support X (where, of course, the interface on each thing will be better), but rather which things that support X are the ones you want to include . The label for what you see is just a configuration that is currently hardcoded , and the improvement in this is to move it eventually to a configuration file or otherwise. Before you convince a team to modify it, I would check that this is not the intention of the code that you rephrased.

Answer 6

Too much coding. This makes reading and comprehension difficult.

As already indicated, it would be better to use an interface.

Basically, programmers do not use object-oriented principles and instead do something using procedural code. Each time we reach the if statement, we must ask ourselves if we should use the concept of OO instead of writing more procedural code.

Answer 7

This is just bad code, it doesn't have a name for it (it doesn't even have an OO design). But the argument may be that the first code does not fall under the principle of open closure . What happens when the list of supported things changes? You must rewrite the method you are using.

But the same thing happens when you use the second piece of code. Assuming the support rule is supported, you have to go to each of the methods and rewrite them. I suggest you have an abstract support class and pass different support rules when they change.

Answer 8

I don’t think he has a name, but maybe check the main list http://en.wikipedia.org/wiki/Anti-pattern knows? http://en.wikipedia.org/wiki/Hard_code probably looks closer.

I think your example probably has no name, while your proposed solution is called Composite .

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

Answer 9

Since you do not show what is really really, it is difficult to give you a reliable sulotion for this. 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)); } } 

Answer 10

I would call it Failure to Encapsulate . This is a coined term, but it is real and quite common

Many people forget that collection is not just hiding data with an object, it is also hiding the behavior inside this object or, more specifically, hiding how the behavior of the object is implemented.

If you have an external DoSomething() necessary for the program to work correctly, you create a lot of problems. You cannot reasonably use inheritance in your list of things. If you change the signature "thing", in this case the string, the behavior will not follow. You need to modify this outer class to add its behavior (by calling DoSomething() back to the derived thing .

I would suggest an “improved” solution that should have a list of thing objects, with a method that implements DoSomething() that acts like NOOP for things that don't do anything. This localizes the behavior of the thing inside itself, and serving a special match list becomes unnecessary.

Answer 11

If it were a single line, I could call it a “magic line”. In this case, I would consider a "magic array of strings."

Answer 12

I do not know if there is a "template" for writing code that is not supported or reused. Why can't you just explain them the reason?

Answer 13

For me, it's best to explain this in terms of computational complexity. Draw two diagrams showing the number of operations required in terms of count(listOfThingsThatSupportX ) and count(listOfThings ) , and compare them with the proposed solution.

Answer 14

Instead of using interfaces, you can use attributes. They will probably describe that the object should be marked as an object of this type, even if marking it as such does not introduce any additional functions. That is, the object described as "Thing A" does not mean that all "Thing A" have a specific interface, it is simply important that they be "Thing A". This looks more like attributes than interfaces.