Is the Composite Pattern SOLID?
X

4

13

A Leaf in the Composite Pattern implements the Component interface, including Add, Remove, and GetChild methods that a Leaf is never going to use. This seems to be a violation of the Interface Segregation Principle.

So is the usage of Composite Pattern SOLID?

link to Composite Pattern: http://www.dofactory.com/Patterns/PatternComposite.aspx

Xeric answered 16/10, 2009 at 18:3 Comment(1)
SRP and LSP are also violated along with ISP. So SOLID is dangerous. :)Piassava
W
12

The real smell in the pattern as depicted in your link and most books is that Component has the methods of a Composite. I think this is probably because the pattern is fairly old and has been repeated that way for years. My take is that only the Composite should have any methods related to compositing.

I once converted a board game over to a computer game. The playing pieces were placed on a map of earth, divided up into hexagons. 99% of all hexagons represented a single location. Unfortunately, a few of the hexagons contained multiple locations, for example, some had a couple islands inside them. I used the composite pattern to represent these locations, but not as depicted on your link. It was something like this (in Java):

public interface Location {
   Set<Army> getArmies();
}

public class SingleLocation implements Location {

   public Set<Army> getArmies() {
      return armies ;
   }

   private Set<Army> armies = new HashSet<Army>();
}

public class CompositeLocation implements Location {

   public Set<Army> getArmies() {

      Set<Army> armies = new HashSet<Army>();

      for(Location subLocation: subLocations) {
         armies.addAll(subLocation.getArmies());
      }

      return armies;
   }

   public void addSubLocation(Location location) {
      subLocations.add(location);
   }

   private Set<Location> subLocations = new HashSet<Location>();
}

Note that only the Composite has compositing methods, and doesn't even expose the fact that it has children to most clients (in this example, the client only wants a list of Armies from a location - the fact that they are at many sub-locations is irrelevant).

Keep in mind design patterns are not set-in-stone things you must implement exactly. Think of them as recipes. When you follow a recipe while cooking, you certainly can just follow it exactly. However, some cooks will throw in their own twists on the recipe. Others won't even look at it because they are experts and can throw something together in the spirit of the recipe without even thinking about it. The same goes for design patterns. They are malleable recipes.

You can also take those SOLID principles too far. If you read Robert Martin's articles, he states that applying the principles across the board without any thought will yield overly complex code. Software is designed through a series of trade-offs and balancings - sometimes you forgo pure SOLID because it yields cleaner less complex code. If you were to make your code perfectly encapsulated, flexible, decoupled, etc., you will have invented a new programming language :-)

Withdrew answered 16/10, 2009 at 18:47 Comment(1)
That's very common, for good reasons - but there are reasons for the standard version. For example, the items may have functional dependencies on each other besides the simple compositing - the height of a paragraph, for example, depends on the height of each line of text, which depends on the height of each character within the line. Using a separate hash means the composited items have to refer back to the composite to find their immediate relatives. Of course, this has advantages too - and the composite can take responsibility for subtree summaries like that paragraph height.Robbegrillet
C
9

I would say that the Composite pattern as described in your link violates the Liskov substitution principle, one of the five SOLID principles.

Component has methods that only make sense for a Composite e.g. Add(). Leaf inherits from Component so it will have an Add() method like any other Component. But Leafs don't have children, so the following method call cannot return a meaningful result:

myLeaf.Add( someChild );

That call would have to throw a MethodNotSupportedException, return null or indicate in some other way to the caller that adding a child to a Leaf does not make sense.

Therefore you cannot treat a Leaf like any other Component because you'll get an exception if you try to. The Liskov substition principle states:

Let q(x) be a property provable about objects x of type T. Then q(y) should be true for objects y of type S where S is a subtype of T.

Components have the property that you can add children to them. But you cannot add children to a Leaf, even though Leaf is a subtype of Component, which violates the principle.

Cleanse answered 16/10, 2009 at 22:10 Comment(2)
A type has to deviate from the semantic interoperability with its parent type to qualify as a Liskov Substitution Principle violation, so if the Leaf type does so then it would be due to an inability to call Add() with a child and subsequently to call GetChild() with the expected index. Merely omitting behavior wouldn't qualify as a violation, however. The Add() method has no implicit or explicit contract requiring that it provide feedback that the child was added, but there is an implicit contract which assumes an added child can be retrieved by the GetChild() method.Parrotfish
BTW, the reason the Null Object pattern is effective is because it omits behavior while maintaining semantic interoperability.Parrotfish
P
4

The GoF book specifically addresses this issue on page 167.

Although the Composite class implements the Add and Remove operations for managing children, an important issue in the Composite pattern is which classes declare these operations... Should we declare these operations in the Component and make them meaningful for Leaf classes, or should we declare and define them only in Composite?

The decision involves a trade-off between safety and transparency:

  • Defining the child management interface at the root of the class hierarchy gives you transparency, because you can treat all components uniformly. It costs you safety, however, because clients may try to do meaningless things like add and remove objects from leaves.
  • Defining child management in the Composite class gives you safety, because any attempt to add or remove objects from leaves will be caught at compile-time in a statically typed language like C++. But you lose transparency, because leaves and composites have different interfaces.

We have emphasized transparency over safety in this pattern.

The last sentence is an admission that the pattern violates type-safety principles. In terms of SOLID, this would primarily be LSP, but possibly ISP as well. Note that "declaring methods which a subclass doesn't use" is an oversimplification of ISP. The real danger of those unused methods is that they will require additional dependencies which the subclass didn't need, increasing coupling between modules.

Piotrowski answered 23/10, 2018 at 15:21 Comment(1)
second option as mentioned i.e. moving child manegement methods to Composite is better and is followed by frameworks like Android (View, Viewgroup, Layouts and widgets like button, imageview etc.)Highland
S
0

Using Composite lets you to treat all objects uniformly and possibly to get rid of "instanceOf" thing which is a clear sign of a code smell. So it's respecting to LSP (Liskov's) at a first glance. However, as you differentiate common method implementations, it's likely to start to violate LSP. So, IMO, we need to keep that balance.

Storied answered 10/11, 2016 at 10:32 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.