2

In my project I have several "basic" interfaces whose behavior is fixed, i.e. the default implementation will always be good for every puropse. So I defined them as concrete classes with data members, Configurable, Named, DataUser etc., e.g.:

class Configurable {
public:
  void SetParameter(int i) { _prop = i; }
  int GetParameter() { return _prop; }
private:
  int _prop;
};

For complex classes which I want to manipulate also using the basic interfaces I define a multiple inheritance:

class Algorithm: public Configurable, public Named, public DataUser {
public:
  virtual ~Algorithm() = default;
  virtual void Process() = 0;
};

Now I have the need to decorate a complex class, e.g.:

class AlgorithmDecorator: public Algorithm{
public:
  AlgorithmDecorator(std::unique_ptr<Algorithm> &&algo): _algo{std::move(algo)} {}
  void Process() { _algo->Process(); /* decoration code here */}
protected:
  std::unique_ptr<Algorithm> _algo;
};

and in doing this I'm not interested in decorating the behavior of the basic classes. But obviously doing like above any call to a method of a basic class for a decorator object will work on the basic class representation of the decorator, not that of _algo, e.g.:

class ConcreteAlgorithm: public Algorithm {
public:
  void Process() { std::cout << GetParameter() << std::endl; }
}

int main() {
  std::unique_ptr<algorithm> algo = std::make_unique<ConcreteAlgorithm>();
  algo = std::make_unique<AlgorithmDecorator>(std::move(algo));
  algo->SetParameter(5);
  algo->Process();
  return 0;
}

The actual output is 0, since the parameter value is set for the decorator instance and not for the wrapped algorithm. This could be solved by making Configurable::SetParameter virtual and overriding it in AlgorithmDecorator setting the parameter value both for the decorator instance and for _algo, but then these two must be kept in sync for consistency. Having two representations playing here is definitely a major problem.

This "double representation" creates several problems other than this, so I ended up with a single, shared representation implementation of Configurable:

#include <iostream>
#include <memory>

class Configurable {
public:
  Configurable() : _repr{std::make_shared<Representation>()} {}
  void SetParameter(int i) { _repr->_prop = i; }
  int GetParameter() { return _repr->_prop; }

protected:
  class Representation {
    friend class Configurable;
    int _prop;
  };
  Configurable(const std::shared_ptr<Representation> &repr) : _repr{repr} {}
  std::shared_ptr<Representation> &GetRepresentation() { return _repr; }

private:
  std::shared_ptr<Representation> _repr;
};

class AlgorithmDecorator;
class Algorithm : public Configurable {
public:
  Algorithm() {}
  virtual ~Algorithm() = default;
  virtual void Process() = 0;

protected:
  friend AlgorithmDecorator;
  Algorithm(std::shared_ptr<Configurable::Representation> &repr) : Configurable(repr) {}
};

class AlgorithmDecorator : public Algorithm {
public:
  AlgorithmDecorator(std::unique_ptr<Algorithm> &&algo)
      : Algorithm(algo->Configurable::GetRepresentation()), _algo{std::move(algo)} {}
  void Process() { _algo->Process(); std::cout << "Decorated" << std::endl; }

protected:
  std::unique_ptr<Algorithm> _algo;
};

class ConcreteAlgorithm : public Algorithm {
public:
  void Process() { std::cout << GetParameter() << std::endl; }
};

int main() {
  std::unique_ptr<Algorithm> algo = std::make_unique<ConcreteAlgorithm>();
  algo = std::make_unique<AlgorithmDecorator>(std::move(algo));
  algo->SetParameter(5);
  algo->Process();
  return 0;
}

This works, but seems awkward and scales quite poorly when inheriting Algorithm form several basic classes.

All of this makes me think about a bad design, but I can't come up with a better solution that:

  1. does not force me to duplicate the default implementation of basic classes in every complex class (Algorithm is just one of the many complex classes in my project);
  2. makes the decorator work as expected, i.e. just add functionalities with no need to care about the underlying representation.

Any hint about how to improve this or on a different pattern that might do the job better is greatly appreciated.

