C++: Problems with inheritance and list functions as members of a class

Question

This question is somewhat of a follow-up question to a question I have previously asked, as I'm trying to understand and implement the concepts suggested. I have refactored everything into smaller classes, and the overall design of a Weapon has been completed (and somewhat implemented). However, I'm having trouble accessing some members, and I believe I will have no trouble completing this project, once this roadblock has been surpassed, as a lot of sub-classes use a similar system.

Onto my problem, I have a parent, partly-virtual class, defined as follows:

class ModeInformation {


public:
    ModeInformation() { m_CreateModes(); m_CreateModeChoiceList(); } // These two functions are always called when an object belonging to the ModeInformation parent class is created, as they store elements in two lists, one in which the pointer-to-Modes themselves are stored, the other where Menu options are stored according to the number of modes
    virtual ~ModeInformation() {}

// Modes of a Weapon (virtual as they might be overridden in child classes)
    virtual Mode* pMode1() const { return NULL; }
    virtual Mode* pMode2() const { return NULL; }
    virtual Mode* pMode3() const { return NULL; }
    virtual Mode* pMode4() const { return NULL; }
    virtual Mode* pMode5() const { return NULL; }

// Lists mentioned earlier 
    virtual list<string>* ModeChoiceList() const { return new list<string>; }
    virtual list<Mode*>* Modes() const { return new list<Mode*>; };

// m_CreateModes() stores pointers-to-Mode in Modes(), if these are not NULL
    void m_CreateModes() {
        if (!pMode1() == NULL) { Modes()->push_back(pMode1()); }
        if (!pMode2() == NULL) { Modes()->push_back(pMode2()); }
        if (!pMode3() == NULL) { Modes()->push_back(pMode3()); }
        if (!pMode4() == NULL) { Modes()->push_back(pMode4()); }
        if (!pMode5() == NULL) { Modes()->push_back(pMode5()); }
    }

// m_CreateModeChoiceList() stores strings in ModeChoiceList(), composed using the sstring library. Also a source of problems, as I will point out later on.
    void m_CreateModeChoiceList() {
        int i = 1;
        for (list<Mode*>::iterator it = Modes()->begin(); it != Modes()->end(); it++) {
            stringstream ChoiceDeclaration;
            ChoiceDeclaration << "\n" << i << ".- Mode " << i;
            ModeChoiceList()->push_back(ChoiceDeclaration.str());
            i++;
        }
        ModeChoiceList()->push_back("\n0.- Quit to previous menu.");
    }

// m_PrintBasicInfo() is called from classes that possess an object pertaining to the ModeInformation class (or derived child-classes), serves as a decision tree to judge whether the Mode List should be printed (in the case the Weapon being printed possesses more than one Mode), otherwise it will print the very first Mode
    void m_PrintBasicInfo() {
        if (Modes()->size() > 1) {
            m_PrintModeList();
            m_ChooseModeFromList();
        }
        else {
            pMode1()->m_PrintBasicInfo(1);
        }
    }

// m_PrintModeList() prints each of the elements stored in ModeChoiceList()
    void m_PrintModeList() {
        list<string>::iterator it = ModeChoiceList()->begin();
        while (it != ModeChoiceList()->end()) {
            cout << *it << endl;
            it++;
        }
    }

// m_ChooseModeFromList() provides a dynamic method for a user to choose which Mode's information will be printed
    virtual void m_ChooseModeFromList() {
        int Input = 0;
        cout << "Please input your choice." << endl;
        cin >> Input;
        cout << endl;
        list<Mode*>::iterator it = Modes()->begin();
        switch (Input) {
        case 1: if (it != Modes()->end()) { (*it)->m_PrintBasicInfo(Input); it++; break; }
                else { m_ChooseInvalidModeFromList(); break; }
        case 2: if (it != Modes()->end()) { (*it)->m_PrintBasicInfo(Input); it++; break; }
                else { m_ChooseInvalidModeFromList(); break; }
        case 3: if (it != Modes()->end()) { (*it)->m_PrintBasicInfo(Input); it++; break; }
                else { m_ChooseInvalidModeFromList(); break; }
        case 4: if (it != Modes()->end()) { (*it)->m_PrintBasicInfo(Input); it++; break; }
                else { m_ChooseInvalidModeFromList(); break; }
        case 5: if (it != Modes()->end()) { (*it)->m_PrintBasicInfo(Input); it++; break; }
                else { m_ChooseInvalidModeFromList(); break; }
        case 0: cout << "Returning to previous menu..." << endl; break;
        default: m_ChooseInvalidModeFromList(); break;
        }
    }

// m_ChooseInvalidModeFromList() prints an invalid option message, and returns to m_ChooseModeFromList()
    void m_ChooseInvalidModeFromList() {
        cout << "Invalid option. Please choose a valid mode." << endl;
        m_ChooseModeFromList();
    }
};

I also have an example child class, belonging to an Weapon, which I am using to test the code's viability and execution. It is as detailed below:

class RailGunModeInformation : public ModeInformation {
public:
    Mode* pMode1() const { return new RailGunMode1(); }
    Mode* pMode2() const { return new RailGunMode2(); }
    list<string>* ModeChoiceList() const { return new list<string>; }
    list<Mode*>* Modes() const { return new list<Mode*>; }
};

