44

I've been guilty of having a 1-to-1 relationship between my interfaces and concrete classes when using dependency injection. When I need to add a method to an interface, I end up breaking all the classes that implement the interface.

This is a simple example, but let's assume that I need to inject an ILogger into one of my classes.

public interface ILogger
{
    void Info(string message);
}

public class Logger : ILogger
{
    public void Info(string message) { }
}

Having a 1-to-1 relationship like this feels like a code smell. Since I only have a single implementation, are there any potentially issues if I create a class and mark the Info method as virtual to override in my tests instead of having to create an interface just for a single class?

public class Logger
{
    public virtual void Info(string message)
    {
        // Log to file
    }
}

If I needed another implementation, I can override the Info method:

public class SqlLogger : Logger
{
    public override void Info(string message)
    {
        // Log to SQL
    }
}

If each of these classes have specific properties or methods that would create a leaky abstraction, I could extract out a base class:

public class Logger
{
    public virtual void Info(string message)
    {
        throw new NotImplementedException();
    }
}

public class SqlLogger : Logger
{
    public override void Info(string message) { }
}

public class FileLogger : Logger
{
    public override void Info(string message) { }
}

The reason why I didn't mark the base class as abstract is because if I ever wanted to add another method, I wouldn't break existing implementations. For example, if my FileLogger needed a Debug method, I can update the base class Logger without breaking the existing SqlLogger.

public class Logger
{
    public virtual void Info(string message)
    {
        throw new NotImplementedException();
    }

    public virtual void Debug(string message)
    {
        throw new NotImplementedException();
    }
}

public class SqlLogger : Logger
{
    public override void Info(string message) { }
}

public class FileLogger : Logger
{
    public override void Info(string message) { }
    public override void Debug(string message) { }
}

Again, this is a simple example, but when I should I prefer an interface?

nivlam
  • 3,223
  • 4
  • 30
  • 39
  • 1
    _The reason why I didn't mark the base class as abstract is because if I ever wanted to add another method_ Hm, an abstract class can contain implementation. You can add Debug method to your abstract Logger class. – Anton Sizikov Apr 25 '12 at 08:09
  • Breaking existing implementations is only a problem if you are writing a reusable library. Are you? Or are you simply writing an line of business application? – Steven Apr 25 '12 at 11:03
  • That's not the scope of the question, but inheritance is overrated. A `SqlLogger` is just a concrete `Logger` with a `SqlLogPersistenceStrategy`. Composition is much better than inheritance in most cases. Also for your problem, what about the ISP? `ILogInfo`, `ILogError`, etc. – plalx Nov 05 '15 at 16:28

4 Answers4

35

The "Quick" Answer

I would stick with interfaces. They are designed to be contracts for consumption for external entities.

@JakubKonecki mentioned multiple inheritance. I think this is the biggest reason to stick with interfaces as it will become very apparent on the consumer side if you force them to take a base class... no one likes base classes being thrust upon them.

The Updated "Quick" Answer

You have stated issues with interface implementations outside your control. A good approach is to simply create a new interface inheriting from the old one and fix your own implementation. You can then notify the other teams that a new interface is available. Over time, you can deprecate older interfaces.

Don't forget you can use the support of explicit interface implementations to help maintain a nice divide between interfaces that are logically the same, but of different versions.

If you want all this to fit in with DI, then try not to define new interfaces and instead favour additions. Alternatively to limit client code changes, try to inherit new interfaces from old ones.

Implementation vs. Consumption

There is a difference between implementing the interface and consuming it. Adding a method breaks the implementation(s), but does not break the consumer.

Removing a method obviously breaks the consumer, but does not break the implementation - however you wouldn't do this if you are backwards-compatibility conscious for your consumers.

My Experiences

We frequently have a 1-to-1 relationship with interfaces. It is largely a formality but you occasionally get nice instances where interfaces are useful because we stub / mock test implementations, or we actually provide client-specific implementations. The fact that this frequently breaks that one implementation if we happen to change the interface isn't a code smell, in my opinion, it is simply how you work against interfaces.

Our interface-based approach is now standing us in good stead as we utilise techniques such as the factory pattern and elements of DI to improve an aged legacy code base. Testing was able to quickly take advantage of the fact that interfaces existed in the code base for many years before finding a "definitive" use (ie, not just 1-1 mappings with concrete classes).

Base Class Cons

Base classes are for sharing implementation details to common entities, the fact they are able to do something similar with sharing an API publicly is a by-product in my opinion. Interfaces are designed to share API publicly, so use them.

With base classes you could also potentially get leakage of implementation details, for example if you need to make something public for another part of the implementation to use. These are no conducive to maintaining a clean public API.

Breaking / Supporting Implementations

If you go down the interface route you may run into difficulty changing even the interface due to breaking contracts. Also, as you mention, you could break implementations outside of your control. There are two ways to tackle this problem:

  1. State that you won't break consumers, but you won't support implementations.
  2. State that once an interface is published, it is never changed.

