Is checking of object type really always sign of bad design?

Question

I have a source of some lines of text, each of which is a message, representing object of some type. I'm making a parser for these lines, which should take the text line as input and give the ready to use object as output. So I make the following hierarchy of classes:

class Message
{
public:
    virtual ~Message(){};
};

class ObjectTypeA : public Message
{/*...*/};
class ObjectTypeB : public Message
{/*...*/};
class ObjectTypeC : public Message
{/*...*/};

and here's how it's used:

std::shared_ptr<Message> parseLine(std::string& line);

void doWork()
{
    std::string line;
    while(getLine(line))
    {
        std::shared_ptr<Message> object=parseLine(line);
        if(dynamic_cast<ObjectTypeA*>(object.get()))
            doSomethingA(*static_cast<ObjectTypeA*>(object.get()));
        else if(dynamic_cast<ObjectTypeB*>(object.get()))
            doCompletelyUnrelatedProcessing(*static_cast<ObjectTypeB*>(object.get()));
        else if(dynamic_cast<ObjectTypeC*>(object.get()))
            doSomethingEvenMoreDifferent(*static_cast<ObjectTypeC*>(object.get()));
    }
}

Here the parser would be a library function, and the objects don't know in advance how they will be processed. So, I can't put the processing code to a virtual function of Message implementations.

But many of the answers in this question say that if one needs to check type of the object, it's a sign of bad design. But I can't seem to see what's bad here. Is there any better way to organize the solution?

Answer 1

You're really asking for opinions on whats good and bad design. Here's mine:

Yours is bad design, because you try to do something in another class that should be handled by the subclasses, because that's what polymorphism is for.

Your mother class should have a

virtual void do_stuff_that_is_specific_to_the_subclass(...) = 0;

method, which you'd implement in your subclasses.

Here the parser would be a library function, and the objects don't know in advance how they will be processed. So, I can't put the processing code to a virtual function of Message implementations.

Why not? You should simply have a

virtual void do_stuff_that_is_specific_to_the_subclass(parser&, ...) = 0;

method that uses the parser differently for each subclass. There's no reason that what you can do in your if/else clauses couldn't just be done in the subclasses, unless it breaks encapsulation, which I'd doubt, because the only reason you've got these objects is that you want to do specific things differently for different lines.

Answer 2

First off, it's not always a sign of bad design. There are very few absolutes in "soft" things like "good" or "bad" design. Nevertheless, it does often indicate a different approach would be preferable, for one or more of these reasons: extensibility, ease of maintenance, familiarity, and similar.

In your particular case: One of the standard ways to make arbitrary class-specific processing possible without type switches or bloating/polluting the interface of the class is to use the Visitor pattern. You create a generic MessageVisitor interface, teach the Message subclasses to call into it, and implement it wherever you need to process them:

class MessageVisitor;

class Message
{
public:
    virtual ~Message(){};

    virtual void accept(MessageVisitor &visitor) = 0;
};

class ObjectTypeA : public Message
{
  void accept(MessageVisitor &visitor) override
  { visitor.visit(*this); }
/*...*/
};

class ObjectTypeB : public Message
{
  void accept(MessageVisitor &visitor) override
  { visitor.visit(*this); }
/*...*/
};

class ObjectTypeC : public Message
{
  void accept(MessageVisitor &visitor) override
  { visitor.visit(*this); }
/*...*/
};

class MessageVisitor
{
public:
  virtual void visit(ObjectTypeA &subject) {}

  virtual void visit(ObjectTypeB &subject) {}

  virtual void visit(ObjectTypeC &subject) {}
};

You would then use it like this:

void doWork()
{
    struct DoWorkMessageVisitor : MessageVisitor
    {
      void visit(ObjectTypeA &subject) override { doSomethingA(subject); }

      void visit(ObjectTypeB &subject) override { doSomethingB(subject); }

      void visit(ObjectTypeC &subject) override { doSomethingC(subject); }
    };
    std::string line;
    while(getLine(line))
    {
        std::shared_ptr<Message> object=parseLine(line);
        DoWorkMessageVisitor v;
        object->accept(v);
    }
}

Feel free to customise this with const overloads etc. as necessary.

Note that accept cannot be implemented in the base class, because you need the correct type of *this in the invocation of visit. That is where the type switch has "moved".

---

An alternative is to make the visit functions in MessageVisitor pure virtual instead of empty. Then, if you need to add a new message type, it will automatically force you to update all places where such type-specific processing occurs.

Answer 3

doSomethingA, doCompletelyUnrelatedProcessing and doSomethingEvenMoreDifferent could be just overrides of pure virtual function of Message class. In your case that would be much more effecient and better as a design solution.