Subclassing and adding data members

Question

I have an hierarchy of classes that looks like the following:

    class Critical
    {
    public:
        Critical(int a, int b) : m_a(a), m_b(b) { }
        virtual ~Critical() { }
        int GetA() { return m_a; }
        int GetB() { return m_b; }
        void SetA(int a) { m_a = a; }
        void SetB(int b) { m_b = b; }
    protected:
        int m_a;
        int m_b;
    };

    class CriticalFlavor : public Critical
    {
    public:
        CriticalFlavor(int a, int b, int flavor) : Critical(a, b), m_flavor(flavor) { }
        virtual ~CriticalFlavor() { }
        int GetFlavor() { return m_flavor; }
        void SetFlavor(int flavor) { m_flavor = flavor; }
    protected:
        int m_flavor;
    };

    class CriticalTwist : public Critical
    {
    public:
        CriticalTwist(int a, int b, int twist) : Critical(a, b), m_twist(twist) { }
        virtual ~CriticalTwist() { }
        int GetTwist() { return m_twist; }
        void SetTwist(int twist) { m_twist = twist; }
    protected:
        int m_twist;
    };

The above does not seem right to me in terms of the design and what bothers me the most is the fact that the addition of member variables seems to drive the interface of these classes (the real code that does the above is a little more complex but still embracing the same pattern). That will proliferate when in need for another "Critical" class that just adds some other property. Does this feel right to you? How could I refactor such code? An idea would be to have just a set of interfaces and use composition when it comes to the base object like the following:

    class Critical
    {
    public:
        virtual int GetA() = 0;
        virtual int GetB() = 0;
        virtual void SetA(int a) = 0;
        virtual void SetB(int b) = 0;
    };

    class CriticalImpl : public Critical
    {
    public:
        CriticalImpl(int a, int b) : m_a(a), m_b(b) { }
        ~CriticalImpl() { }
        int GetA() { return m_a; }
        int GetB() { return m_b; }
        void SetA(int a) { m_a = a; }
        void SetB(int b) { m_b = b; }
    private:
        int m_a;
        int m_b;
    };

    class CriticalFlavor
    {
    public:
        virtual int GetFlavor() = 0;
        virtual void SetFlavor(int flavor) = 0;
    };

    class CriticalFlavorImpl : public Critical, public CriticalFlavor
    {
    public:
        CriticalFlavorImpl(int a, int b, int flavor) : m_flavor(flavor), m_critical(new CriticalImpl(a, b)) { }
        ~CriticalFlavorImpl() { delete m_critical; }
        int GetFlavor() { return m_flavor; }
        void SetFlavor(int flavor) { m_flavor = flavor; }
 int GetA() { return m_critical->GetA(); }
        int GetB() { return m_critical->GetB(); }
        void SetA(int a) { m_critical->SetA(a); }
        void SetB(int b) { m_critical->SetB(b); }
    private:
        int m_flavor;
 CriticalImpl* m_critical;
    };

Answer 1

My suggestion: find the most patient person you work with who is familiar with this code, and ask them some of these questions. I assume you are not posting a more complete example because of IP concerns. That makes it hard to provide good advice.

Based on your first code sample, I would say just use structs with public data and no accessors. But if I saw the real code, I would probably change my tune.

For your second code sample: A benefit is that you could have another class depend on CriticalFlavor without knowing anything about Critical (could be used to implement something like the Bridge pattern, for example). But if that potential benefit isn't an actual benefit in your situation, then it's just making your code unnecessarily complicated and YAGNI (probably).

---

The comments from your reviewers:

base classes should be abstract

I would say, "base classes usually should have at least one virtual method, other than the destructor". If not, then it's just a means to share common code or data amongst other classes; try to use composition instead.

Most of the time, at least one of those virtual methods will be pure virtual, so the base class will be abstract. But sometimes there is a good default implementation for every virtual method and subclasses will pick and choose which to override. In that case, make the base class constructors protected to prevent instantiation of the base class.

protected members are not advisable in base classes

... and they're completely pointless in non-base classes, so this advice basically says to never use them.

When are protected member variables advisable? Infrequently. Your code example isn't realistic enough to determine what you're trying to do or how it could be best written. The members are protected, but there are public getters/setters, so they're essentially public.

design by interface

Don't get carried away by this, or you may end up with an amazingly complicated design for an amazingly simple task. Use interfaces where they make sense. It's hard to tell if they make sense without seeing some of the "calling code" that uses Critical and its subclasses. Who calls GetFlavor and GetTwist?

Does the calling code only interact with Critical subclasses via the Critical interface, or does the calling code know the specific subclass and call specific subclass methods? Have you added interface methods to Critical in order to provide access to data/functionality only present in some of the subclasses? That can be a bad smell.