8
  • 2
    Assuming that the Algorithm-specific interface is enough for the client code, make Algorithm pure virtual, and don't inherit the other classes (Configurable, Named, DataUser); then create a AlgorithmBase that inherits all of them (Algorithm, Configurable, Named, DataUser), and derive your ConcreteAlgorithm(s) from that. Derive decorators from the bare bones pure virtual Algorithm only, then compose the decorated object and use in clients through the Algorithm interface. Nov 10, 2023 at 11:11
  • This would mean to implement in the decorator class the redirection to all the methods of the basic objects in the composition, right? I initially discarded this option since it requires a lot of boilerplate work plus also splitting all of the basic classes in an interface plus a base concrete implementation. But maybe it's the most architecturally correct way to obtain the desired behavior... I'll give it a try, thanks for the suggestion! Nov 10, 2023 at 16:01
  • No, what I meant by "Assuming that the Algorithm-specific interface is enough for the client code" was: if it's the case that at the call site (where you use the decorated object) you don't need any methods other than those defined on the bare-bones Algorithm interface (in the scenario I proposed based on your example, this would just be the Process() method), then Algorithm as I suggested it serves to segregate a subset of the full interface, and then you can build a decorator over that subset (no need to redirect or expose any other methods). 1/2 Nov 10, 2023 at 17:36
  • 1
    Yes, the decorator is meant to be a transparent wrapper over some other interface, but following the Interface Segregation Principle (ISP), you can have different "views" of your class in different clients (you don't have to expose the same set of methods to every consumer). If at the place where you need to decorate the class you only require a narrower interface, segregate that out, and only decorate that - that's a perfectly valid Decorator pattern implementation. Now, I don't know enough about your specific use case, so this might not work for you, but it's an approach to consider. 2/2 Nov 10, 2023 at 17:37
  • 1
    @FilipMilovanović: I think there is enough content in your comments for writing an answer.
    – Doc Brown
    Nov 11, 2023 at 8:31

1 Answer 1

4

If applying design patterns, avoid shortcuts

Your decorator is an incomplete one:

  • The inheritance to the decorated class (Component in GoF terminology) is not for reuse its implementation, but to ensure compliance with its interface and substitutability.
  • The implementation should forward all the operations to the decorated object. Reusing the operation without forwarding is a shortcut that may in some cases work, but in most of the cases not, as your example shows.

GoF recommends therefore in particular:

Components and decorations must descend from a common Component class. It's important to keek this common class lightweight; that is, it should focus on defining an interface, not on storing the data. The definition of the data representation should be deferred to the subclasses.

This is exactly what the inheritance of Configurable breaks.

Make your design work

The shared representation is a way to solve it. A better way imho, would be to make Configurable abstract and without data, and use a ConcreteConfigurable as a strategy for implementing the Configurable in you concrete algorithm (i.e. just forward the calls). The decorator would then use as strategy the algorithm it decorates, which is also a Configurable and is then guaranteed to use the same strategy with less boilerplate code (see quick attempt, but needs refinement).

A pragmatic workaround in your case, would be to make the operation of Configurable virtual and forward them in the decorator. You'd then have a useless representation, but your decorator decorates correctly:

class AlgorithmDecorator: public Algorithm{
public:
  AlgorithmDecorator(std::unique_ptr<Algorithm> &&algo): _algo{std::move(algo)} {}
  void Process() { _algo->Process(); /* decoration code here */}
  void SetParameter(int i) override { _algo->SetParameter(i); }
  int GetParameter() override { return _algo->GetParameter(); }
protected:
  std::unique_ptr<Algorithm> _algo;
};

Challenge

But is the decorator is really the best way for what you want to achieve? If it's only about adding a couple of steps to the algorithm, I wonder if an implementation of the template method pattern would not lead to a simpler design:

class Algorithm: public Configurable, public Named, public DataUser {
public:
  virtual ~Algorithm() = default;
  virtual void doBefore() {}
  virtual void doAfter() {}
  virtual void doProcess() = 0; 
  virtual void process() {
      doBefore(); 
      doProcess(); 
      doAfter();  
  }
};

class MyAlgorithm : public Algorithm {
public :
    void doProcess() override { std:cout<<"Do smothing"<<std::endl; }
};

class MyDecoratedAlgorith : public MyAlgorithm {
public :
    void doBefore() override { std:cout<<"Prepare more"<<std::endl; }
    void doAfter() override { std:cout<<"Do smothing more"<<std::endl; }
};
1
  • Thanks for your very detailed and complete answer. I'd need to take some time to go through it and s evaluate the applicability to my real case (which is more complex than that of the example). About the last point, I think I need the decorator since I want to be able to add behavior incrementally at runtime. But template methods and the NVI idiom sounds quite good to me so I'll see if I can go that way. Nov 13, 2023 at 16:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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