6

I am reviewing a colleague's code and as part of the changes, this person introduced a few new pure data classes like so (C#):

public class Result : IResult
{
    public bool Succeeded { get; set; }
    public string ResponseMessage { get; set; }
}

And then afterwards defined an interface for the data schema, and used the interface throughout the code:

public interface IResult
{
    bool Succeeded { get; set; }
    string ResponseMessage { get; set; }
}

When I asked why, she just said it was "good practice", and I couldn't find any reason to complain other than that it seemed like an unnecessary abstraction.

Is this actually documented somewhere as a good practice? Is there any reason you would define an interface if you know that the class is just going to contain pure data and nothing else? (if it helps in this case, the class for a JSON REST API object)

  • 3
    "It is good practice" is not a valid reason; it's just an excuse to not have to explain the real reasons why (if any actually exist). By the same token, "is this actually documented somewhere as a good practice" is not a valid reason either; you have to tie it to your specific needs. – Robert Harvey Apr 9 at 17:17
  • 2
    Possible duplicate of The need for adding an interface to every class – gnat Apr 9 at 18:11
  • 1
    Are there any other interface implementations of IResult in your codebase, apart from Result? – Doc Brown Apr 9 at 18:12
  • 2
    The mutable anemic class makes me cringe. – Nick Alexeev Apr 9 at 23:48
  • @NickAlexeev Yes but AFAIK in C# readonly can be only set by constructor, so you'd lose the nice initialization person = Result { Succeeded = ...., .... } – max630 Apr 10 at 7:11
8

When used in this manner, interfaces denote capabilities.

For example, the IEnumerable interface indicates that the class in question is capable of being enumerated, in this manner:

foreach(var item in collection)
{
    // Do something with `item`
}

where collection is some object that implements the IEnumerable interface; that is, you can walk through the items in the object one at a time, and do something with each item.

In the case you've described, it would seem that IResult is a capability that provides for a couple of "standard" fields to operate on. It follows that, in addition to the class:

public class Result : IResult
{
    public string Succeeded { get; set; }
    public string ResponseMessage { get; set; }
}

.. I assume that there are also other classes that can implement the same interface. Here is a possible example:

public class Message : IResult
{
    public string Content { get; set; }
    public string Succeeded { get; set; }
    public string ResponseMessage { get; set; }

    public void Foo(int someParameter)
    {
       // ...
    }
}

It follows that you can use an interface to provide two different implementations. For example, it's possible that IResult exists so that a Mock Object can be used with it, for testing purposes.

In the absence of either of these possibilities (i.e. your colleague is merely implementing an interface for a single pure data class, just because), I claim that the interface serves no useful purpose, other than to provide an unnecessary layer of indirection.

  • 6
    To expand on the last paragraph: such useless interfaces can be an indication that someone has smoked too much SOLID and now believes you should never depend on concrete classes. That is good advice at component boundaries but not within components. In particular, the code of some application can always be refactored when the interface becomes necessary. Until then: YAGNI. – amon Apr 9 at 18:10
  • To add to this answer: Keep in mind that a class implementing the interface could provide more elaborate implementations of the properties instead of using auto-properties. – Tim Pohlmann Apr 10 at 11:16
6

As the code currently stands,

public interface IResult
{
    bool Succeeded { get; set; }
    string ResponseMessage { get; set; }
}

we have an interface that replicates a simple data class. It adds complexity to the code without bringing any benefits. It's not good practice.

However, if we change that interface:

public interface IResult
{
    bool Succeeded { get; }
    string ResponseMessage { get; }
}

we now have something quite different. We have a read-only abstraction over that data. So if we make another change, to the class this time:

internal class Result : IResult
{
    public bool Succeeded { get; internal set; }
    public string ResponseMessage { get; internal set; }
} 

we've now got an internal class that has internally writable properties and a read-only public view of that data.

