30

In the comments of this answer it is stated that "checking whether the object has implemented the interface , rampant as it may be, is a bad thing"

Below is what I believe is an example of this practice:

public interface IFoo
{
   void Bar();
}

public void DoSomething(IEnumerable<object> things)
{
   foreach(var o in things)
   {
       if(o is IFoo)
           ((IFoo)o).Bar();
   }
}

With my curiosity piqued as someone who has used variations of this pattern before, I searched for a good example or explanation of why it is a bad thing and was unable to find one.

While it is very possible that I misunderstood the comment, can someone provide me with an example or link to better explain the comment?

Community
  • 1
  • 1
Gene C
  • 2,010
  • 26
  • 34
  • 7
    Well, start by asking yourself if you can't just use an `IEnumerable` instead. Chances are there's no reason. –  Oct 11 '11 at 18:59
  • @delnan - How, pray tell, can you use an IEnumerable if you don't know if the object implements IFoo? The whole point is that you have a collection of different kinds of objects, with no common base class other than object in common. – Erik Funkenbusch Oct 12 '11 at 01:27
  • 2
    poor design could be a reason for the lack of common base/interface, it depends though, sometimes object genuinely is the best option – jk. Oct 12 '11 at 08:20

8 Answers8

36

It depends on what you're trying to do. Sometimes it can be appropriate - examples could include:

  • LINQ to Objects, where it's used to optimize operations like Count which can be performed more efficiently on an IList<T> via the specialized members.
  • LINQ to XML, where it's used to provide a really friendly API which accepts a wide range of types, iterating over values where appropriate
  • If you wanted to find all the controls of a certain type under a particular control in Windows Forms, you would want to check whether each control was a container to determine whether or not to recurse.

In other cases it's less appropriate and you should consider whether you can change the parameter type instead. It's definitely a "smell" - normally you shouldn't concern yourself with the implementation details of whatever has been handed to you; you should just use the API provided by the declared parameter type. This is also known as a violation of the Liskov Substitution Principle.

Whatever the dogmatic developers around may say, there are times when you simply do want to check an object's execution time type. It's hard to override object.Equals(object) correctly without using is/as/GetType, for example :) It's not always a bad thing, but it should always make you consider whether there's a better approach. Use sparingly, only where it's genuinely the most appropriate design.

I would personally rather write the code you've shown like this, mind you:

public void DoSomething(IEnumerable<object> things)
{
    foreach(var foo in things.OfType<IFoo>())
    {
        foo.Bar();
    }
}

It accomplishes the same thing, but in a neater way :)

Community
  • 1
  • 1
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • -1 for not answering the question (as opposed to Erno, who does answer the asked question). – Sverre Rabbelier Oct 17 '11 at 18:34
  • @SverreRabbelier: Presumably my explanation of my view that it's not *always* inappropriate helped the OP, hence why they accepted the answer. – Jon Skeet Oct 17 '11 at 18:40
  • Ah, hadn't noticed that (I just saw that Erno had less votes). Apparently my vote is 'locked in' until the question is edited though. – Sverre Rabbelier Oct 17 '11 at 19:31
  • @SverreRabbelier: Have added a bit more information about why it's a bit of a smell at the same time - see if you think that makes it a better answer too :) – Jon Skeet Oct 17 '11 at 19:34
18

I would expect the method to look like this, it seems much safer:

public void DoSomething(IEnumerable<IFoo> things)
{
   foreach(var o in things)
   {
           o.Bar();
   }
}

To read about the referred violation of the Liskov Principle: What is the Liskov Substitution Principle?

Community
  • 1
  • 1
