Is referencing an implementing base type in an interface a code smell?

Question

I'm faced with a design decision that doesn't smell to me, but gives me pause. Take a look at the following code sample:

public interface IGenerator
{
  ///<summary>
  /// Combines two generators; performs magic as well
  /// </summary>
  BaseGenerator Combine(BaseGenerator next);

  ///<summary>
  /// Does what a generator does.
  /// </summary>
  object GenerateLolKThx();
}

public abstract class BaseGenerator : IGenerator
{  
  ///<summary>
  /// Combines two generators; performs magic as well
  /// </summary>
  public BaseGenerator Combine(BaseGenerator next)
  {
     // do stuff that I really don't want implementors to try and do
     // because its complex and can result in bad juju if done wrong
     return SuperSecretCombine(this, next);
  }

  ///<summary>
  /// Does what a generator does.
  /// </summary>
  public abstract object GenerateLolKThx();

  /* other base class methods */
}

I don't want to go into more detail about WHY I don't want to trust implementors with the Combine method; suffice it to say its complex. I do, however, want to do my best to force anybody who wants to implement IGenerator to extend BaseGenerator, as that's the only way to properly combine two generators. This is enforced by the interface itself.

I'm worried that there are unexpected issues (indicated by a "smell") caused by my referencing an implementation of an interface within that interface. But I also know that this sort of thing isn't unheard of in CS, nor is it in and of itself bad (i.e., an XML schema that describes XSDs and language compilers that are written in the language they compile).

I'm interested in reasons WHY this might be a code smell, possible pitfalls caused by this type of design, and alternative designs that accomplish what I desire (ensure my implementation of Combine is used to combine any and all types of generators). TIA.

Answer 1

Could that be done with an extension method instead? Then you can just use the interface. Extension methods are a good way of adding common implementation logic to an interface.

This works best of you primarily talk about IGenerator (not concrete types) - since extension resolution prefers types - i.e. FredsGenerator could declare a bespoke Combine method, which would (if you are typing variables as FredsGenerator) take preference. But this is no different to your example, since FredsGenerator could re-implement the interface, stealing the implementation:

using System;
interface IFoo {
    void Bar();
}
abstract class FooBase {
    public void Bar() { Console.WriteLine("Non-virtual; you can't override me!!!"); }
}
class FooImpl : FooBase, IFoo {
    new public void Bar() { Console.WriteLine("mwahahahahah"); }
}
static class Program {
    static void Main() {
        IFoo foo = new FooImpl();
        foo.Bar();
    }
}

At least with the extension method approach your code can't be fooled into running the re-implemented version; your code only knows about IGenerator, so only the Generator.Combine version will be used. Ever.

---

Example (edit reworked from your example):

using System;
public interface IGenerator
{
    // note: no Combine
    object GenerateLolKThx();
}

public static class Generator
{
    ///<summary>
    /// Combines two generators; performs magic as well
    /// </summary>
    public static IGenerator Combine(this IGenerator current, IGenerator next)
    {
        // do stuff that I really don't want implementors to try and do
        // because its complex and can result in bad juju if done wrong
        Console.WriteLine("Super secret logic here...");
        // and prove we have access the the objects...
        Console.WriteLine(current.GenerateLolKThx());
        Console.WriteLine(next.GenerateLolKThx());
        return next; // just for illustration
    }
}
class MyGenerator : IGenerator
{
    private readonly object state;
    public MyGenerator(object state) {this.state = state;}
    public object GenerateLolKThx() { return state; }
}
public static class Program
{
    static void Main()
    {
        IGenerator foo = new MyGenerator("Hello"),
             bar = new MyGenerator("world");

        IGenerator combined = foo.Combine(bar);
    }
}

Answer 2

Why not reference the interface?

public interface IGenerator
{
  ///<summary>
  /// Combines two generators; performs magic as well
  /// </summary>
  IGenerator Combine(IGenerator next);

  ///<summary>
  /// Does what a generator does.
  /// </summary>
  object GenerateLolKThx();
}

Answer 3

I'm interested in reasons WHY this might be a code smell

Here's my answer: one of the purposes (perhaps even the main one) of separating interface from implementation is to make it possible to create many different implementations for an interface. Binding an interface to a particular implementing class breaks this.

You are effectively saying - "this interface must be implemented by a BaseGenerator or its subclass", but then why separate IGenerator from BaseGenerator?

Answer 4

Is BaseGenerator (and its subclasses) the only class which implements the IGenerator interface?

• If so, why have IGenerator at all (why not just have BaseGenerator)?
• If not, do you expect (or not) that other non-BaseGenerator classes which implement IGenerator will be able to define a sensible implementation of the Combine method?

[This looks like a question not an answer, but it's meant to be Socratic.]

Answer 5

It sounds like this whole thing could be implemented better if multiple inheritance were available. If I'm right, there's no code smell here. If I'm wrong, there might well be an odd code smell as that interface is tightly coupled to one of its implementations.

Answer 6

Based on the limited code you show, it looks to me that you are using an interface just for the sake of it where an abstract base class is more appropriate. If you don't trust anyone else to merge BaseGenerators, then that should be the base of your hierarchy and Combine should not be virtual so no-one can override it.