Thread: dynamic_cast

I'm trying to downcast from a GameEvent* to a Shot*

Code:

void Game::addEvent(GameEvent* event) {
  boardTime=&(Time)(event->getTime());
  int timePeriod=event->getTime().getPeriod();
  if (timePeriod==2 && halfTimeouts==0) {
    halfTimeouts=1;
    homeTimeouts=3;
    awayTimeouts=3;
  }
  if (event->getType().compare("shot")==0) {
    Shot* shot=dynamic_cast<Shot*>(event);
    if (shot != 0) {
      cout << shot->getShooter()->getName()<<endl;
    }
    else {
      cout << "This object is not of type Shot"<<endl;
    }
    if (shot->getSuccessful()==1) {
      if (shot->getShooter()->getTeam()==homeTeam) {
        homeScore+=shot->getType();
      }
      else {
        awayScore+=shot->getType();
      }
    }
    events.push_back(event);
  }
}

I'm getting a segfault because when I call getShooter() there's no information to get. I was wondering what I'm doing wrong.

Maybe because that bit of code is outside where you test if the event pointer is a Shot pointer or not?

That's what the

Code:

(event->getType().compare("shot")==0)

is for. Each GameEvent has a variable string type which tells me that it's a shot. Also, I'm know why the segfault exists, but I'm wondering why shot is never being initialized.

I think what hk_mp5kpdw means is that you should have written:

Code:

  if (event->getType().compare("shot")==0) {
    Shot* shot=dynamic_cast<Shot*>(event);
    if (shot != 0) {
      cout << shot->getShooter()->getName()<<endl;
      if (shot->getSuccessful()==1) {
        if (shot->getShooter()->getTeam()==homeTeam) {
          homeScore+=shot->getType();
        }
        else {
          awayScore+=shot->getType();
        }
      }
      events.push_back(event);
    }
    else {
      cout << "This object is not of type Shot"<<endl;
    }
  }

If the purpose of that test is to definitively determine that event is a shot event then do you really need the if/else test after the dynamic cast? Would the bit in green below ever get executed if we've already determined that we have a shot object?

Otherwise, if not, then you'd definitely need to be careful here:

Code:

  if (event->getType().compare("shot")==0) {
    Shot* shot=dynamic_cast<Shot*>(event);
    if (shot != 0) {
      cout << shot->getShooter()->getName()<<endl;
    }
    else {
      cout << "This object is not of type Shot"<<endl;
    }
    if (shot->getSuccessful()==1) {
      if (shot->getShooter()->getTeam()==homeTeam) {
        homeScore+=shot->getType();
      }
      else {
        awayScore+=shot->getType();
      }
    }
    events.push_back(event);
  }

If the dynamic cast happens to fail, you simply print a message (the green bit) and then go on and attempt to use the pointer (the bit in red above) as if it succeeded. Maybe that red bit belongs with the blue bit.

[edit]Yeah, that what I was talking about. Thanks laserlight[/edit]

By the way, why do you have what amounts to two type checks? First you check the event type string against "shot" and then you check the event object type against Shot. Shouldn't those two things be guaranteed to occur at the same time, so that you can use static_cast instead of dynamic_cast?

Originally Posted by laserlight

I think what hk_mp5kpdw means is that you should have written:

Personally, I'd consider doing it the other way around.

Code:

    Shot* shot=dynamic_cast<Shot*>(event);
    if (shot != 0 && event->getType().compare("shot")==0) {
      cout << shot->getShooter()->getName()<<endl;
      if (shot->getSuccessful()==1) {
        if (shot->getShooter()->getTeam()==homeTeam) {
          homeScore+=shot->getType();
        }
        else {
          awayScore+=shot->getType();
        }
      }
      events.push_back(event);
    }
    else {
      cout << "This object is not of type Shot"<<endl;
    }
  }

All that's happening here is that there are multiple checks on the event type. If both checks are required (i.e. one can give a false positive that is caught by the other) then make it more obvious in how you do the test.

In practice, I'd prefer to do the dynamic_cast<> check first. That's a philosophical thing: use language features rather than hand-rolled code to achieve a desired effect if possible. More chance - subject to using a suitably verified compiler - of the code doing what is intended in various operating conditions. I therefore tend to look askance at classes that implement some form of type checking mechanism.

There may be a performance argument for doing it the other way around (dynamic_cast<> is not necessarily all that efficient at run time), but that sort of decision needs to be made based on profiling, etc.

I'd also be looking closely to see if the getType->compare() parts are even needed. That suggests holes in the class design.

That suggests holes in the class design.

I would agree that this statement seems to sum up the problem here.

dynamic_cast<> is not necessarily all that efficient at run time)

Definitely an understatement. I used to have a link to a website that compared all the casts and by far dynamic was the slowest. Of course this may depend on compiler implementation and so forth but in general dynamic_cast is quite slow. You really should never have to query an object for it's C++ type. This points to a flaw in the design of the system.