Emond
  • 50,210
  • 11
  • 84
  • 115
  • Another advantage is that this also moves the type-checking from runtime to compile-time, so you might catch errors sooner. – Mechanical snail Oct 12 '11 at 00:37
  • 1
    That only works if you know the object is an IEnumerable, it might also be an IEnumerable or an group of disparate objects. – Erik Funkenbusch Oct 12 '11 at 01:29
  • @MystereMan - yes and in those cases you might go for the solution Jon suggested or make separate DoSomethings – Emond Oct 12 '11 at 04:33
  • The thing is that you might be assuming too much making the code brittle (how do you write good tests for this method - you'll need to paas any kind of type that fits in an object... – Emond Oct 12 '11 at 08:03
10

If you want to know why the commenter made that comment, probably best to ask them to explain.

I would not consider the code you posted to be "bad". A more "genuinely" bad practice is to use interfaces as markers. That is, you're not planning on actually using a method of the interface; rather, you have declared the interface on a class as a way of describing it in some way. Use attributes, not interfaces, as markers on classes.

Marker interfaces are hazardous in a number of ways. A real-world situation I once ran into where an important product made a bad decision on the basis of a marker interface is here: http://blogs.msdn.com/b/ericlippert/archive/2004/04/05/108086.aspx

That said, the C# compiler itself uses a "marker interface" in one situation. Mads tells the story here: http://blogs.msdn.com/b/madst/archive/2006/10/10/what-is-a-collection_3f00_.aspx

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Would using empty interfaces to implement mixin extensions also be considered bad? There really isn't a way to do that with attributes. – asawyer Oct 11 '11 at 19:58
  • FxCop says this marker interfaces are okay if you need compile time verification. What is the advantage if you control the interface and don't plan on updating it? – Michael B Oct 11 '11 at 20:56
6

A reason is that there will be a dependency on that interface that is not immediately visible without digging in the code.

koen
  • 13,349
  • 10
  • 46
  • 51
4

The statement

checking whether the object has implemented the interface , rampant as it may be, is a bad thing

Is overly dogmatic in my opinion. As other people have answered, you may well be able to pass a collection of IFoo to your method and achieve the same result.

However, interfaces can be useful to add optional features to classes. For example the .net framework provides the IDataErrorInfo interface*. When this is implemented it indicates to a consumer that in addition to the class' standard functionality, it can also provide error information.

In this case, the error information is optional. A WPF view model may or may not provide error information. Without querying for interfaces, this optional functionality would not be possible without base classes with huge surface area.

*We'll ignore for the moment the terrible design of the IDataErrorInfo interface.

Grokys
  • 16,228
  • 14
  • 69
  • 101
1

If your method requires that you inject an instance of an interface, you should treat it the same regardless of the implementation.
In your example you generally wouldn't have a generic list of object, but a list of ISomething's and calling an ISomething.Bar() would be implemented by the concrete type, therefore calling it's implementaiton. If that implementation is to do nothing, then you don't have to do a check.

Christian Specht
  • 35,843
  • 15
  • 128
  • 182
indigo0086
  • 431
  • 6
  • 13
1

I dislike this whole "switch on type" style of coding for a couple of reasons. (Examples drawn in relation to my industry, game development. Apologies in advance. :) )

First and foremost, I think it's sloppy to have a heterogeneous collection of items. E.g. I could have a collection of "everything everywhere," but then when iterating the collection to apply bullet effects or fire damage or enemy AI, I have to walk this list which is mostly stuff I don't care about. It's much "cleaner" IMHO to have separate collections of bullets, raging fires, and enemies. Note that there's no reason why I can't have a single item in multiple collections; a single burning robotic missile could be referenced in all three of those lists to do parts of its "update" as appropriate for the three types of logic it needs to run. Outside of having "one single collection that references everything," I think a collection containing everything everywhere is not terribly useful; you can't do anything with anything in the list unless you query it for what it can do.

I hate doing unnecessary work. This really ties into the above, but when you create a given thing you know what its capabilities are (or can query them at that point), so you might as well take the opportunity at that time to put them in the right more specific collections. You have 16ms to process everything in the world, do you want to waste your time dealing with, querying, and selecting from generic things, or do you want to get down to business and operate only on the specific things you care about?

In my experience, transforming a codebase from generic operation on heterogeneous datasets to one that has homogeneous datasets has resulted in not only performance increases but also comprehension increases that come from simpler code doing more obvious work and in general a reduction in the amount of code required to do any given task.

So yeah, it's dogmatic to say that querying interfaces is bad, but it does seem to make things simpler if you can figure out how to avoid needing to query anything. As for my "performance" statements and the counter that "if you don't measure it, you can't say anything about it," it should be obvious that not doing something is faster than doing it. Whether or not this is important to an individual project, programmer, or function is up to the person with the editor, but if I can simplify code and while doing so make it do less work for the same results, I'm going to do it without bothering to measure.

dash-tom-bang
  • 17,383
  • 5
  • 46
  • 62
0

I don’t see this as a “bad thing” at all, at least not in itself. The code is merely a literal transcription of “x all of the y in z”, and in a situation where you need to do that, it’s perfectly acceptable. You can of course use things.OfType<Foo>() for the sake of concision.

The main reason to recommend against it is that, according to OOP theology, interfaces are intended to model the different kinds of “black box” for which an object may substituted. Predicating an algorithm on fulfillment of an interface constitutes moving behaviour to the algorithm that should be in that interface.

Essentially, an interface is a behavioural role. If you think OOP is a good idea, then you should use interfaces only to model behaviours, so that algorithms don’t have to. I don’t think what passes for OOP these days is in fact a good idea, so this is as far as my answer can be useful.

Jon Purdy
  • 53,300
  • 8
  • 96
  • 166