C++ : A response to functors

I received an interesting and detailed response to my posts on the uses of std::for_each from my friend Jun. Before we dive in I'd like to point out that I find it interesting that there seems to be an infinite number of ways of solving the same problem in C++! If you've ever used C# you'll know that the for(object in IEnumerable) syntax is part of the language. However this is a very different language despite the syntatical similarities and it doesn't do to pit one language against another ... they all have their uses.

Soapbox time over, let's get on with it!

Jun writes...

The thing I don't like about functors (function objects) is that they can clutter the code and they obscure the flow of the logic. Originally it is clear what you want - I only have to look at what you say is the ugly code (void B::Update()).

But I don't agree that the code has become easier to read or understand (i.e. maintain) toward the end of your article.

At the start it is clear that B has a container which is used to steer its controls.


In the end this simple piece of code is stretched out over a class A_UpdateHandler (it's Update function), B::Init() function and of course the B::Update() function. Undoubtfully the Init(), Update() and the A_UpdateHandler will end up apart from each other in the source file and it will become quite tricky for others (including yourself in a couple of months time) to piece out the flow of the code.

What happens when elements are added to or removed from m_APCollection after Init() has been called?


The design in the end is a workaround for the fact that C++ doesn't support Lambda calculus. If you look at it - the original Update() function is clear, but to be able to use std::for_each you had to create a functor and use a std::bind2nd with a std::mem_fun_ref which makes the end result more complex than what you originally started with.

I can imagine that if someone else would have to make a change to this code and allow for changes to be made to m_APCollection at runtime - (s)he could be in for a bind.


Everything in C++ is solvable by introducing an extra layer of indirection. But by doing so, simple things can be lost in the translation.

If you would use pseudo code to describe your algorithm it would be something like:

for a in m_APCollection do
    control = GetControlByName ( a.GetName( ) )
    if ( control ) then
        graphic.doSomething( control.GetCurrentValue( ) 
                             > a.GetThreshhold( ) )
    end
end

Now wouldn't it be nice to be able to stick as close to this as possible?

You definitely have to read FOREACH Redux by Niebler!
check this out:

BOOST_FOREACH ( char c, "Hello World" )
{
    std::cout << ch;
}

or even

BOOST_FOREACH ( A *a, m_APCollection )
{
    std::string const &name=a->GetName( );
    Control *control = GetControlByName( name ); 
    // can return NULL
    if ( control )
    {
        bool value = control->GetCurrentValue( ) 
                     > a->GetThreshold( );
        graphic.doSomething( value );
    }
}

Sticks pretty close to what you originally intended? Then in turn it
is not that different from what you considered to be ugly code


Now I am curious if there is a way to force std::for_each in without having to write a wrapper and having to use "bind" and "mem_fun_ref".


How about this sneaky way to get access to that private function(compiles with the .NET C++ compiler - I haven't checked with GNU or others). No need to make changes to the interface and this is the update function:

void B::Update( )
{
    Graphic const &graphic = GetGraphic( );
    DoSomethingWithA functor(graphic, *this, 
                             &B::GetControlByName);
    std::for_each(mAPCollection.begin( ), 
                  mAPCollection.end( ), functor);
}

(*disclaimer - I am not promoting this and any loss or
damage resulting from this code is your own responsibility!


Jun


So I agree with all of what is said here and have taken it on board. My main mistake in this case was the desire to use a tool that was unsuited to the task in hand. Having said that I learnt a lot so it is always recommended to try these things out! I actually managed to use a more straight forward functor without the pointer to a private member function hack to solve a similar problem after this and I think it made for much better code. Of the two solutions offered the BOOST_FOREACH is the most robust and readable. The Niebler article is well worth a read. Many good generic tricks are introduced in an interesting context and I learnt a lot. Especially the ScopeGuard trick from Alexandrescu ... but that's a topic for another post. The full code for Jun's own solution is shown below and I think you'll agree too that it is more readable and hence easier to maintain. The intention is clear and as it says in The Pragmatic Programmer "Program close to the Problem Domain".

#ifndef __B__H
#define __B__H

#include 
#include 

class A
{
public:
    std::string const& GetName() const;
    int GetThreshold() const;
};

struct Control { int GetCurrentValue() const { return 11; } };
struct Graphic { void DoSomething(bool) const {} };

class B
{
public:
    void Update();
private:
    Control const* GetControlByName(
                   std::string const &name) const;
    Graphic const& GetGraphic() const;

    std::vector mAPCollection;
};

#endif // __B__H

// b.cpp
#include "b.h"
#include 

using namespace std;

string const& A::GetName() const
{
    static string const name("Knanshon");
    return name;
}

int A::GetThreshold() const { return 42; }

//--------------------------------------

namespace
{

    class DoSomethingWithA
    {
        Graphic const &mGraphic;
        B &mB;
        typedef Control const* (B::*GetCtrl)
                               (std::string const&) const;
        GetCtrl  mFoo;
    public:
        DoSomethingWithA(Graphic const &graphic, B &b, 
                         GetCtrl foo)
            : mGraphic(graphic)
            , mB(b)
            , mFoo(foo)
        {
        }

        void operator()(A *a)
        {
            string const &name=a->GetName();
            Control const *ctrl=(mB.*mFoo)(name); 
            // can return NULL
            if ( ctrl )
            {
                bool value = ctrl->GetCurrentValue() > 
                             a->GetThreshold();
                mGraphic.DoSomething(value);
            }
        }
    };

} // anonymous namespace
    
//---------------------------------------------------

Control const* B::GetControlByName(
    string const &name) const
{
    return 0;
}

Graphic const& B::GetGraphic() const
{
    static Graphic const g;
    return g;
}

void B::Update()
{
    Graphic const &graphic=GetGraphic();
    DoSomethingWithA functor(graphic, *this, 
                             &B::GetControlByName);
    std::for_each(mAPCollection.begin(), 
                  mAPCollection.end(), functor);
}

// eof