oop - C++ - Proper way to implement type-specific behavior?

Question

Let's say I have a parent class, Arbitrary, and two child classes, Foo and Bar. I'm trying to implement a function to insert any Arbitrary object into a database, however, since the child classes contain data specific to those classes, I need to perform slightly different operations depending on the type.

Coming into C++ from Java/C#, my first instinct was to have a function that takes the parent as the parameter use something like instanceof and some if statements to handle child-class-specific behavior.

Pseudocode:

void someClass(Arbitrary obj){
    obj.doSomething(); //a member function from the parent class
    //more operations based on parent class
    if(obj instanceof Foo){
        //do Foo specific stuff
    }
    if(obj instanceof Bar){
        //do Bar specific stuff
    }
}

However, after looking into how to implement this in C++, the general consensus seemed to be that this is poor design.

If you have to use instanceof, there is, in most cases, something wrong with your design. – mslot

I considered the possibility of overloading the function with each type, but that would seemingly lead to code duplication. And, I would still end up needing to handle the child-specific behavior in the parent class, so that wouldn't solve the problem anyway.

So, my question is, what's the better way of performing operations that where all parent and child classes should be accepted as input, but in which behavior is dictated by the object type?

Answer 1

First, you want to take your Arbitrary by pointer or reference, otherwise you will slice off the derived class. Next, sounds like a case of a virtual method.

void someClass(Arbitrary* obj) {
    obj->insertIntoDB();
}

where:

class Arbitrary {
public:
    virtual ~Arbitrary();
    virtual void insertIntoDB() = 0;
};

So that the subclasses can provide specific overrides:

class Foo : public Arbitrary {
public:
    void insertIntoDB() override
    //                  ^^^ if C++11
    {
        // do Foo-specific insertion here
    }
};

Now there might be some common functionality in this insertion between Foo and Bar... so you should put that as a protected method in Arbitrary. protected so that both Foo and Bar have access to it but someClass() doesn't.

Answer 2

The issue here is that this will arguably violate SOLID design principles, given that any extension in the number of mapped classes would require new branches in the if statement, otherwise the existing dispatch method will fail (it won't work with any subclass, just those it knows about).

What you are describing looks well suited to inheritance polymorphicism - each of Arbitrary (base), Foo and Bar can take on the concerns of its own fields.

There is likely to be some common database plumbing which can be DRY'd up the base method.

class Arbitrary { // Your base class
  protected:
   virtual void mapFields(DbCommand& dbCommand) {
     // Map the base fields here
   }

  public:
   void saveToDatabase() { // External caller invokes this on any subclass
      openConnection();
      DbCommand& command = createDbCommand();
      mapFields(command); // Polymorphic call
      executeDbTransaction(command);
   }
}

class Foo : public Arbitrary {
   protected: // Hide implementation external parties
     virtual void mapFields(DbCommand& dbCommand) {
        Arbitrary::mapFields();
        // Map Foo specific fields here
     }
}

class Bar : public Arbitrary {
   protected:
     virtual void mapFields(DbCommand& dbCommand) {
        Arbitrary::mapFields();
        // Map Bar specific fields here
     }
}

If the base class, Arbitrary itself cannot exist in isolation, it should also be marked as abstract.

Answer 3

In my opinion, if at any place you need to write

if( is_instance_of(Derived1) )
    //do something
else if ( is_instance_of(Derived2) )
    //do somthing else
...

then it's as sign of bad design. First and most straight forward issue is that of "Maintainence". You have to take care in case further derivation happens. However, sometimes it's necessary. for e.g if your all classes are part of some library. In other cases you should avoid this coding as far as possible.

Most often you can remove the need to check for specific instance by introducing some new classes in the hierarchy. For e.g :-

class BankAccount {};
class SavingAccount : public BankAccount { void creditInterest(); };
class CheckingAccount : public BankAccount { void creditInterest(): };

In this case, there seems to be a need for if/else statement to check for actual object as there is no corresponsing creditInterest() in BanAccount class. However, indroducing a new class could obviate the need for that checking.

class BankAccount {};
class InterestBearingAccount : public BankAccount { void creditInterest(): } {};
class SavingAccount : public InterestBearingAccount { void creditInterest(): };
class CheckingAccount : public InterestBearingAccount { void creditInterest(): };

Answer 4

As StuartLC pointed out, the current design violates the SOLID principles. However, both his answer and Barry's answer has strong coupling with the database, which I do not like (should Arbitrary really need to know about the database?). I would suggest that you make some additional abstraction, and make the database operations independent of the the data types.

One possible implementation may be like:

class Arbitrary {
public:
    virtual std::string serialize();
    static Arbitrary* deserialize();
};

Your database-related would be like (please notice that the parameter form Arbitrary obj is wrong and can truncate the object):

void someMethod(const Arbitrary& obj)
{
    // ...
    db.insert(obj.serialize());
}

You can retrieve the string from the database later and deserialize into a suitable object.

Answer 5

So, my question is, what's the better way of performing operations that where all parent and child classes should be accepted as input, but in which behavior is dictated by the object type?

You can use Visitor pattern.

#include <iostream>

using namespace std;

class Arbitrary;
class Foo;
class Bar;

class ArbitraryVisitor
{
public:

    virtual void visitParent(Arbitrary& m)  {};
    virtual void visitFoo(Foo& vm) {};
    virtual void visitBar(Bar& vm) {};
};

class Arbitrary
{
public:
    virtual void DoSomething()
    {
        cout<<"do Parent specific stuff"<<endl;
    }

    virtual void accept(ArbitraryVisitor& v)
    {
        v.visitParent(*this);
    }
};

class Foo: public Arbitrary
{
public:

    virtual void DoSomething()
    {
        cout<<"do Foo specific stuff"<<endl;
    }

    virtual void accept(ArbitraryVisitor& v)
    {
        v.visitFoo(*this);
    }
};

class Bar: public Arbitrary
{
public:

    virtual void DoSomething()
    {
        cout<<"do Bar specific stuff"<<endl;
    }

    virtual void accept(ArbitraryVisitor& v)
    {
        v.visitBar(*this);
    }
};

class SetArbitaryVisitor : public ArbitraryVisitor
{
    void visitParent(Arbitrary& vm)
    {
        vm.DoSomething();
    }

    void visitFoo(Foo& vm)
    {
        vm.DoSomething();
    }

    void visitBar(Bar& vm)
    {
        vm.DoSomething();
    }
};

int main()
{
    Arbitrary *arb = new Foo();
    SetArbitaryVisitor scv;
    arb->accept(scv);
}