c++ with pointers losing pointer reference

Question

I got a class that is described below

class Investment
{
private:
    void init(BWAPI::UnitType type1, BWAPI::UpgradeType type2, BWAPI::TechType type3, int numOfItems);
    UpgradeType upgradeType;
    TechType techType;
    UnitType unitType;

and my init method is like that

void Investment::init(BWAPI::UnitType type1, BWAPI::UpgradeType type2, BWAPI::TechType type3, int numOfItems)
{
    if (type1 != NULL) {

        this->unitType = type1;
        this->type = type1.isBuilding() ? BUILDING_TYPE : UNIT_TYPE;

    } else if (type2 != NULL) {

        this->upgradeType = type2;  
        this->type = UPGRADE_TYPE;

    } else if (type3 != NULL) {

        this->techType = type3; 
        this->type = TECH_TYPE;
    }

    this->numOfItems = numOfItems;
}

and I get my item (that can be only one of the three possible types) this way

const void* Investment::getItem() const
{
    if (unitType != NULL)
        return &unitType;
    else if (upgradeType != NULL)
        return &upgradeType;
    else if (techType != NULL)
        return &techType;

    return NULL;
}

but what happens when I get the item using UnitType* type = (UnitType*) investment->getItem(); the pointer lose its value

when I call type->getName().c_str(); it returns an empty string

to get investment I call a method of a Stack

// I have a stack of type std::vector<T*> stack;

T* getNext() {

    clear();

    for (int i = 0, leni = stack.size(); i < leni; i++) {

        T* t = stack.at(i);

        if (!t->isDone() && !t->isCanceled())
            return t;
    }

    return NULL;
}

Answer 1

Since you can't refactor as suggested, you should check what type you need to return based on the type member that you assign in the init() function. I assume this will work, but you haven't shown where type is defined:

const void* Investment::getItem() const
{
    switch(type)
    {
        case BUILDING_TYPE:
        case UNIT_TYPE:
           return &unitType;
        case UPGRADE_TYPE:
           return &upgradeType;
        case TECH_TYPE:
           return &techType;
    }

    return NULL;
}

Doing this however, means that the caller of getItem() has to be able to perform the appropriate downcast. Given that you don't provide this explicitly, this is going to be a constant source of confusion of users of your code.

However:

If you are passing around void* and expecting users of your class to downcast to the correct type, this is almost always an indication of bad design in C++. The correct approach would be to refactor this into a proper polymorphic class hierarchy as suggested by @juanchopanza in his answer.

Answer 2

Just a quick look at the code makes me suspicious... Are the types used (i.e. UpgradeType, TechType and UnitType) pointers? It doesn't look like that since you are using the address-of operator to return their pointers in getItem.

If they are not pointers, you can't compare them to NULL, as non-pointer are never NULL. This means that getItem will always return a pointer to the unitType variable.

I suggest you do a redesign, as the one suggested in the answer by juanchopanza.

Answer 3

I think you could greatly simplify the problem by having the Investment class hold a (preferably smart) pointer to a Type base class. In this example, I use a raw pointer which gets deleted in the Investment destructor, but you should probably use an std::unique_ptr, a boost::scoped_ptr, or an std::shared_ptr if you want to decouple the lifetime of the type from Investment.

class InvestmentType { // some public virtual methods };

class UpdgadeType : public InvestmentType { /* Implement InvestmentType methods*/ };
class TechType : public InvestmentType { /* Implement InvestmentType methods*/ };
class UnitType : public InvestmentType { /* Implement InvestmentType methods*/ };

class Investment
{
 private:
    void init(const InvestmentType&  type, int numOfItems) : nItems_(numOfItems), type_(new type) {}
    int nItems_;
    InvestmentType* type_;
    ~Investment() { delete type_; }
 public:
    const InvestmentType* getItem() const { return type_; }
};