This keeps the JSON serializer code happy. Most such libraries (I'm thinking specifically for .NET here as you've tagged your question, C#) do not handle creating objects via constructors very well. They want writable properties, but don't care whether those properties are public or not as they use reflection to construct them.

And this approach brings benefits to the rest of the code as the rest of the system is only given access to a read-only version of the data. This encourages the use of immutability and puts the responsibility for creating such objects into just one place in the system. And they definitely are good practices.

  • I think this is the most relevant reason for this particular use case – Logan S. Apr 11 at 22:41
5

One of the ways I remember the relationships between interfaces, inheritance, and encapsulation is the following:

  • Inheritance: is a relationship. Car is a Vehicle.
  • Encapsulation: has a relationship. Car has a Tire.
  • Interface: acts as relationship. Car acts as an IPeopleTransport

The reason I mention that is because there needs to be a reason to have the relationships to begin with. If there is only ever one way an object will act, then there is absolutely no reason to have an interface to it.

You'll notice the acts as relationship implies the sentiments expressed in Robert Harvey's answer.

Essentially, the interface allows you to treat any object that implements it as if it as if it fulfilled that role. One example would be the C# Enumerable.Range to enumerate over numbers with a foreach call.

foreach(var i in Enumerable.Range(10,100))
{
   Console.WriteLine(i);
}

In the specific example you provided I would argue that Result only has one role, so there is no need for the IResult interface.

2

Premature keyword interfaces mitigate some problems:

  1. Enables multiple inheritance in languages that need keyword interfaces for that (Java & C#)
  2. Eliminate the need to change the name in using clients (I prefix, C# only)

Both are only important if you must strictly follow the open closed principle. This encourages not making changes to tested working code. That is far more important when you have published code libraries that get updated separately. If you can change every class you feel like changing every version the only real cost is redoing your testing. Refactoring tools make the actual code change trivial.

Premature keyword interfaces cause their own problems:

  1. Indirection can solve any problem except too much indirection

The "everything gets an interface" mentality creates piles and piles of ceremonial code that can be created without thinking. There's even an "extract interface" refactoring to do this for you. This is not fun to work with when you just want to add a new field. Ideally, the decision that a field should exist should happen in only one place.

Let's be honest. The keyword interface is a terrible patch to these languages broken type system that was created before they knew how to support multiple inheritance properly.

But the Dependency Inversion Principle says to depend on abstractions!

  • High-level modules should not depend on low-level modules. Both should depend on abstractions (interfaces).
  • Abstractions should not depend on details. Details (classes) should depend on abstractions.

Exactly right. But nothing in that says your abstraction must be a keyword interface.

A keyword interface is when you actually type out the word "interface". Interfaces come in many other forms than that. Every class that doesn't disable inheritance is an interface. So sorry but String is not an interface. But most all of your other classes are. You don't need everything to have a keyword interface

What makes it tricky is realizing when your class makes a good abstraction and when it doesn't. Protected constructors help keep clients from knowing that your class is concrete. If clients don't know your concrete you are an interface. That's all there really is to it. Lazy architects try to skip needing to check for this by insisting on giving everything a keyword interface. But a little work checking this now will allow you to put off creating a keyword interface before you actually need it to support polymorphism.

An example from my Java damaged brain:

class Report {
    public print() {
        System.out.println(
            "Succeeded: " + result.succeeded() + " " + 
            "Response Message: " + result.responseMessage() 
        );
    }
    Report(Result result) {
        this.result = result;
    }
    Result result;
} 

Looks like Report depends on Result right? Yeah but so what? Who said Result was concrete? Report doesn't use new on Result. For all Report knows Result is a keyword interface. Report might be a composition containing many objects. We don't know. That not knowing is what you need to preserve. That not knowing is what makes Result an abstraction. You don't have to give everything a keyword interface to do that. Do that when you need to do that.

If your programmer tries to invoke SOILD as an excuse for this let me show you a little something from Mr Martin himself:

enter image description here

Note that the models have no interfaces on them at all. That doesn't mean the models will never change. Just preserve the ability to add interfaces later and there's no need to add them now.

The interfaces you do see here are protecting clients from knowing exactly what behavior they depend on across a boundary that is likely to see independent changes. The models have no behavior so the boundary isn't as important to them.

  • 1
    I first thought you are showing the diagram "ironically", you know, to say: The guy who came up with SOLID also came up with this mess, so let's take these things with a large grain of salt. But then I realized you mean that as a positive example / clarification. So my bad :) – Robert Bräutigam Apr 10 at 11:15
  • @candied_orange just for the record. Open arrows are "use" relationship and closed arrows are "inheritance" relationship. Right? This diagram always bugs me due to so many arrows. – Laiv Apr 10 at 12:04
  • @Laiv Right. --> is using. --|> is just polymorphism. Doesn't really matter what kind. That's used to protect against knowing the implementation of some behavior across a boundary that is likely to see changes. The models have no behavior so no need to protect them like that. The most confusing thing is people expect the arrows to be about flow of control. They aren't. They're about what depends on what. But since flow of control typically goes thru -|> backwords you can figure it out like I showed here – candied_orange Apr 10 at 12:29
  • 1
    @RobertBräutigam actually I recommend taking all of this with a grain of salt. The biggest problems this SOLID stuff causes come from over applying it in situations that don't call for it. Ask why before following any pattern. Tear down patterns that were followed by rote. Only permit code to exist that can justify it's existence. Just be sure you understand what you're tearing down. – candied_orange Apr 10 at 15:03
0

Depends on how you use the interface. Realworld useful example from our domain

public interface ICreated
{
    string CreatedBy { get; set; }
    DateTime Created { get; set; }
}

public class CreatedListener : IBusinessContextListener
{
    public void OnSavingChanges(IBusinessContext ctx)
    {
        foreach (var created in ctx.Db.ChangeTracker.Entries<ICreated>().Where(c => c.State == EntityState.Added))
        {
            created.Entity.CreatedBy = ctx.Username;
            created.Entity.CreatedUTC = DateTime.UtcNow;
        }
    }

    public void OnTransactionCommited(IBusinessContext ctx)
    {
    }
}

collection
   .AddScoped<IBusinessContextListener, CreatedListener>()
   .AddScoped<IBusinessContextListener, UpdatedListener>();

Here we use interface for two things A) markup interface B) To abstract certain behavior so that generic code can operate on the entity. Without either of these two the interface becomes redundant.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.