A diamond-inheritance problem

Question

Just for fun I am working on a XUL implementation for Windows. In XUL, UI elements can be written in XML like this:

<window width="800" height="600"></window>

I am considering a system for getting and setting element attributes. It's working pretty well but I am not certain if the use of diamond inheritance is potentially hazardous here. I've posted a complete code sample below:

#include <boost/lexical_cast.hpp>
#include <string>
#include <map>


class Attribute
{
public:
    virtual void get(std::string & outValue) = 0;
    virtual void set(const std::string & inValue) = 0;

    static int String2Int(const std::string & inString)
    {
        return boost::lexical_cast<int>(inString);
    }

    static std::string Int2String(int inValue)
    {
        return boost::lexical_cast<std::string>(inValue);
    }
};


class Width : public Attribute
{
public:
    Width(){}

    virtual void get(std::string & outValue)
    {
        outValue = Int2String(getWidth());
    }

    virtual void set(const std::string & inValue)
    {
        setWidth(String2Int(inValue));
    }

    virtual int getWidth() const = 0;

    virtual void setWidth(int inWidth) = 0;
};


class Height : public Attribute
{
public:
    Height(){}

    virtual void get(std::string & outValue)
    {
        outValue = Int2String(getHeight());
    }

    virtual void set(const std::string & inValue)
    {
        setHeight(String2Int(inValue));
    }

    virtual int getHeight() const = 0;

    virtual void setHeight(int inHeight) = 0;
};

class Element : public Width,  // concerning the is-a vs has-a philosophy
                public Height  //   => see my note below
{
public:
    Element() :
        mWidth(0),
        mHeight(0)
    {
        // STATIC CAST NEEDED HERE OTHERWISE WE GET COMPILER ERROR:
        // error C2594: '=' : ambiguous conversions from 'Element *const ' to 'Attribute *'
        mAttrControllers["width"] = static_cast<Width*>(this);
        mAttrControllers["height"] = static_cast<Height*>(this);
    }

    void setAttribute(const std::string & inAttrName, const std::string & inAttrValue)
    {
        Attributes::iterator it = mAttrControllers.find(inAttrName);
        if (it != mAttrControllers.end())
        {
            Attribute * attribute = it->second;
            attribute->set(inAttrValue);
        }
    }

    std::string getAttribute(const std::string & inAttrName)
    {
        std::string result;
        Attributes::iterator it = mAttrControllers.find(inAttrName);
        if (it != mAttrControllers.end())
        {
            Attribute * attribute = it->second;
            attribute->get(result);
        }
        return result;
    }

    virtual int getWidth() const
    {
        return mWidth;
    }

    virtual void setWidth(int inWidth)
    {
        mWidth = inWidth;
    }

    virtual int getHeight() const
    {
        return mHeight;
    }

    virtual void setHeight(int inHeight)
    {
        mHeight = inHeight;
    }

private:
    typedef std::map<std::string, Attribute *> Attributes;
    Attributes mAttrControllers;
    int mWidth;
    int mHeight;
};


int main()
{
    Element el;
    el.setAttribute("width", "800");
    el.setAttribute("height", "600");
    int w = el.getWidth();
    int h = el.getHeight();
    return 0;
}

I think it's ok since the base class Attributes has no data members, so no conflicts can arise there. But I thought I'd check with the community. Your insights are much appreciated!

Edit About the "is-a" vs "has-a", and "favor composition over inheritance" remarks I have this to say:

• There is an advantage to inheritance here. If Element inherits Width then it is forced to implement the getWidth and setWidth methods. So adding an attribute implies an 'automatic' update of Element's interface.
• I originally named these classes AttributeController, WidthController and HeightController, but I found them too verbose. You could say that my Element is an Attribute controller. (Ok, that's lame, but not untrue!)
• Further proof: the definitions of Width and Height do not contain any data members. The Element class actually has them. The Width and Height classes only provide the interfaces. So it's more of a can-do relationship.

Answer 1

In your scenario, Element should probably not inherit from Width and Height, but instead, Width and Height should be data members of element. It's composition as opposed to is-a since arguably an Element is-not-a Width or Height but is composed of a Width and Height (and probably some other stuff too).

Answer 2

If I was going to do this, I'd do a virtual inherit on Attribute. I don't think it will matter significantly, but that will minimize the duplication if it does end up mattering.

Answer 3

First of all, use virtual inheritance if you are facing face a diamond problem, that is, use public virtual instead of just public on the base classes.

Secondly, it's not well defined what your get- and set-will do as there are two implementations. The only time I use multiple inheritance is when I extend pure virtual classes (aka interfaces). It's a good reason for Java to not support multiple inheritance.

Third, and more importantly, this seems like a classic case between a misunderstanding of inheritance ("is a") vs aggregation ("has a") in object orientation. You can two very simple guidelines when determining if a class should inherit another class. If you have class A and class B which inherits class A, the sentence "A is a B" should make sense. If "A has a B" sounds better, you should really consider letting B be a member of A instead.

In your case the sentences "Element is a Height" and "Element is a Width" really doesn't make sense. "Element has a Height" makes perfectly sense.

Answer 4

Element should inherit Width only if you need to use Element objects as Width ones . Inheritance is not for code reuse.

Maybe would be worth for you taking a look into boost::program_options library. I like the fancy way they have for registering properties.