views:

235

answers:

2

Situation

I want to implement the Composite pattern:

class Animal
{
public:
    virtual void Run() = 0;
    virtual void Eat(const std::string & food) = 0;
    virtual ~Animal(){}
};

class Human : public Animal
{
public:
    void Run(){ std::cout << "Hey Guys I'm Running!" << std::endl; }
    void Eat(const std::string & food)
    {
        std::cout << "I am eating " << food << "; Yummy!" << std::endl;
    }
};

class Horse : public Animal
{
public:
    void Run(){ std::cout << "I am running real fast!" << std::endl; }
    void Eat(const std::string & food)
    {
        std::cout << "Meah!! " << food << ", Meah!!" << std::endl;
    }
};

class CompositeAnimal : public Animal
{
public:
    void Run()
    {
        for(std::vector<Animal *>::iterator i = animals.begin();
            i != animals.end(); ++i)
        {
            (*i)->Run();
        }
    }

    // It's not DRY. yuck!
    void Eat(const std::string & food)
    {
        for(std::vector<Animal *>::iterator i = animals.begin();
            i != animals.end(); ++i)
        {
            (*i)->Eat(food);
        }
    }

    void Add(Animal * animal)
    {
        animals.push_back(animal);
    }

private:
    std::vector<Animal *> animals;
};

The Problem

You see, for my simple requirement of the composite pattern, I end up writing a lot of the same repeating code iterating over the same array.

Possible solution with macros

#define COMPOSITE_ANIMAL_DELEGATE(_methodName, _paramArgs, _callArgs)\
    void _methodName _paramArgs                                      \
    {                                                                \
        for(std::vector<Animal *>::iterator i = animals.begin();     \
            i != animals.end(); ++i)                                 \
        {                                                            \
            (*i)->_methodName _callArgs;                             \
        }                                                            \
    }

Now I can use it like this:

class CompositeAnimal : public Animal
{
public:
    // It "seems" DRY. Cool

    COMPOSITE_ANIMAL_DELEGATE(Run, (), ())
    COMPOSITE_ANIMAL_DELEGATE(Eat, (const std::string & food), (food))

    void Add(Animal * animal)
    {
        animals.push_back(animal);
    }

private:
    std::vector<Animal *> animals
};

The question

Is there a way to do it "cleaner" with C++ meta-programming?

The harder question

std::for_each has been suggested as a solution. I think our problem here is a specific case of the more general question, let's consider our new macro:

#define LOGGED_COMPOSITE_ANIMAL_DELEGATE(_methodName, _paramArgs, _callArgs)\
    void _methodName _paramArgs                                      \
    {                                                                \
        log << "Iterating over " << animals.size() << " animals";    \
        for(std::vector<Animal *>::iterator i = animals.begin();     \
            i != animals.end(); ++i)                                 \
        {                                                            \
            (*i)->_methodName _callArgs;                             \
        }                                                            \
        log << "Done"                                                \
    }

Looks like this can't be replaced by for_each

Aftermath

Looking at GMan's excellent answer, this part of C++ is definitely non-trivial. Personally, if we just want to reduce the amount of boilerplate code, I think macros probably is the right tool for the job for this particular situation.

GMan suggested std::mem_fun and std::bind2nd to return functors. Unfortunately, this API doesn't support 3 parameters (I can't believe something like this got released into the STL).

For illustrative purpose, here're the delegate functions using boost::bind instead:

void Run()
{
    for_each(boost::bind(&Animal::Run, _1));
}

void Eat(const std::string & food)
{
    for_each(boost::bind(&Animal::Eat, _1, food));
}
+10  A: 

I'm not sure I really see the problem, per se. Why not something like:

void Run()
{
    std::for_each(animals.begin(), animals.end(),
                    std::mem_fun(&Animal::Run));
}

void Eat(const std::string & food)
{
    std::for_each(animals.begin(), animals.end(),
                    std::bind2nd(std::mem_fun(&Animal::Eat), food));
}