I have witnessed the latter, I see it come in two guises:

  1. Completely separate interfaces for any new stuff: MyInterfaceV1, MyInterfaceV2.
  2. Interface inheritance: MyInterfaceV2 : MyInterfaceV1.

I personally wouldn't choose to go down this route, I would choose to not support implementations from breaking changes. But sometimes we don't have this choice.

Some Code

public interface IGetNames
{
    List<string> GetNames();
}

// One option is to redefine the entire interface and use 
// explicit interface implementations in your concrete classes.
public interface IGetMoreNames
{
    List<string> GetNames();
    List<string> GetMoreNames();
}

// Another option is to inherit.
public interface IGetMoreNames : IGetNames
{
    List<string> GetMoreNames();
}

// A final option is to only define new stuff.
public interface IGetMoreNames 
{
    List<string> GetMoreNames();
}
Adam Houldsworth
  • 63,413
  • 11
  • 150
  • 187
  • One of the difficulties I'm having is when this interface is shared across the enterprise. If I update an interface, it breaks the implementations for all the other teams that use this interface. At this point, we're forcing these changes on them. In a perfect world, everyone would be happy and willing to update their implementations, but that's not always the case. – nivlam Apr 25 '12 at 08:14
  • If you can't change the interface - you can create another one: `inteface IDebuger { void Debug (string message);}` and implement it in FileLogger. So if other teams does not need it, they will not use and implement it. – Anton Sizikov Apr 25 '12 at 08:21
  • @nivlam The alternative to this is once an interface is created, it is locked to change. Create new interfaces that inherit the old ones, then implementations can optionally also implement the new interfaces... meaning just your internal implementation. I have updated my answer to suit. – Adam Houldsworth Apr 25 '12 at 08:25
12

Your ILogger interface is breaking the interface segregation principle when you start adding Debug, Error, and Critical methods besides Info. Take a look at the horrible Log4Net ILog interface and you'll know what I'm talking about.

Instead of creating a method per log severity, create a single method that takes a log object:

void Log(LogEntry entry);

This completely solves all of your problems, because:

  1. LogEntry will be a simple DTO and you can add new properties to it, without breaking any client.
  2. You can create a set of extension methods for your ILogger interface that map to that single Log method.

Here's an example of such extension method:

public static class LoggerExtensions
{
    public static void Debug(this ILogger logger, string message)
    {
        logger.Log(new LogEntry(message)
        {
            Severity = LoggingSeverity.Debug,
        });
    }

    public static void Info(this ILogger logger, string message)
    {
        logger.Log(new LogEntry(message)
        {
            Severity = LoggingSeverity.Information,
        });
    }
}

For a more detailed discussion on this design, please read this.

Steven
  • 166,672
  • 24
  • 332
  • 435
  • The problem now is that the contract doesn't define what severity level is or isin't supported. With the ISP in mind, what about `ILogInfo`, `ILogError`, `ILogDebug`, etc ? – plalx Nov 05 '15 at 16:20
  • I find this answer a bit impractical. It would be ridiculous to create a new interface for every single method that a class may implement. Naturally you group related functionality together. I struggle to think of a context in which you would only need to log Info and not also Warning/Error/Debug. So frankly I think this code purity for the sake of purity. – Siddhartha Gandhi Feb 01 '22 at 20:31
  • Hi @SiddharthaGandhi, you might have misinterpreted my answer (which might be my fault of not making myself clear enough). The `LoggerExtensions` class can contain as many extension methods you want, so you should add `Warning` and `Error` overloads ass you see fit. In another SO q/a I gave a more articulate answer that might hopefully be a better read. Take a look [here](https://stackoverflow.com/a/5646876/264697). – Steven Feb 01 '22 at 20:37
4

You should always prefer the interface.

Yes, in some cases you will have the same methods on class and interface, but in more complex scenarios you won't. Also remember, that there is no multiple inheritance in .NET.

You should keep your interfaces in a separate assembly and your classes should be internal.

Another benefit of coding against interfaces is an ability to easily mock them in unit tests.

Jakub Konecki
  • 45,581
  • 7
  • 87
  • 126
  • 1
    Why is it a good thing to keep the interfaces in a separate assembly ? – thedev Apr 25 '12 at 08:03
  • 5
    @thedev You can publish the interfaces without publishing an implementation. These can also be shared without leaking an unwanted implementation that just adds overhead or maintenance issues. – Adam Houldsworth Apr 25 '12 at 08:06
0

I Prefer interfaces. Given stubs and mocks are also implementations (sort of), I always have at least two implementations of any interface. Also, Interfaces can be stubbed and mocked for tests.

Further, the contract angle that Adam Houldsworth mentions is very constructive. IMHO it makes the code cleaner than 1-1 implementations of interfaces make it smelly.

kenchilada
  • 7,469
  • 5
  • 25
  • 34
Morten
  • 3,778
  • 2
  • 25
  • 45