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

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