Decorator Pattern with member functions

Question

Motivation: without SaveDecorator, we would have to write:

void Person::save (std::ostream& os) const {
    os << tag << '\n';
    saveParticular(os);
}

void Worker::save (std::ostream& os) const {
    os << tag << '\n';
    Person::saveParticular(os);
    saveParticular(os);
}

void Teacher::save (std::ostream& os) const {
    os << tag << '\n';
    Person::saveParticular(os);
    Worker::saveParticular(os);
    saveParticular(os);
}

and it is easy to forget to always add the line os << tag << '\n'; at the beginning for new derived types of Person. So SaveDecorator will decorate the save member functions, which is appearing as virtual overrides in all the derived types of Person, as follows:

#include <iostream>

template <typename T>  // Could not seem to apply pack expansion with the syntax 'void(T::*)(std::ostream&) const... fs', so this alias is apparently needed.
using memberf = void(T::*)(std::ostream&) const;

template <typename...> struct SaveDecorator;

template <>
struct SaveDecorator<> {
    template <typename T> void execute (const T*, std::ostream&) const {}  // End of recursion.
};

template <typename T, typename... Ts>
class SaveDecorator<T, Ts...> : public SaveDecorator<Ts...> {
    void(T::*func)(std::ostream&) const;
public:
    SaveDecorator (void(T::*f)(std::ostream&) const, memberf<Ts>... fs) : SaveDecorator<Ts...>(fs...), func(f) { }  
    void operator()(const T* t, std::ostream& os) const { os << T::tag << '\n';  execute(t, os); }
protected:
    void execute (const T* t, std::ostream& os) const {
        SaveDecorator<Ts...>::execute(t, os);  // More decorated lines to func that are added in the middle.
        (t->*func)(os);
    }
};

template <typename T, typename... Ts>
SaveDecorator<T, Ts...> makeSaveDecorator() {
    return SaveDecorator<T, Ts...>(&T::saveParticular, &Ts::saveParticular...);
}

class Person {
    std::string name;
public:
    static const std::string tag;
    virtual ~Person() = default;
    Person (const std::string& n) : name(n) { }
    virtual void save (std::ostream& os) const { makeSaveDecorator<Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << name << '\n'; }
};
const std::string Person::tag = "Person";

class Worker : public Person {
    int ID;
public:
    static const std::string tag;
    Worker (const std::string& name, int i) : Person(name), ID(i) { }
    virtual void save (std::ostream& os) const override { makeSaveDecorator<Worker, Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << ID << '\n'; }
};
const std::string Worker::tag = "Worker";

class Teacher : public Worker {
    std::string school;
public:
    static const std::string tag;
    Teacher (const std::string& name, int ID, const std::string& s) : Worker(name, ID), school(s) { }
    virtual void save (std::ostream& os) const override { makeSaveDecorator<Teacher, Worker, Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << school << '\n'; }
};
const std::string Teacher::tag = "Teacher";

int main() {
    Person person("Mike");
    Worker worker("Sam", 25);
    Teacher teacher("Mary", 45, "Northern CI");
    person.save(std::cout);
    worker.save(std::cout);
    teacher.save(std::cout);
}

Output:

Person
Mike
Worker
Sam
25
Teacher
Mary
45
Northern CI

Here is the decorator class rewritten as generically as I could:

#include <iostream>

template <typename R, typename T, typename... Args>  // Could not seem to apply pack expansion with the syntax 'R(Ts::*)(Args...) const... fs', so this alias is apparently needed.
using memberf = R(T::*)(Args...) const;

template <typename...> struct MemberFunctionDecorator;

template <template <typename, typename...> class Pack, typename R, typename... Args>
struct MemberFunctionDecorator<Pack<R, Args...>> {
    MemberFunctionDecorator (R(*)(Args...)) { }
    template <typename T> R execute (const T*, Args&&...) const {}  // End of recursion.
};

template <template <typename, typename...> class Pack, typename R, typename... Args, typename T, typename... Ts>
class MemberFunctionDecorator<Pack<R, Args...>, T, Ts...> : public MemberFunctionDecorator<Pack<R, Args...>, Ts...> {
    R(*argFunc)(Args...);
    R(T::*func)(Args...) const;
public:
    MemberFunctionDecorator (R(*af)(Args...), R(T::*f)(Args...) const, memberf<R, Ts, Args...>... fs) : MemberFunctionDecorator<Pack<R, Args...>, Ts...>(af, fs...), argFunc(af), func(f) { }
    R operator()(const T* t, Args&&... args) const { (*argFunc)(std::forward<Args>(args)...);  return execute(t, std::forward<Args>(args)...); } 
protected:
    R execute (const T* t, Args&&... args) const {
        MemberFunctionDecorator<Pack<R, Args...>, Ts...>::execute(t, std::forward<Args>(args)...);  // More decorated lines to func that are added in the middle.
        return (t->*func)(std::forward<Args>(args)...);
    }
};

// Now illustrating the usage of MemberFunctionDecorator.
template <typename ReturnType, typename... Args> struct ArgPack;

template <typename T> void f(std::ostream& os) { os << T::tag << '\n'; }

template <typename T, typename... Ts>
MemberFunctionDecorator<ArgPack<void, std::ostream&>, T, Ts...> saveDecorator() {  // Now we pass the particular member functions, including the std::ostream& argument and void return type.
    return MemberFunctionDecorator<ArgPack<void, std::ostream&>, T, Ts...>(&f<T>, &T::saveParticular, &Ts::saveParticular...);  // Cannot pass a (template) lambda function into the first argument.
}

class Person {
    std::string name;
public:
    static const std::string tag;
    virtual ~Person() = default;
    Person (const std::string& n) : name(n) { }
    virtual void save (std::ostream& os) const { saveDecorator<Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << name << '\n'; }
};
const std::string Person::tag = "Person";

