Delegate for GUI project

Question

I wrote simple delegate in C++11 for my GUI project. I think that some parts can be optimized or cleaned.

#ifndef DELEGATE_H
#define DELEGATE_H

//Container interface
template<typename... Args>
class IContainer
{
public:
    virtual void Call(Args...) {}
    virtual ~IContainer() {}
    IContainer<Args...> *next;
};

//Container realization
template< typename T, typename M, typename... Args > class MContainer : public IContainer<Args...>
{
public:
    MContainer( T* c, M m ) : mClass( c ), mMethod( m ) {}
    void Call(Args... args)
    {
        (mClass->*mMethod)( args... );
    }

private:
    T *mClass;
    M mMethod;
};

template< typename M, typename... Args > class FContainer : public IContainer<Args...>
{
public:
    FContainer( M m ) : mMethod( m ) {}
    void Call(Args... args)
    {
        (mMethod)( args... );
    }

private:
    M mMethod;
};

//Delegate
template<typename... Args>
class Delegate
{
public:
    Delegate()
    {
        mContainerHead = new IContainer<Args...>();
        mContainerTail = mContainerHead;
        mContainerHead->next = 0;
    }
    ~Delegate()
    {
        IContainer<Args...> *container = mContainerHead;
        while(container)
        {
            IContainer<Args...> *temp = container->next;
            delete container;
            container = temp;
        }
    }

    void Clear()
    {
        IContainer<Args...> *container = mContainerHead->next;
        while(container)
        {
            IContainer<Args...> *temp = container->next;
            delete container;
            container = temp;
        }
        mContainerHead->next = 0;
        mContainerTail = mContainerHead;
    }

    template<typename T, typename M>
    void Connect(T *c, M m)
    {
        mContainerTail->next = new MContainer< T, M, Args... >(c,m);
        mContainerTail->next->next = 0;
        mContainerTail = mContainerTail->next;
    }

    template<typename M>
    void Connect(M m)
    {
        mContainerTail->next = new FContainer< M, Args... >(m);
        mContainerTail->next->next = 0;
        mContainerTail = mContainerTail->next;
    }

    template<typename T, typename M>
    void Disconnect(T *c, M m)
    {
        IContainer<Args...> *container = mContainerHead;
        while(container->next)
        {
            MContainer<T, M, Args...> *temp = dynamic_cast< MContainer<T, M, Args...>* >(container->next);

            if(temp)
            {
                if(container->next == mContainerTail)
                {
                    mContainerTail = container;
                }
                container->next = container->next->next;
                delete temp;
                break;
            }
            container = container->next;
        }
    }

    template<typename M>
    void Disconnect(M m)
    {
        IContainer<Args...> *container = mContainerHead;
        while(container->next)
        {
            FContainer<M, Args...> *temp = dynamic_cast< FContainer<M, Args...>* >(container->next);

            if(temp)
            {
                if(container->next == mContainerTail)
                {
                    mContainerTail = container;
                }
                container->next = container->next->next;
                delete temp;
                break;
            }
            container = container->next;
        }
    }

    void operator ()(Args... args)
    {
        Call(args...);
    }

    void Call(Args... args)
    {
        IContainer<Args...> *container = mContainerHead;
        while(container)
        {
            container->Call(args...);
            container = container->next;
        }
    }
private:
    IContainer<Args...> *mContainerHead;
    IContainer<Args...> *mContainerTail;
};

#endif // DELEGATE_H

Using example:

struct TestClass1
{
    void hello(int a, int b)
    {
        std::cout << a << " + " << b << " = " << a+b << std::endl;
    };
};

struct TestClass2
{
    void hello(int a, int b)
    {
        std::cout << a << " - " << b << " = " << a-b << std::endl;
    };
};

struct TestClass3
{
    void hello(int a, int b)
    {
        std::cout << a << " * " << b << " = " << a*b << std::endl;
    };
};

int main()
{
    TestClass1 t1;
    TestClass2 t2;
    TestClass3 t3;
    Delegate<int, int> d;
    d.Connect(&t1, &TestClass1::hello);
    d.Connect(&t2, &TestClass2::hello);
    d.Connect(&t3, &TestClass3::hello);
    d.Call(10,5);

    d.Disconnect(&t2, &TestClass2::hello);

    return 0;
}

Answer 1

IContainer

In the class IContainer the member next is public. Which leads to potential incorrect useage.

Non private members is a code smell they should be hidden behind access methods (not get/set but more like chain()). Also it would be easier if you used smart pointers to make sure that everything is correctly cleaned up.

Delegate

The Delegate class has owned raw pointers and does not follow the rule of five (google rule of three as it is more common but the rule of three was expanded to the rule of five with C++11).

You can fix this be either implementing (or disabling) all the compiler generated methods. Or converting your owned raw pointers into smart pointers. My choice would be to convert them into smart pointers as correctly handing multiple RAW pointers with exceptions propagating is non trivial (a class with multiple owned RAW pointers is a Code smell even if you do implement the rule of five).

The code ~Delegate() and Clear() is very similar (ie code duplication). You should refactor this to move common code to a single function. If the code changes you then only have a single location that will need fixing.

In the method Conect() a lot of work is done that is usually associated with the constructor of the object. If you give IConnect an appropriate constructor this code becomes highly simplified and easier to follow.

In the method Disconnect()

    IContainer<Args...> *container = mContainerHead;
    while(container->next)
    {
        // STUFF
        container = container->next;
    }

This looks more like a for(;;) loop.

    for(IContainer<Args...> *container = mContainerHead; container->next; container = container->next)
    {
        // STUFF
    }

I think this does not read as well as it could.

            container->next = container->next->next;

            // I would have done:
            container->next = temp->next;

There is no indication that anything was removed from the list. There is also the possibility that might be multiple objects in the list that match the input requirements. So even if you remove one item this does not mean there does not exist an item with the same properties.

Here you have two options: 1) Add another bool parameter (that defaults to false) to indicate that all matching items should be removed from the list. 2) Return a boolean that indicates if anything was removed from the list (this allows the user to manually loop until all matching items have been removed if required).

In method Call() again I would have used a for(;;) loop.

You can also do one small optimization by not calling Call() on the actual head node (as you using sentinal that does nothing no point it calling the sentinal).