4

I have this out of context scenario, where what I think is good practices leaves me in a situation of both implementing an interface, and using composition to do the implementation.


Imagine the following:

I have a Character with Health and Mana, like so:

interface ICharacter
{
    Health Health { get; }
    Mana Mana { get; }
}

class Character : ICharacter
{
    public Health Health { get; private set; }
    public Mana Mana { get; private set; }

    public Character(Health health, Mana mana)
    {
        this.Health = health;
        this.Mana = mana;
    }
}

I then decide, that Health and Mana should be combined into one object, since they belong together.

interface IResource
{
    Health Health { get; }
    Mana Mana { get; }
}

I change my Character to

interface ICharacter
{
    IResource Resource { get; }
}

class Character : ICharacter
{
    public IResource Resource { get; private set; }

    public Character(IResource resource)
    {
        this.Resource = resource;
    }
}

However, so that clients of Character can access a character's health without saying character.Health.Current and "violating" the principle of least knowledge I want Character to provide this information. And to improve encapsulation I would no longer disclose how Character stores its Health and Mana, like so:

interface ICharacter : IResource
{
}

class Character : ICharacter
{
    private IResource resource;

    public Health Health
    {
        get
        {
            return resource.Health;
        }
    }

    public Mana Mana
    {
        get
        {
            return resource.Mana;
        }
    }

    public Character(IResource resource)
    {
        this.resource = resource;
    }
}

This gets my Character back to having Health and Mana which I like, but Character is now implementing the same interface it is am composed of.


In implementation, this looks quite a lot like the Decorator Pattern, but in intention, they have nothing in common.

Questions

  1. Is this pattern / antipattern of implementing the same interface that a class is composed of recognized, and if so, what is it called?

  2. Would you consider this good a idea over just having a property to IResource? - Why? / why not?

  3. Any situations to be wary off, where this is a definitive no-go?

Sidenote - ICharacter would certainly have more methods, but to shorten it and keeping it concise, I only included the relevant part here.

amon
  • 132,749
  • 27
  • 279
  • 375
