Is it bad practice to check object types with an identifying member variable?

Question

Preamble: I am making a simple board game in C++, in which AI characters move around squares on the board. There are different types of squares, each inherited from an abstract class, each with different effects on the AI character. For example, some are shops where the character can buy stuff, others are workplaces where characters get money, etc.

Likewise, there are different AI characters (also inherited from an abstract class), which make different kind of decisions. Some work more than others, some eat more than others, etc.

The problem is that the AI character needs to know the square type in order to make a decision about what to do. At the moment checking the type using a type-name string as a member variable is the easiest solution, but it feels horribly inelegant and not very helpful when it comes to adding new types of squares and AI characters.

Question: Is it bad practice to check the type of an object by asking it for an identifier which is placed into its class for this purpose? Is it bad practice to use other methods of run time type identification such as dynamic_cast and typeinfo for purposes of controlling the flow of execution?

Example: Below the Square class (accessed through currentSquare() has a member function getType() which returns a string with the name of the type:

//Normal AI character
void decide() {
    if(currentSquare()->getType()=="HOME") {
        watchTV();
        sleep();
    } 
    if(currentSquare()->getType()=="WORKPLACE") {
        work();
    } 
}

//Party animal AI
void decide() {
    if(currentSquare()->getType()=="HOME") {
        moveTo(getGameBoard()->closestBar());
    } 
    if(currentSquare()->getType()=="WORKPLACE") {
        sleep();
    } 
}

Answer 1

As Amon pointed out, this is a good application for the visitor pattern. Using it, your AI classes will end up looking something like this:

void decide(HomeSquare square);
void decide(WorkSquare square);
void decide(ShopSquare square);

And your squares have an accept function that looks like:

void accept(AI ai)
{
    ai.decide(this);
}

That lets you use inheritance to create appropriate defaults, or force every AI to implement certain decisions. You can create an AI that behaves exactly like another one except at home, for example, without copying a bunch of code. The compiler will point out where you forget something, and you don't have to cast all over the place.

Answer 2

I prefer to use enums rather than strings for situations like this, and the "check type" code should be written to exclude the unspecified cases.

switch (currentSquare()->getType())
{ default: BUG("case not expected");
  case workplace: ...
    break;
  case home: ..
    break;
}

Answer 3

I would like to point out the fragility of using strings for identifying classes of objects. You are somewhat working around type safety and if something goes wrong you may receive weird errors at run time. If you have quite some classes, you want to check, it can become a pain to update all parts of the code if you introduce a new class or delete another. Moreover it complicates or even prevents static analysis thus reducing the likelihood of automatically finding bugs (like dead code).

Compare this to using the visitor pattern that others already pointed out. You will get type safety and (if something goes wrong) compile time errors. On top of this the visitor pattern gives you more flexibility and you won't get long cascaded if-else blocks or switch blocks, thus improving readability and maintainability. Static analysis tools will work as intended.

So I would argue that using member variables for identification is at the very best suboptimal.

---

Full disclosure: I'm mainly a java developer but I think in this case the differences between java and c++ are not a concern. If I am wrong someone please correct me. ;)