Not too bad.


If you really wanted to get rid of the (small) boilerplate code, add:

template <typename Func>
void for_each(Func func)
{
    std::for_each(animals.begin(), animals.end(), func);
}

As a private utility member, then use that:

void Run()
{
    for_each(std::mem_fun(&Animal::Run));
}

void Eat(const std::string & food)
{
    for_each(std::bind2nd(std::mem_fun(&Animal::Eat), food));
}

A bit more concise. No need for meta-programming.

In fact, meta-programming will ultimately fail. You're trying to generate functions, which are defined textually. Meta-programming cannot generate text, so you'll inevitably use a macro somewhere to generate text.

At the next level, you'd write the function then try to take out the boilerplate code. std::for_each does this quite well. And of course as has been demonstrated, if you find that to be too much repetition, just factor that out as well.


In response to the LoggedCompositeAnimal example in the comment, your best bet is to make something akin to:

class log_action
{
public:
    // could also take the stream to output to
    log_action(const std::string& pMessage) :
    mMessage(pMessage),
    mTime(std::clock())
    {
        std::cout << "Ready to call " << pMessage << std::endl;
    }

    ~log_action(void)
    {
        const std::clock_t endTime = std::clock();

        std::cout << "Done calling " << pMessage << std::endl;
        std::cout << "Spent time: " << ((endTime - mTime) / CLOCKS_PER_SEC)
                    << " seconds." << std::endl;
    }

private:
    std::string mMessage;
    std::clock_t mTime;
};

Which just mostly automatically logs actions. Then:

class LoggedCompositeAnimal : public CompositeAnimal
{
public:
    void Run()
    {
        log_action log(compose_message("Run"));
        CompositeAnimal::Run();
    }

    void Eat(const std::string & food)
    {
        log_action log(compose_message("Eat"));
        CompositeAnimal::Eat(food);
    }

private:
    const std::string compose_message(const std::string& pAction)
    {
        return pAction + " on " +
                    lexical_cast<std::string>(animals.size()) + " animals.";
    }
};

Like that. Information on lexical_cast.

