CODEWITHSUNDEEP
c++pointersdynamiclinked-list
Coder95
Coder95

Reputation: 131

C++ problem with push() and pop() methods for dynamic LinkedList using pointers

First of all I want to create an dynamic LinkedList with Pointers without using STL. It is an exercise to understand how pointers work in C++.

My problem is that my push() method doesn't create a FifoElement like I would expect. When I runstart the program it shows me nothing after I use the pop() method. An example of my main is below.

To push new objects into the List I'm overloading the <<operator and to pop the first element from the list i'm overloading the >>operator

My two classes are Fifo and FifoElement

Fifo:

template <class T>
class Fifo
{
    public:
        Fifo();

        void push(const T&);
        T pop();

        Fifo& operator<<(const T&);
        Fifo& operator>>(T&);


    private:
        FifoElement<T> *top;
};

FifoElement:

template <class T>
class Fifo;

template<class T>
class FifoElement
{
    friend class Fifo<T>;

    public:
        FifoElement();

    private:
        T value;
        FifoElement<T> *next;
};

snippet of code for push method:

...
template <class T>
void Fifo<T>::push(const T& val){
    //creates new Element
    FifoElement<T> *newElem = new FifoElement<T>(); //<= I think my problem is here but I am not sure..

    //inserts new value into Element
    newElem->value = val;

    //If Fifo/List is empty: Element is top...
    if(top == nullptr){
        top = newElem; //...füge am Anfang ein
        return;
    }

    //or:
    //when List has elements go to the end and then insert element at the end
    FifoElement<T> *tmpElem = top;
    while(tmpElem->next != nullptr){
        tmpElem = tmpElem->next;
        cout << tmpElem << endl;
    }
    //set new Element as next of the last element in the list 
    tmpElem->next = newElem;
}

template <class T>
Fifo<T>& Fifo<T>::operator<<(const T& val){
    push(val);
    return *this;
}
...

and the pop() method part:

...
template <class T>
T Fifo<T>::pop(){
    //should read the first element of the list and delete it...
    FifoElement<T> *tmpElem = nullptr;
    FifoElement<T> *returnElem = nullptr;

    //if top is empty it means that the list is empty...
    if(top == nullptr){
        cout << "Liste ist leer!" << endl;
        returnElem = 0;
        return returnElem->value;
    }
    //the new element is the new top
    else if(top->next != nullptr){
        top = top->next;
        returnElem = tmpElem;
        //hier wird Element gelöscht!
        delete tmpElem;
        tmpElem = nullptr;
        //
        return returnElem->value;

    }

    //if only top exists then return it and delete it after that
    else{
        delete top;
        top = nullptr;
        returnElem = tmpElem;
        delete tmpElem;
        tmpElem = nullptr;
        return returnElem->value;
    }
}

template <class T>
Fifo<T>& Fifo<T>::operator>>(T& val){
    pop();
    return *this;
}
...

an example of my main would be like this:

    ...
    int main() {
    Fifo<string> test;
    string ex = "Hey stackoverflow whatsup? :)";
    string ex2 = "can anyone help me?";
    test << ex;
    test << ex2
    test.pop(); //output here should be: "Hey, stackoverflow whatsup? :)"
    test.pop(); //output here should be: "can anyone help me?"
    ...
    return 0;
}

I hope that I didn't forget anything. It would be nice if someone could help me out. I'm sitting on that program since 3 days :(

Upvotes: 0

Views: 245

Answers (1)

Thomas
Thomas

Reputation: 5138

Your pop method was too complex and contained errors, especially with memory handling. Doing something like

returnElem = tmpElem;
delete tmpElem;

is wrong because returnElem just became a dangling pointer since you did not create a copy of the FifoElement<T> and both pointers are pointing to the same element. I've rewritten it as

template <class T>
T Fifo<T>::pop(){
    T value;
    if( top ){
        auto const old = top;
        top = top->next;
        value = old->value;  
        delete old;
    } else {
        cerr << "error - queue empty";
    }
    return value;
}

With that change your sample code works. I have not checked it for additional errors. See working example here

Upvotes: 2

Related Questions