Chris Wohlert
  • 529
  • 3
  • 15
  • 8
    When books and articles refer to *"prefer composition over inheritance"*, they are specifically not talking about *interfaces*; they're talking about *state* and *behaviour* inherited from a base class. As your example demonstrates, interfaces are often a useful tool for using composition instead of inheritance. – Ben Cottrell Apr 12 '17 at 08:45
  • 2
    Game developers tend to handle such design issues a bit different than business application developers. You might want to ask this question on https://gamedev.stackexchange.com instead. – Philipp Apr 12 '17 at 09:19
  • @BenCottrell, I always thought *prefer composition over inheritance* was about being able to change the functionality, without changing the class, even in runtime. Implementing an interface has this same restriction as inheriting a class. – Chris Wohlert Apr 12 '17 at 10:33
  • 1
    @Philipp, This is a business application rewritten to a domain everyone understands. Thanks though. :) – Chris Wohlert Apr 12 '17 at 10:41
  • I'm not sure what you mean by *"Implementing an interface has this same restriction as inheriting a class."*, since an interface doesn't prescribe any functionality for the class implementing it, the only thing prescribed is how other things can interact with it. – Ben Cottrell Apr 12 '17 at 11:14
  • 1
    A class implementing an interface cannot change its behavior without being rewritten. Only through composition is this possible. – Chris Wohlert Apr 12 '17 at 11:18
  • Changing IResource shouldn't require a change to the Character class. Isn't that the whole point to using dependency injection in the constructor in the first place? – JeffO Apr 12 '17 at 13:33
  • @ChrisWohlert I would think the class implementing the interface is not the same as the one whose behaviour needs to change (i.e. the "composed" class which might want its dynamic behaviour or unit test stubs to be injected). instead it would make more sense for the class implementing the interface to be the one implementing one possible variation of behaviour. The advice to *"prefer composition"* often uses interfaces to de-couple that behaviour from the composed class. However the composed class itself doesn't actually gain or lose anything by implementing the interface. – Ben Cottrell Apr 12 '17 at 16:10
  • The main thing to recognize is that the "principle of least knowlege" can violate the "principle of least code", at least the way you interpret it. In the places where both Health and Mana are needed, you have not explained why anyone would pass a Character or ICharacter instead of a Resource or IResource: if they only need knowledge of those two things, provide the minimum. – Frank Hileman Apr 13 '17 at 23:00
  • Sounds like your character methods might be an adapter/facade rather than a decorator (given that it's intent is to make the API prettier rather than to add behaviour). – kwah Apr 15 '17 at 07:25

4 Answers4

10

Composition over Inheritance, why not both?

The principle is correctly quoted as "prefer composition over inheritance". No one said you couldn't do both. Sometimes you have to do both. But given a choice, composition is usually the better one.

Your IResource reminds me of different patterns than the ones you mention. Parameter object makes constructing an object easier by bundling it's dependencies together.

Beware, taken to an extreme this smacks of a service locator which can be bad in some situations.

Respecting the Law of Demeter is a good thing if you understand it well. It is causing you a problem here but it's a good problem. It's forcing you to simplify access to health and mana until it's obvious that your Character isn't doing anything but aggregation.

What's missing is abstraction. Sure you promised Character would have more methods but as far as I see here Character isn't adding anything but unneeded indirection.

Right now Character looks like a brain dead value object (such as String). If that's what it's going to be then give it it's own copies of these values and flatten it out.

If Character is going to be a behavior object I'd expect Health to be hidden by encapsulation and interacted with using methods like Wound(int) and Die() not GetHealth(). This follows Tell, don't ask. A character's heath status is no one else's business. This is real encapsulation.

In either case the Character class should not be a brief stop on the way to dealing with character details. This should be THE place to resolve them. Don't make me dig past it.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • Thank you very much @CandiedOrange. The *Parameter object* is exactly how I got into this situation without knowing about it, thank you. `Character` certainly is a behavior object with methods like `Attack`, but thank you for clarifying that *Encapsulation* is my issue, not *Law of Demeter*, `Wound` and such certainly belong on `Character` and `Character` should probably only provide *readonly* information such as `CurrentHealth` without giving access to the full `Health` object. – Chris Wohlert Apr 18 '17 at 10:39
  • @ChrisWohlert most welcome. The issue isn't "read only". That would still be a getter. Heath is state and exposed state tempts people to do work on it outside of the object. That turns the object inside out. But presumably you have to report health to something. If that's say a view (GUI?) then the best way to follow tell, don't ask is to pass a (health focused?) view into Character and tell that view what % the health is at. That preserves encapsulation. Also lets you update as soon as health changes. – candied_orange Apr 18 '17 at 10:52
  • I'm sorry, by `readonly` I should've said `immutable`. Preventing others from working on it outside was the problem I tried to prevent with that sentence. The passing of a view object to the `Character` is very interesting, not something I would normally do. Would you prefer this to say a `HealthChanged` event? Which is how I would normally solve that issue. – Chris Wohlert Apr 18 '17 at 14:39
  • Passing a view in and calling it on a change IS an event. Look up the observer pattern. It will show you how to do this for multiple views. Long as you pass the view all it needs then it doesn't need to call you back demanding you have getters. You don't even have to pass the real health. Just enough to display it for this view. Say a percentage. This would make Character a behavior object and mutable. If it has an identity that is not tied to it's state then it's an entity. Immutable won't let you even change heath without creating another object. Getters are for value objects like Strings. – candied_orange Apr 18 '17 at 21:36
  • I know an immutable `Health` will not let me change it without creating a new, which is exactly what I was thinking, such that `Wound` would say `this.Health = this.Health.Subtract(damage);`. I would be familiar with your approach in other languages, but since events are the observer pattern in C#, I probably just got it mixed up. The `Character` is already mutable and a behavior object if it has methods that change internal state, no? - *I would only be using one of the solutions, either events or getters.* – Chris Wohlert Apr 19 '17 at 06:16
  • Character sounds like a behavior object to me. See if you can figure out how to make this code work: `character1.damage(character2)` – candied_orange Apr 20 '17 at 03:16
  • Yeah, it already does, just omitted. Like I said, it had methods like `Attack`. – Chris Wohlert Apr 20 '17 at 06:00
4

What benefits are you getting from bundling health and mana together? Those just sound like values to me and separate attributes, but regardless whether they are values or objects you could put them as properties instead. These are meant to be easily checked against the character.

I think you had it right the first time. If your mana/health might have behaviors or rules giving cause to be more than just a value, I think you should program against their interface (which allows you to mock them when unit testing Character's behaviors):

interface ICharacter
{
    IMana Mana { get; }
    IHealth Health { get; }
}

public class Character : ICharacter
{
    public IMana Mana { get; }
    public IHealth Health { get };

    public Character(IMana mana, IHealth health)
    {
        Mana = mana;
        Health = health;
    }
}

If they are just values, just have them be values. With the value approach you might not need to even to provide them to the constructor. You can just use reasonable defaults. Now where encapsulation becomes useful is you can bake-in the healing and mana logic within a publicly exposed method and the calling code cannot modify the health and mana without using the exposed methods:

interface ICharacter
{
    int Mana { get; }
    int Health { get; }
    void Heal(int numPotions);
    void RejuvenateMana(int numPotions);
}

public class Character : ICharacter
{
    public int Mana { get; private set; } // default is 0
    public int Health { get; private set; } = 100;

    public void Heal(int numPotions) 
    {
        Health += numPotions;//todo: add real logic
    }

    public void RejuvenateMana(int numPotions)
    {
        Mana += numPotions;//todo: add real logic
    }

}

Once you realize that you might have different behaviors and rules, you'll likely want to add in those rules as dependencies or have some other way to vary the behaviors between character types. Depending on how everything works together you might find that healing, rejuvenating, taking damage, etc might be handled by a different object and in that case you'd want to make health/mana be publicly assignable properties if ICharacter will be acted upon by something else. Otherwise it might be sensible to provide the dependencies that calculate health/mana in as a dependency.

Would you consider this good a idea over just having a property to IResource? - Why? / why not?

These seem to me to be 2 separate attributes of character. I think combining them would be doing so just for the sake of orgranizing things, and would cause calling code to have to unnecessarily dig for information that could have been readily available.

Is this pattern / antipattern of implementing the same interface that a class is composed of recognized, and if so, what is it called?

I'd call this over-engineering and over-generalization. IResource has become such a generic thing in this case it doesn't really have a good single meaning and breaks Interface Segregation principle in my opinion by being too generalized and not broken apart enough. What you are doing though by implementing an interface and taking in dependencies that implement that same interface is common though in some circumstances, in some programming languages all objects inherit from some kind of Object or in the case of Objective-C there is an NSObject class all inherit from and an NSObject protocol (interface). This pattern has been used before to bake things in for everything to have but usually only works in cases where Everything will need to have the same exact properties (ie: interface components will always have x,y positions, etc).

Any situations to be wary off, where this is a definitive no-go?

As I mentioned before I feel like this breaks ISP. Clients should not have to depend on things they don't need. If character is a resource then a character can't change independently from IResource since ICharacter inherits everything in the IResource interface. And ICharacter could potentially have IResource traits it doesn't need if IResource is used in such a generic manner and more traits get added that do not belong to ICharacter.


Edit: I'd really like to see how the Character class will interact with other classes. For instance if there was a BattleCalculator class that takes in 2 characters and looks at their attributes to see how much damage will be applied based on attributes like Health, Agility, Weapon, etc it might be the case that it doesn't need to know about mana (assuming mana is just being used to change other character attributes). In that case you can further split the interfaces so mana-related stuff is in a different interface, and the battle calculator doesn't need to know about it, and this assumes all characters would have Mana:

public interface IManaBearer 
{ 
    IMana Mana { get; } 
}
public interface ICharacter { ... }
public class Character: IManaBearer, ICharacter {...}

public class CharacterBattleCalculator
{
    public CharacterBattleCalculator(ICharacter player1, ICharacter player2)
    {
        ...
    }
    ...
}
morbidhawk
  • 362
  • 2
  • 11
  • 1
    Adding on to the points you've made here, with an example, a destructible chest will have health, but mana makes NO sense for it. This is why we have the interface segregation principal :D – TheCatWhisperer Apr 12 '17 at 20:43
  • 1
    Obviously, methods like `Heal` would be on `Health`, making it **not** a value object. – Chris Wohlert Apr 18 '17 at 10:29
3

Is this pattern / antipattern of implementing the same interface that a class is composed of recognized, and if so, what is it called?

It's a Proxy or Dectorator. Proxies generally refer to Wrappers or Adapters that simply forward method calls onto another class or instance — like an internal property. And Decorators are basically Proxies that come with additional methods or "overrides."

Would you consider this good a idea over just having a property to IResource? - Why? / why not?

In and of itself, it's a neutral idea. It comes down to the nomenclature. If an ICharacter is an IResource, then it's a good idea. If it only has an IResource, it's a terrible idea.

Any situations to be wary off, where this is a definitive no-go?

In addition to when it's not a semantic fit, it's also a bad idea when the wrapping will be hard to maintain. This could be the case if the inner interface is volatile (still in development). ... I'd personally also avoid a proxy/decorator if the "principle of least knowledge" is my only justification — especially for large interfaces.

Composition over Inheritance, why not both?

In your case, because you're not actually doing both!

Classes are inherited. Interfaces are implemented. So, you're doing "both composition and implementing interfaces." And, that's a perfectly normal, everyday thing to do.

This is what "both" composition and inheritance would look like:

class A : B
{
  public C Prop;
}

Class A inherits from B and is composed of C.

So, generally speaking, why not do both?

No reason at all. If it makes sense for A to inherit from B and use C in its composition, do it. If not, don't.

Just do what makes sense.


Bonus advice time! For yours and your coworkers' sakes, be aware of why the principles exist. Preferring composition over inheritance has both benefits and drawbacks. And frankly, the advantages of Principle of Least Knowledge are shaky at best (in my opinion). But primarily, if C from our example can do things we don't want consumers of A to do directly, C should be private. But, if we're creating a wrapper for each method on C, we should just make C public.

svidgen
  • 13,414
  • 2
  • 34
  • 60
  • Although "interfaces are implemented", abstractly they are identical to base classes, and that is why they are not present in many object oriented languages (they are not needed). So multiple interface implementation is a type of multiple inheritance, usually the only one allowed (otherwise, we have no need for interfaces). – Frank Hileman Apr 13 '17 at 22:55
  • @FrankHileman I'm really not sure what you're trying to say! But, I very much disagree that interfaces are "identical to base classes." When you implementing an interface, you're explicitly *not* inheriting anything, as you do from a base class. You're agreeing to fulfill a contract. Whereas, when you inherit from a base class, you're literally taking another class, along with its full implementation *and* all of it's contracts, as a "starting point." – svidgen Apr 14 '17 at 13:43
  • An interface, in a language such as C# or Java, is functionally identical to a purely abstract base class: one with no members implemented. Interfaces use inheritance exactly the same way as classes. The concept of interface was added to these languages only to work around a flaw in the language design: the inability to do multiple inheritance with classes. It was thought this would reduce complexity, but at the time, design issues with multiple inheritance were already solved. Interfaces should only be used when you need multiple inheritance. – Frank Hileman Apr 14 '17 at 13:55
  • An interface by itself is not a contract, no more than the abstract members in a base class. A contract specifies semantics using pre-conditions, post-conditions, and invariants. An interface is simply a collection of member signatures with no meaning. If you add some documentation to an interface, and someone reads it, you can at least create an informal contract. – Frank Hileman Apr 14 '17 at 13:57
  • @FrankHileman Regarding C# -- no. Abstract classes can contain implementation details. Not sure about Java. But yes ... that's what we mean by "contract." We mean, "this class has these public methods and properties." – svidgen Apr 14 '17 at 14:02
  • "abstract members" contain no "implementation details". Consider a modified form of C# where multiple inheritance using classes is allowed, and interfaces are removed from the language. In this case, the role of interface would be taken over by base classes with abstract members. That is exactly how other languages work. Interface method dispatch is fundamentally similar to class method dispatch. – Frank Hileman Apr 14 '17 at 14:05
  • If getting your code to compile is considered fulfilling a "contract", then the contract essentially has no conditions -- not the definition of contract in software engineering. An interface implementation can throw exceptions and do all kinds of crazy things, yet still compile. – Frank Hileman Apr 14 '17 at 14:08
  • @FrankHileman But "abstract classes" *can* contain implementation details. Hence, they're fundamentally different than an interface, which *only* serves as a *basic* contract. ... And yes, it *is* a contract, even if you don't think it's a very good one :) – svidgen Apr 14 '17 at 14:28
  • "Pure abstract" means there is no implementation. Please see the terminology in C++, for example. – Frank Hileman Apr 15 '17 at 15:39
  • @svidgen, Thanks for the answer. To join in on your and Frank's discussion. I get that a Base class and an interface in different, but from the eyes of the implementer, don't they create the same restrictions? The exact restrictions composition resolves. – Chris Wohlert Apr 18 '17 at 10:28
  • @ChrisWohlert The difference is that an interface is designed strictly to be a strictly non-exclusive contract with no *possibility* of carrying implementation details. Once that possibility does exist, you're "inheriting." And, in languages that support it, shortly thereafter follows the possibility of conflicts between multiple ancestors. In languages that *don't* support it, you're just stuck with a single parent. Composition doesn't carry this dilemma at all -- **hence the principle.** Composition makes it easier to pull functionality from multiple classes without conflict. – svidgen Apr 18 '17 at 13:57
  • @svidgen, Just to be clear, I am in no way saying that implementing an interface is the same as inheriting from a base class. What I am saying is, Composition provides the ability to extend functionality without changing the class, even at runtime. Since implementing an interface does not provide this, it is **in this context** equal to inheriting from a base class. – Chris Wohlert Apr 18 '17 at 14:32
  • @ChrisWohlert Heh ... I'm not sure what you're ultimately getting at. But, I don't see implementing an interface as being inherently more rigid at *runtime*. Both an interface and the interface created by the class definition *itself* create compile-time restrictions. Inner "composition objects" also come with compile-time restrictions -- and very similar ones if they're `public`. But at runtime, and with the right tactics or patterns, you can generally screw things up however you like, swapping out implementation details willy nilly regardless of inheritance, interfaces, or composition... – svidgen Apr 18 '17 at 15:01
  • @svidgen, *But at runtime, and with the right tactics or patterns, you can generally screw things up however you like, swapping out implementation details willy nilly regardless of inheritance, interfaces, or composition.*. This is exactly where all the patterns and tactics I have read, tells me to use composition instead of inheritance. If you know any off the top of your head, I would very much like to know them. :) – Chris Wohlert Apr 18 '17 at 16:00
  • @ChrisWohlert Aside from designing your class with methods composed entirely of wrappers, either to other objects or a lambda lookup table, you can generally screw with the memory itself! http://stackoverflow.com/questions/7299097/dynamically-replace-the-contents-of-a-c-sharp-method – svidgen Apr 18 '17 at 16:25
  • @ChrisWohlert "from the eyes of the implementer, don't they create the same restrictions?" You are correct, interfaces and pure abstract base classes are nearly identical in terms of restrictions. The only problem interfaces solve, is the inability to do multiple inheritance, in those languages with interfaces. Interfaces are a specialized form of base class where all members are abstract and multiple inheritance is allowed. – Frank Hileman Apr 19 '17 at 16:02
  • @ChrisWohlert The difference between composition and inheritance (interfaces or base class) can be summed up: adding an interface or base class to an existing class "E" expands the scope of the existing class: "E" can now do "more things". Adding a child object (composition) to "E" means one of two things: if you provide that child object when needed, "use this to do more things"; if you never provide the child publicly, then "E" is delegating work to a child object. Composition can make classes smaller and promote code reuse. – Frank Hileman Apr 19 '17 at 16:11
  • @ChrisWohlert I should mention as well that purely functional composition is included in the term; calling non-instance methods (static in C#) is also composition. – Frank Hileman Apr 19 '17 at 16:13
  • @FrankHileman, Exactly what I tried to say, nice summation. – Chris Wohlert Apr 20 '17 at 06:05
0

Two notes to keep you going:

  1. If someone knows ICharacter then it knows Iresource, so exposing the internals of the resource class didn't made the class using the character know less.

  2. I think you made it more confusing that someone knows this interface of IResource, which was made because you said that this two pieces of data comes together, and then you allow it to access each piece of data individually. You need to decide - either both pieces come together (and then they are also exposed together), or that it makes since to have each of them without the other (and then don't have them make a class)

Belgi
  • 567
  • 1
  • 4
  • 9