class Worker : public Person {
    int ID;
public:
    static const std::string tag;
    Worker (const std::string& name, int i) : Person(name), ID(i) { }
    virtual void save (std::ostream& os) const override { saveDecorator<Worker, Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << ID << '\n'; }
};
const std::string Worker::tag = "Worker";

class Teacher : public Worker {
    std::string school;
public:
    static const std::string tag;
    Teacher (const std::string& name, int ID, const std::string& s) : Worker(name, ID), school(s) { }
    virtual void save (std::ostream& os) const override { saveDecorator<Teacher, Worker, Person>()(this, os); }
    void saveParticular (std::ostream& os) const { os << school << '\n'; }
};
const std::string Teacher::tag = "Teacher";

int main() {
    Person person("Mike");
    Worker worker("Sam", 25);
    Teacher teacher("Mary", 45, "Northern CI");
    person.save(std::cout);
    worker.save(std::cout);
    teacher.save(std::cout);
}

Answer 1

Without knowing the context, it’s impossible to say whether this design is a good idea. It seems over-engineered, but…. 🤷🏼

I will point out that you could get the same effect much, much easier, by simply inverting which function is virtual, and which is not. Instead of save() being virtual and saveParticular() being non-virtual, do it the other way around. This idiom is called the non-virtual interface idiom.

class Person {
    std::string name;
public:
    virtual ~Person() = default;
    Person (const std::string& n) : name(n) { }

    // note: not virtual
    void save (std::ostream& os) const
    {
        os << get_tag() << '\n';
        saveParticular(os);
    }

    // need this, too; must be virtual but doesn't need to be public.
    virtual std::string get_tag() const { return "Person"; }

protected:
    // note: virtual, and protected
    virtual void saveParticular (std::ostream& os) const { os << name << '\n'; }
};

class Worker : public Person {
    int ID;
public:
    Worker (const std::string& name, int i) : Person(name), ID(i) { }

    std::string get_tag() const override { return "Worker"; }

protected:
    void saveParticular (std::ostream& os) const override
    {
        Person::saveParticular(os);
        os << ID << '\n';
    }
};

class Teacher : public Worker {
    std::string school;
public:
    Teacher (const std::string& name, int ID, const std::string& s) : Worker(name, ID), school(s) { }

    std::string get_tag() const override { return "Teacher"; }

protected:
    void saveParticular (std::ostream& os) const override
    {
        Teacher::saveParticular(os);
        os << school << '\n';
    }
};

Compared to your design, there are no drawbacks. Any drawbacks already have parallels:

• If you forget to implement get_tag(), then you’ll get the parent’s tag. Same thing would happen if you forget to add the static tag member in your version.
• If you forget to implement saveParticular(), then you’ll get the parent’s behaviour. Same thing would happen if you forgot to implement save() with your version.
• If you do a sloppy copy-paste and forget to change the parent class name in saveParticular(), you’ll get the parent’s behaviour again. Same thing would happen if you do a sloppy copy-paste and forget to add the current class name to the makeSaveDecorator() list.)

The benefits compared to your design, however, are numerous.

First: simplicity. You won’t go cross-eyed trying to mentally parse recursive parameter pack expansions, or aliases-of-pointers-to-member-functions. I shudder to think of the error messages you might get if you mistype something somewhere in your version.

Second: robustness. The big problem with your design—the thing that would make me veto ever using it in any of my projects—is the need to list the inheritance chain:

    makeSaveDecorator<Teacher, Worker, Person>()(...);

The number of ways this can break silently is impressive. You could misorder the classes:

    makeSaveDecorator<Worker, Teacher, Person>()(...);

You could forget a step in the hierarchy:

    makeSaveDecorator<Teacher, Person>()(...);

You could insert random classes into the mix for giggles:

    makeSaveDecorator<Teacher, Ninja, Person>()(...);

Or—and this is the worst part, because it means spooky-action-at-a-distance behavioural changes—if anyone ever refactored the class hierarchy, the list may no longer reflect the actual hierarchy. Things would continue to “work”, even though it is no longer correct behaviour.

Third: efficiency. For build efficiency, it’s a no-brainer. There are exactly 3 class instantiations in my version. Do you even know how many there are in yours? I can’t be bothered to even try to count. For run-time efficiency, you might assume the fact that my version involves two dynamic dispatches (one for saveParticular(), plus one for get_tag()) means it’s the obvious loser… but yours has a virtual call… multiple pointer calls (to func in SaveDecorator, recursively), recursive calls (so more stack space is needed), and much, much more. (Plus, because my design is so simple, it will be very easy for the compiler to de-virtualize whenever possible.)

(Note that if you moved beyond C++11… which you should; that version is over a decade old… you could probably take advantage of C++17’s fold expressions. Those would probably simplify things enormously, and eliminate the need for recursive template expansions. With that, the build-time efficiency of your version might get much closer to mine… but still won’t be able to match it.)

As for the generic version… if something isn’t a good solution when done specifically, it certainly isn’t going to be a good solution when implemented generically.

For me, the bottom line is that the NVI idiom solves the main problem you are trying to solve—making sure that tag line is always printed first—and it does so with orders of magnitude more simplicity. The fact that I wouldn’t have to manually list the inheritance chain seals the deal for me. The improved compile times, and better run-time performance are just icing on the cake.

But like I said in the beginning, you haven’t given any real context. Your solution may be the most brilliant solution ultimately possible… or it may be the most ridiculous code ever written. It is impossible to know without knowing what the real intended usage is.

However, if the intended usage really is just to write a standard preamble before polymorphic, class-specific output… then NVI is probably the way to go.