GMan
`std::bind2nd` is quite new to me. What if I have 3 parameters?
kizzx2
I think the loop iteration here is a specific case, my bad for the poor example. Let's imagine we have a `LoggedCompositeAnimal` which logs all operations _before_ and _after_ iterating ("Ready to call Run on 3 animals...", "Done calling run on 3 animals. Spent time 50 seconds").
kizzx2
@kizzx2: One is the limit, actually. :) This portion of the standard library reeks of terrible design. If you want to get serious about binding stuff, look into [`Boost.Bind`](http://www.boost.org/doc/libs/1_43_0/libs/bind/bind.html). It has a much more simple, expressive, and powerful interface. (Otherwise, you have to manually make functors.) And at your second comment, there isn't much to do there either but make re-usable code. I'll add in an example to see if we're on the same page.
GMan
then just use boost::bind
vividos
@GMan: The addition was some serious kung-fu! This is pretty close to an ideal solution (I'll look into boost::bind). I suppose the "Then change `for_each`" part would reside in `LoggedCompositeAnimal.cpp` and will not change `for_each` for everybody else, right? (All this makes me wonder if "just use the macro" would be the "correct" solution in this case)
kizzx2
@kizz: Sorry, I was treating `LoggedCompositeAnimal` as our only composite class. I've edited to reflect how I would implement it with `CompositeAnimal` as the base.
GMan
@kizzx2: Whenever you design a piece of code don't just blindlessly follow design advices, but focus on the real objective. DRY is not about typing less, but about making code reusable and easier to maintain. Adding macros to the mix will actually worsen the maintainability of the code, so adding macros to make it DRY-er stride you from the real objective. There are different things that could theoretically be added to the last part here, like using CRTP and wrapping functors to reduce the actual code, but none of them is going to really improve the code.
David Rodríguez - dribeas
@dribeas: Could you be more specific about how to "actually improve the code"? Yes, DRYer isn't always better, but most of the time it is (OK, flame start! :p). Using our example, there are cases where each function of the `Composite` shouldn't just blindly delegate to the child components. But in *this* case, that's exactly what I need, so I'm looking for cleverer ways to do it. Of course, you could say the language is designed to work generic situations and there doesn't exist an optimal solution for every special case. This is purely for education purpose.
kizzx2
@kizz: I think what he's trying to say is don't repeat DRY as a mantra for the sake of it. Understand the problem DRY tries to solve, and apply it if that problem comes up, not out of the blue. The problem DRY solves is harder maintenance and wasting time. So if you find you could save time and improve maintenance by re-using code, do so. But basically making a macro as a different way of saving a few keystrokes misses the point. It's not about keystrokes, it's about content. Granted, your original macros helped, but other generic functionality already exists.
GMan
I'm not really sure why the emphasis is placed so much on "saving keystrokes." Baring corner cases, most of the time "saving keystrokes" means reducing maintenance efforts and time. It might be assumed that all the macro did here is iterate the vector, but what if I want something like `LoggedCompositeAnimal`? Using macros (or meta-programming), I change that in one place, which is by definition less prone to error. If I later figure that each function is in fact special and can't be DRYed up in a macro, I would of course expand it instead of stretching the macro's limit.
kizzx2
With that aside, I do agree that DRYer isn't always the better -- learned it the hard way from over-architecturing. It's surprisingly _more_ difficult to "simplify architecture" than to "apply more architecture."
kizzx2
+2  A: 

You could make functors instead of methods:

struct Run
{
    void operator()(Animal * a)
    {
        a->Run();
    }
};

struct Eat
{
    std::string food;
    Eat(const std::string& food) : food(food) {}

    void operator()(Animal * a)
    {
        a->Eat(food);
    }
};

And add CompositeAnimal::apply (#include <algorithm>):

template <typename Func>
void apply(Func& f)
{
    std::for_each(animals.begin(), animals.end(), f);
}

Then your code would work like this:

int main()
{
    CompositeAnimal ca;
    ca.Add(new Horse());
    ca.Add(new Human());

    Run r;
    ca.apply(r);

    Eat e("dinner");
    ca.apply(e);
}

Output:

> ./x
I am running real fast!
Hey Guys I'm Running!
Meah!! dinner, Meah!!
I am eating dinner; Yummy!

To keep the interface consistent, you could go one step further.

Rename struct Run to Running and struct Eat to Eating to prevent method/struct clash.

Then CompositeAnimal::Run would look like this, using the apply method and the struct Running:

void Run()
{
    Running r;
    apply(r);
}

And similarly CompositeAnimal::Eat:

void Eat(const std::string & food)
{
    Eating e(food);
    apply(e);
}

And you can call now:

ca.Run();
ca.Eat("dinner");

output still the same:

I am running real fast!
Hey Guys I'm Running!
Meah!! dinner, Meah!!
I am eating dinner; Yummy!
stefanB
Thanks for the info on Functors! A potential problem with this approach, though, is that it breaks the Composite Pattern. My callers shouldn't know that the `Animal` I give is actually a `Composite`. So my caller shouldn't have to know to call `apply`, he should just call `Eat` and `Run` directly.
kizzx2
I noticed, hence a small fix :)
stefanB
You could add 'template<class T> void operator->*(T t) { apply(t); }`and then even write `ca->*Run(); ca->*Eat("pudding");`:-)
Luther Blissett
Looking closely, I'm not sure how the Functor approach solves the original premise (DRY). Since I need to create new methods for each and then just delegate to the Functor, whose sole function is to provide the functionality of `Run` and `Eat`, why don't I just write the function body inside `Run` and `Eat` but have to delegate to a functor?
kizzx2
It depends on your design. You could just provide `apply` and functors. Then the only difference would be that on Compound class `apply` would apply it to all the objects. You could add new functionality by adding new functors.
stefanB