---

Your comment:

the addition of member variables seems to drive the interface of these classes

makes me think of an item in C++ Coding Standards (Sutter/Alexandrescu): "Be clear what kind of class you're writing". Sorry, no online reference to that item (buy the book).

---

Let me also suggest that you honestly evaluate your skill level and that of your reviewers. Are your reviewers experienced C++ developers who really know what they are talking about (if so, listen!), or have they just returned from Code Review 101 and know to say things like "public data is bad" and "destructors should always be virtual"? If the latter, hopefully you can respond to the review comments by saying, "That's generally good advice, but does not apply in this situation because XYZ."

Answer 2

It's hard to say whether inheritance is the right idea here without knowing more about your real code. Do you want polymorphism (acting on a Critical and getting different effects if it's a CriticalFlavor, CriticalTwist or CriticalWhatever)? Does the order of construction of A's and B's and things matter? Do you plan to have a more complicated class tree, with things like CriticalFlavorTemperature and CriticalTwistColor?

That said, here's what I'd probably do for a composition approach:

class Critical
{
public:
    Critical(int a, int b) : m_a(a), m_b(b) { }
    ~Critical() { }
    int GetA() const { return m_a; }
    int GetB() const { return m_b; }
    void SetA(int a) { m_a = a; }
    void SetB(int b) { m_b = b; }
private:
    int m_a;
    int m_b;
};

class CriticalFlavor
{
public:
    CriticalFlavor(int a, int b, int flavor) : m_flavor(flavor), m_critical(a, b) { }
    ~CriticalFlavor() {}
    int GetFlavor() const { return m_flavor; }
    void SetFlavor(int flavor) { m_flavor = flavor; }
    int GetA() const { return m_critical.GetA(); }
    int GetB() const { return m_critical.GetB(); }
    void SetA(int a) { m_critical.SetA(a); }
    void SetB(int b) { m_critical.GetB(b); }
private:
    int m_flavor;
    Critical m_critical;
};

Answer 3

@Marius all I can understand from your post is that you want to separate access to data from actual data structure.

class Critical
{
public:
    virtual int GetA() = 0;
    virtual int GetB() = 0;
    virtual void SetA(int a) = 0;
    virtual void SetB(int b) = 0;
};

class Flavor
{
public:
    virtual int GetFlavor() = 0;
    virtual void SetFlavor(int a) = 0;
};

template<class T>
class AccessCritical : public Critical
{
public:
    AccessCritical(T &ref) : data(ref) {}

    int GetA() { return (data.m_a); }
    int GetB() { return (data.m_b); }
    void SetA(int a) { data.m_a = a; }
    void SetB(int b) { data.m_b = b; }
private:
    T &data;
};

template<class T>
class AccessFlavor : public Flavor
{
public:
    AccessFlavor(T &ref) : data(ref) {}
    int GetFlavor() { return (data.m_flavor); }
    void SetFlavor(int flavor) { data.m_flavor = flavor; }
private:
    T &data;
};

struct MyStructA {
    int m_a, m_b;

    AccessCritical<MyStructA> critical;
    MyStructA() : critical(*this) {}
};

struct MyStructB {
    int m_flavor;

    AccessFlavor<MyStructB> flavor;
    MyStructB() : flavor(*this) {}
};

struct MyStructC {
    int m_a, m_b;
    int m_flavor;

    AccessCritical<MyStructC> critical;
    AccessFlavor<MyStructC> flavor;
    MyStructC() : critical(*this), flavor(*this) {}
};
void processFlavor(Flavor *flavor)
{
    flavor->SetFlavor(flavor->GetFlavor() * 3);
}
void processCritical(Critical *critical)
{
    critical->SetA(critical->GetA() + critical->GetB());
}
int main() {
    MyStructA a;
    a.critical.SetA(2);
    a.critical.SetB(3);
    processCritical(& a.critical);

    MyStructB b;
    b.flavor.SetFlavor(4);
    processFlavor(& b.flavor);

    MyStructC c;
    c.critical.SetA(20);
    c.critical.SetB(30);
    c.flavor.SetFlavor(40);
    processFlavor(& c.flavor);
    processCritical(& c.critical);

    cerr << a.critical.GetA() << ' ' << a.critical.GetB() << endl;
    cerr << b.flavor.GetFlavor() << endl;
    cerr << c.critical.GetA() << ' ' << c.critical.GetB() << endl;
    cerr << c.flavor.GetFlavor() << endl;

    return 0;
}

Answer 4

It seems to me that this hierachy of classes are no more than data buckets, that they don't actually do anything other than to act as an accessor to some underlying data model. Maybe just restrict your design to a single class with your critical accessor, and another more generic named property method (ie. std::string GetStringProperty( std::string key ){} )?