28

I''m working on a C# library where the API provides several public interfaces and a single concrete factory class (itself an interface implementation). This factory provides implementations of the various interfaces. Other than the factory, none of the actual implementations are available to the user.

As is the nature of using interfaces, this decouples my users from being concerned with how I variously decide to refactor the implementations. I know that's good practice in itself, but I do feel I'm maybe being a little restrictive not making the implementations public.

Is this common/accepted practice? What are the pros and cons of this approach?

Bondolin
  • 393
  • 4
  • 10
  • 1
    One thing to note: If your parameter types are also interfaces, it allows your consumers to implement them themselves. Which might be good or bad. – Paŭlo Ebermann Sep 19 '22 at 23:50
  • Tangentially related, I just saw [this video](https://youtu.be/d76WWAD99Yo) a couple days ago that gives a performance-based reason for closed/sealed implementations. – Daevin Sep 20 '22 at 18:12
  • 9
    I see a close vote on this question, but definitely disagree. This is the exactly type of question which SE.SE is built upon and intended for. – Panzercrisis Sep 20 '22 at 18:48
  • 1
    Comment, as it is totally unrelated to accepted answer (and hence can't be an answer to the question): exposing interfaces let *users* of the library to test their own code with unit tests, exposing non-creatable classes requires users of the library to wrap/proxy calls with intermediate interface to be able to test their own code that uses the library... – Alexei Levenkov Sep 20 '22 at 22:06
  • 1
    My rule of thumb is an `interface` is public but an implementation is internal. Then you provide a mechanism to register the implementation with DI (`services.AddMyThing();`). The slight hiccup is that when unit testing, you have to use [InternalsVisibleTo] (or the csproj equivilent), to allow the unit test project to see the implementation class. – Neil Sep 21 '22 at 14:08
  • @Neil sounds like a very similar setup, thanks. I tend to defer to Mark Seemann on not [Unit testing internals](https://blog.ploeh.dk/2015/09/22/unit-testing-internals/), so what I am doing instead is have an "internal" subassembly with public access on all the things to which I'd otherwise have internal access. Then of course there's the question of how to keep those internal types from surfacing to the consumers of my main assembly, for which I follow the advice given here - https://stackoverflow.com/a/72723880/1399272. – Bondolin Sep 22 '22 at 19:20
  • *"but I do feel I'm maybe being a little restrictive"* I don't know how it is in C#, but in Java if you have an open source library and you need a very specific implementation you can create one or more classes in the same package of the library and even override some classes. However the developer in this way understands very well that he is doing something that might be broken in future versions. So using only interfaces rather than restricting makes clear how things stand. – FluidCode Sep 22 '22 at 21:18
  • When not sure this is the only way to do something, provide an interface. When you are absolutely sure this is the only way to do this (for some good years to come), provide a concrete implementation. – S.D. Sep 23 '22 at 05:37
  • I DO unit test internals, because they are the business logic. Handlers and services etc, must be tested. Just because the implementation is not public doesn't mean its not necessary to unit test it, it just means I don't want you to directly instantiate it. – Neil Sep 25 '22 at 09:06
  • @Neil for sure, that all ought to be tested. Seemann's take is to test it indirectly, transitively through the public API; I modify this a bit by splitting out an intermediate public API, i.e. an "internal" project whose assembly is only used by and visible to my code. – Bondolin Sep 26 '22 at 13:14
  • @Bondolin If you are testing business logic or data interaction, by calling an API because that's the 'public interface' then that's not a unit test, that's an integration test. A true unit test will mock it's services, whereas an integration test will test all layers within the process. Personally, I will unit test all areas of an implementation, hitting every combination of code paths extensively, but an integration test is more selective, and I will mainly check the happy path. – Neil Sep 27 '22 at 11:44
  • @Neil yeah, guess you're right, that is more of an integration test. – Bondolin Sep 27 '22 at 12:35

3 Answers3

45

As is the nature of using interfaces, this decouples my users from being concerned with how I variously decide to refactor the implementations.

Let's take a step back and make sure we understand what the value of an interface is. It's not really what you've stated. Take the following example:

interface IFoo 
{
  void baz();
}

Now later you want to refactor this and realize the method shouldn't be named baz but bar instead. Can you do that refactoring without the users of the interfaces being concerned about it? No. How about adding or changing parameter types? No. You can add new methods to the interface without breaking things but that doesn't mean the user is not or should not be concerned. Nothing about this changes by using an interface or not defining one.

What about refactoring the implementation? You can (carefully) change the implementation of a public method on a concrete class without users knowing or having concerns about it. You can also change method's implementation to be incompatible with previous versions. Putting an interface in front of that doesn't really change anything with those two scenarios.

The only point of an interface is to define a contract which is independent of a specific concrete class. In other words, the client should not need to care which implementation it is dealing with. They are expected to be interchangeable with regard to the methods defined on the interface. It shouldn't even matter to the client whether the contract is defined on an interface or on a concrete class. All an interface does is decouple a set of public method signatures from a concrete class definition. It is a formal way to allow for substitution of implementations.

I think the C# Hungarian wart standard of prefixing interfaces with 'I' is misguided (and misguiding.) Interfaces don't do anything. There's no reason developers need to be concerned with whether the type they are working with is an interface. A lot of people might think this is just a preference but it creates a real issue. This standard means that you can't start with a class and then later decided it should be an interface without breaking everything that was using it. Therefore, C# APIs are so packed with interfaces that have a single implementation that I believe it has lead to a lot of developers who 'grow up' in C# not understanding the point of them.

In a nutshell, interfaces only support adding additional concrete implementations of a contract and especially allowing users to do so. And, naming standard aside, they can be used to do so without having to introduce them prematurely as long as you don't allow clients to bind to anything other than public methods. If you follow the 'I' wart standard, you must create an interface for every class you might ever want to later provide more implementations of without breaking user code.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
  • 10
    I is misguided because I don't want to know if what I'm talking to is an interface or not. Without it, so long as you never use `new` on it, you never know exactly what it is. Sadly, while Java has avoided the I silliness they went and made it so that even when the source code doesn't know if you're talking to a concrete type the binary still knows. Sigh. – candied_orange Sep 19 '22 at 19:18
  • 14
    Absolutely agree. I've had to defend this position more than once and objectively, this preemptive creation of interfaces is the most damaging aspect, in my estimation. I look at C# source and I often find that every class has an interface and every interface maps to exactly one class. I don't want to pretend to be Dijkstra but 'cripples the mind' is on the tip of my tongue. – JimmyJames Sep 19 '22 at 20:09
  • @candied_orange On a side note, I actually think that `new` (or any direct constructor call) should only be allowable from the class itself. If you are going down the factory rabbit hole anyway, that shouldn't matter. – JimmyJames Sep 19 '22 at 20:12
  • @JimmyJames thanks for the detailed response. I agree the Hungarian notion is a bit unhelpful, and does incline toward a violation of the Reused Abstractions Principle (see also https://blog.ploeh.dk/2010/12/18/OnRoleInterfaces,theReusedAbstractionsPrincipleandServiceLocators/). The `var` keyword helps some here, but only so far. – Bondolin Sep 19 '22 at 21:00
  • As far as interface use in general goes, agree that the main point is an interaction contract, and the only real decoupling I get out of them is for swapping one implementation for another. I guess I invoke Liskov here or something. In this case, this is the main intent of my question - for this iteration I'm backing the API with simple objects, but it's a communication stack, so I may eventually go for a reactive or full-on Actor Model implementation eventually. – Bondolin Sep 19 '22 at 21:04
  • @Bondolin The quotes in [this answer](https://stackoverflow.com/questions/681700/interface-naming-convention/16549894#16549894) are pretty apropos but somehow the answer goes somewhere else entirely. This is what I find problematic: the standard is fubared but if you don't follow it, everyone will hate you and probably for good reason. Kind of a damned if you do, damned if you don't situation. At this point I think C# (or IDEs) should generate an 'I' interface for every class you create. – JimmyJames Sep 19 '22 at 21:14
  • It seems stupid that a naming standard would make things so complicated (but it does!) I want to be helpful so I think what all this really means is that you need to really think about how you see your design evolving. It's really hard to predict the future but you are going to have to do a Babe Ruth and [call your shot](https://en.wikipedia.org/wiki/Babe_Ruth%27s_called_shot). – JimmyJames Sep 19 '22 at 21:20
  • @JimmyJames come to think of it, I asked about this almost 8 years ago.[=](https://softwareengineering.stackexchange.com/q/257976/131624). Wow I feel old. – candied_orange Sep 19 '22 at 22:43
  • 5
    `I` exists because the .NET framework treats `class` and `interface` very differently in terms of its inheritance model. – dan04 Sep 20 '22 at 00:30
  • While I agree with the general sentiment (interfaces are horribly overused), there are other reasons than the one listed why you might want to use interfaces. A major one: They allow you to hide dependencies and allows for things like an onion architecture. I don't want my business logic to depend on whatever database technology I rely on. I can use `IProductRepository` without ever having a dependency on Entity Framework in the business code. – Voo Sep 20 '22 at 07:58
  • 2
    @Voo how is that different to having `class ProductRepository`? – Caleth Sep 20 '22 at 08:55
  • @Caleth The interface definition should be technology agnostic (say `Product GetProduct(..)`), while the implementation obviously requires some specific tech (say EF Core). If your business code defines the interface and uses it, it has no dependency on EF Core, which means it can't leak into your nice, clean logic. If on the other hand you use the class directly, you can now use EF Core throughout your whole domain logic. It's a classic inversion of control. – Voo Sep 20 '22 at 09:03
  • 2
    @Voo again, how does that relate to `class` vs `interface`? The domain defines (`I`)`Product` and (`I`)`ProductRepository`, and you have e.g. `EFProduct` and `EFProductRepository` as subtypes of those. – Caleth Sep 20 '22 at 09:49
  • @Caleth Sure you can abuse abstract classes without behavior instead of using C# interfaces to create an architectural interface, but that's a very different topic (the fact that "interface" has two rather different meanings, has lead to lots of confusion down the road). The point here is that the separation of architectural interface and implementation (contrary to having both in a single entity) can be useful to remove dependencies. – Voo Sep 20 '22 at 10:39
  • 7
    @dan04 According to one the founding members of the .NET team '... the "I" prefix on interfaces is a clear recognition of the influence of COM (and Java) on the .NET Framework. COM popularized, even institutionalized, the notation that interfaces begin with "I."' Which is quite a different reason than what you claim. If you didn't use the convention, the code will still compile and work just the same, right? – JimmyJames Sep 20 '22 at 13:46
  • 2
    @dan04 Java treats `class` and `interface` very differently in terms of its inheritance model and it doesn't have the harmful `I` wart. The difference BTW being that they finally figured out how to support multiple inheritance which is why they created the interface keyword. – candied_orange Sep 20 '22 at 16:20
  • 12
    @Voo To be clear, I am not advocating again interfaces. They are very useful for abstractions. What I think is detrimental is the pattern where every class gets an interface even if there's no practical purpose for it. If all you are doing with interfaces is stuff like this: `IFoo foo = new Foo();` there's really no point to having the interface. – JimmyJames Sep 20 '22 at 16:22
  • 3
    @dan04 I am curious though, in practical terms, what is it that makes 'knowing' at a glance that something is an interface so crucial? I never once in my 10+ years coding in Java recall struggling with this. Any halfway decent IDE should make it readily apparent. How does this help you specifically? – JimmyJames Sep 20 '22 at 16:31
  • @Jimmy I mean you'd usually use an IOC controller, but as I said it's useful to separate dependencies. This seems less obvious than I thought it'd be (I just wanted it as an addendum for other situations where interfaces are useful other than having multiple implementations) so it's probably not useful and try and explain this in comments. – Voo Sep 20 '22 at 17:27
  • 2
    @JimmyJames rather than crucial it's something I actively do not want to know about. I hate the I prefix in C# and FooImpl in Java. We're hacking our way around the bug in the languages. We were never supposed to know what a type was when using it. – candied_orange Sep 20 '22 at 18:30
  • 1
    @candied_orange I agree with you but I'd like to hear the other side of the argument, OR if there is no such meaningful argument, I take it as confirmation of my belief that it's a bad idea, like (almost) all warts. – JimmyJames Sep 20 '22 at 18:33
  • As someone that was introduced to C# later in my career I also don't really understand why it uses so many interfaces. And worse, I see the other way around happening: C# sharp developers working with another language, and using interfaces all over the place where it's really not idiomatic for that language. – Ivo Sep 22 '22 at 08:29
  • One big wrinkle is that `interface` is a keyword, as well as a concept., so the question "Is it good practice to only expose interfaces (concept)?" has the answer "by definition, that's the only thing you can do" whereas the question "Is it good practice to only expose `interface`s (keyword)" has answers like this. – Caleth Sep 22 '22 at 11:02
  • @candied_orange: IMHO, a lot of problems could have been avoided if there had been a means by which an interface could specify "constructors" which would be translated as static factory methods. There are many cases where a request for an object which satisfies an interface (eg. FillableWithWater) but has no other requirements could be satisfied reasonably well by an object of a particular fixed type (ExpandableWaterContainer), and someone who wants an object that meets other requirements as well (e.g. the ability to drop water from a height) would know of some particular type that ... – supercat Sep 22 '22 at 18:24
  • ...meets the expanded requirement (e.g. FireFightingAircraft). – supercat Sep 22 '22 at 18:25
  • @supercat I totally thought your concrete implementation was going to be a water balloon ; ) – candied_orange Sep 22 '22 at 20:00
  • @supercat Does the addition of allowing static methods on interfaces address what you are describing, at least in part? I thought that was a really good addition in Java, along with default implementations. With that feature, if you never expose a public constructor on your class (e.g. only expose a factory method) it makes it really easy to refactor to an interface later, if needed. – JimmyJames Sep 22 '22 at 21:37
23

While this may be an opinion-based question, and the real answer is the same as with many endeavors in software design and development ("it depends"), I'm going to say yes, do not expose these implementation details by adhering to the Principle of Least Privilege. If and only if you find your design has good reason to expose the implementation for extension -- adherence to another design guidepost known as the Open/Closed Principle -- should you make that available to other parties.

Jesse C. Slicer
  • 6,002
  • 2
  • 32
  • 44
10

In addition to the points already mentioned by JimmyJames, there's one additional drawback in exposing interfaces rather than classes: Your consumers can make their own classes implement them, e.g.

// consumer-side code
class MyFoo : IFoo
{
    void DoSomething() { ... }
}

Why is this a problem?

  1. Imagine you want to extend your IFoo interface with an additional DoSomethingElse method. You can't, without breaking MyFoo's code.

    (Note that C# 8+ allows you to work around this issue by adding a default interface method, but if you did not intend your interfaces to be implemented by consumer-side code, why clutter them with code that does not belong there?)

  2. Consumers can replace your implementation with theirs. Imagine that your API exposes a method FrobnicateFoo(IFoo foo), which is meant to be used with a Foo created by your factory methods. Well, your consumer can just pass in their own MyFoo instance instead.

    The general recommendation in C# is to make classes "sealed" unless they were explicitly designed to support inheritance. You can't "seal" your interfaces to consumer-side code.

What you can do is to only expose an abstract base class: If all its constructors are internal, your API users can't inherit from it. However, this is not something you need to do a priori: You can start with a regular class Foo. Later, should the need arise to use a subclass (example), you can make your factory method return a (private) SubclassOfFoo instead without breaking backwards compatibility.

Heinzi
  • 9,646
  • 3
  • 46
  • 59
  • This is one serious drawback :) – mishan Sep 20 '22 at 16:52
  • The corresponding advantage is that consuming code can deal with either your IFoo or the third-party IFoo. – user253751 Sep 20 '22 at 18:27
  • @user253751: Indeed. The cost is that `FrobnicateFoo(IFoo foo)` can only use the public interface of `IFoo` to interact with `foo`, whereas `FrobnicateFoo(Foo foo)` could use `internal` members of `Foo`, which can be modified later without introducing a breaking change to the API. – Heinzi Sep 20 '22 at 20:46
  • @Heinzi if you are using this pattern there's a good chance that FooFrobnicator.FrobnicateFoo(IFoo) does cast to Foo - but OtherFooFrobnicator may cast to OtherFoo (we may specify that each foo must be frobnicated by the corresponding frobnicator) – user253751 Sep 20 '22 at 23:58
  • @Heinzi This seems bad design to me. If a method needs a `Car`, you can't make it ask for a `Vehicle`. I know my mechanic wouldn't be quite happy if I asked him to repair a Boeing 737. – Simone Sep 21 '22 at 07:46
  • 1
    @Simone: I wholeheartedly agree, that's the whole point of my answer: That OP's plan to hide the concrete Foo (Car) and only expose IFoo (Vehicle) to the API user might not be a good idea. :-) – Heinzi Sep 21 '22 at 09:42
  • 2
    Thanks for the feedback, good stuff. This brings up some tradeoffs I'm maybe dealing with, which is exactly what I was asking for. We're engineers, we expect any design to have its strengths and weaknesses, so it's a little disconcerting when I see one but not the other. Know thy enemy. That said, if these are what kind of issues I might be dealing with, I think I am ok in this case with the trades I am making, but I want to make sure I am understanding your points correctly, so let me try to respond to them and maybe you tell me where I'm off - – Bondolin Sep 21 '22 at 12:24
  • 1. Adding an abstract method would break client code just like adding a method to an interface; however, using a class does gives me the freedom to add a *concrete* method to the exposed class w/o breaking client code. So that could be useful. – Bondolin Sep 21 '22 at 12:28
  • 2. I agree using a sealed class or internal constructor prevents user implementations. However, I'm not sure how much of an issue this ends up being in this case - in fact, it might be less a bug and more a feature. I want my API to work well in a unit test context. So I actually want my users to be able to provide mock implementations of these interfaces/abstract classes/whatever. If they can't, they have to resort to making adapters and mock those instead. As far as continuity of behavior goes, I understand that to be the whole point of the Liskov Substitution Principle. – Bondolin Sep 21 '22 at 12:33
  • 1
    @Bondolin: Ad 1: Exactly. Ad 2: I agree, if you *want* to provide the option to have user-defined implementations of your interfaces, that's a great feature. You don't *need* it for easy unit testing though: You could have a `public FrobnicateFoo(Foo foo)` which calls an `internal FrobnicateFoo(IFoo foo)`, and the internal method is visible to your unit test project via the `InternalsVisibleTo` attribute. This would allow you to hide your IFoo interface, so that any changes to it would only affect your project and your unit test project, not the public API. – Heinzi Sep 21 '22 at 14:04
  • 1
    #1 is not a drawback. If you change the definition of `IFoo`, you *should* break consumer code, because it no longer works with your library. This is why interfaces take so much thought and should only be changed when necessary, and why the Interface Substitution Principle is so important. – JounceCracklePop Sep 21 '22 at 22:30
  • 2
    #2 is not a drawback either; in fact it's *the whole point*. If you write the method `FrobnicateFoo(IFoo foo)` and it doesn't work with every valid implementation of `IFoo`, then your method is wrong. Why is this supposed to be a downside? The answer is to fix your method signature, or fix your method. Honestly it sounds to me like you are using and understanding interfaces very incorrectly. – JounceCracklePop Sep 21 '22 at 22:32
  • 1
    @JounceCracklePop: *"The answer is to fix your method signature..."* But that was the whole point of the question: Whether to expose the API as `FrobnicateFoo(IFoo)` or as `FronbnicateFoo(Foo)`. The first one provides greater flexibility for the consumer, but it comes at a non-zero cost for the API maintainer: As you correctly point out *"interfaces take so much thought and should only be changed when necessary"*. – Heinzi Sep 22 '22 at 07:17
  • @Heinzi It feels weird discussing the hypothetical "but what if your design is bad?" in SE Stack Exchange. I mean, no design is perfect, but we're trying to understand the ideal here. If `FrobnicateFoo` truly only works with one specific implementation `Foo`, then it exactly fits the definition of a *private method* on `Foo`. This is *not* a drawback of only exposing `IFoo`, and again it just means the method signature should be fixed (in this case, becoming private). – JounceCracklePop Sep 22 '22 at 22:33
  • @JounceCracklePop: If `FrobnicateFoo` is private, how is the consumer of OP's API supposed to call it? – Heinzi Sep 23 '22 at 04:36
  • @Heinzi How in-depth can we discuss this while still using meaningless domain words? Good designs don't look like that. It's a code smell. It's bad practice. That was the question. If you have `IFoo` defined and you're exposing a method that you expect the consumer to call and it expects only a specific `Foo`, that's a great point to focus your attention to review your design, because it's smelly. Something is wrong with `IFoo`. Interfaces are always designed according to the needs of the consumer, not the implementer. – JounceCracklePop Sep 25 '22 at 06:38