Right now, it compiles, but I keep getting an "Unhandled exception at 0x0FC3CAB6 (ucrtbased.dll) in Factory Method.exe: An invalid parameter was passed to a function that considers invalid parameters fatal." error. No matter how much I try to rework this while keeping its overall function, I can't seem to get it to work, getting a load of different errors. So I figured I'd ask the experts for some help, as I've spent hours banging my head against the wall trying to jump over this hurdle.

Wayyyy to much stars in your code. Your still learning C++: don't work with naked pointers. Try and use stack object rather than pointers to dynamically allocated objects. If needed, learn and use references. If you _really_ need pointers, learn about smart pointers. — YSC, Jun 13 '18 at 12:11
I assume tons of memory leaks here: you create objects with `new` almost everywhere, and I cannot see a single `delete`. Raw pointers should be reserved to low level classes that internally do resource management like containers. All other classes should rely on containers and smart pointer. — Serge Ballesta, Jun 13 '18 at 12:19
To understand your error it is necessary see running context. But some question about code: Why you use that interface : virtual list* ModeChoiceList() const? For each call of this function you create new empty list, and I dont see where you delete it. Is this behavior such as you want? May you need something like that; class A { list modeChoiceList; virtual list& ModeChoiceList() const { return modeChoiceList; } } — Andrii Rallo, Jun 13 '18 at 12:24
Each time you call `Modes()` you return a new (and empty!) list. So every call to `it = Modes()->begin(); it != Modes()->end();` is comparing iterators into two *different* lists. — Bo Persson, Jun 13 '18 at 12:27
You don't need a separate class `RailGunModeInformation`. An *object* `ModeInformation railgun_modes` which is constructed from `Mode railgun_mode_1;` and `Mode railgun_mode_2;`. `ModeInformation` needs a *data member* `list m_modes` (**not** a member function returning `new list`, which is an *empty list*) — Caleth, Jun 13 '18 at 12:51

Surt · Answer 1 · 2018-06-13T13:07:33.887

Your first problem is this

void m_CreateModes() {
    if (!pMode1() == NULL) { Modes()->push_back(pMode1()); }
    if (!pMode2() == NULL) { Modes()->push_back(pMode2()); }
    if (!pMode3() == NULL) { Modes()->push_back(pMode3()); }
    if (!pMode4() == NULL) { Modes()->push_back(pMode4()); }
    if (!pMode5() == NULL) { Modes()->push_back(pMode5()); }
}

As mentioned in the comments Modes() return a new list each time but the result from the puch_back is not accessable after the next '}'

I'll just use C++11 to remove some of more tedious aspects of the code.

using ModeList = list<Mode*>;

ModeList m_CreateModes() {
    ModeList modes = Modes();
    if (!pMode1() == NULL) { modes->push_back(pMode1()); }
    if (!pMode2() == NULL) { modes->push_back(pMode2()); }
    if (!pMode3() == NULL) { modes->push_back(pMode3()); }
    if (!pMode4() == NULL) { modes->push_back(pMode4()); }
    if (!pMode5() == NULL) { modes->push_back(pMode5()); }
    return modes;
}

Now modes live until exit of the function and is returned, the caller must now store the returned value or the information is lost. Alternatively if if you want to reuse the values returned modes could be a member variable of the base class.

Other issues, this could be misunderstood

    if (!pMode1() == NULL) { modes->push_back(pMode1()); }

Lets say pMode1() returns NULL, then you get and NULL is defined as (0)

    if (!NULL == NULL) { Modes()->push_back(pMode1()); }

    if (true == 0) { Modes()->push_back(pMode1()); }

and if it returns something not NULL

    if (!0xdeadbeef == NULL) { Modes()->push_back(pMode1()); }

    if (false == 0) { Modes()->push_back(pMode1()); }

So what you really wanted was, replace NULL with nullptr (C++11)

    if (!pMode1() == nullptr ) { modes->push_back(pMode1()); }

Now you get a warning that bool is not a nullptr_t and you should write

    if (!(pMode1() == nullptr) ) { modes->push_back(pMode1()); }

or

    if (pMode1() != nullptr ) { modes->push_back(pMode1()); }

or even

    if (pMode1()) { modes->push_back(pMode1()); }

Other notes

m_ChooseModeFromList is likely missing a loop.

the repeated use of the same code with just a number in difference tells me it should be an array of some sort, std::array or std::vector.

the use of raw pointers is usually wrong, but not always.

a good self help is using a debugger to step through the code.

Thank you for your help, however, I'm still having some problems. I've changed the m_CreateModes() and m_CreateModeChoiceList() functions according to your advice, and fixed the resulting code accordingly.However, now I'm running into this: "Unhandled exception at 0x76F04A18 (ntdll.dll) in ProgramName.exe: 0xC00000FD: Stack overflow (parameters: 0x00000001, 0x00802FA8).", in the RailGunModeInformation class, at the following lines: ModeList Modes() { ModeList ListMode; ListMode = m_CreateModes(); return ListMode; } — S. Marques, Jun 13 '18 at 15:54
@S.Marques, now is the time to use your debugger, it will be "obvious" where the problem is. I would guess that you leak memory another place too, consider using less 'new' and more std::vector, and if pointers can't be avoided try using std::shared_ptr. — Surt, Jun 13 '18 at 17:47

C++: Problems with inheritance and list functions as members of a class

1 Answers1