HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Linked stack implementation in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationlinkedstack

Problem

Please feel free to be as harsh as possible.

Implementation file:

#include "Stack.h"
#include 
using namespace std;

// Initializes an empty Stack.
Stack::Stack() {
    this->head = new node();
    this->num_elements = 0;

    // set default values for head
    this->head->next = 0;
    this->head->value = 0;
}

// Cleans up memory before the Stack's destruction.
Stack::~Stack() {
    while (this->head->next != 0) {
        pop();
    }
    delete this->head;
}

// Pushes value onto the top of the stack.
void Stack::push(int value) {
    node* newTopNode = new node();
    newTopNode->value = value;

    // currentTopNode may be null if stack is empty
    node* currentTopNode = this->head->next;

    this->head->next = newTopNode;
    newTopNode->next = currentTopNode;

    this->num_elements++;
}

// Pops the top-most value off the stack and returns its value.
int Stack::pop() {
    if (this->head->next == NULL) {
        cout head->next;

        // save the value of the node before deleting it
        int topNodeValue = topNodeToRemove->value;

        this->head->next = this->head->next->next;

        delete topNodeToRemove;
        this->num_elements--;

        return topNodeValue;
    }
}

// Returns the value at the top of the stack.  Works similarly to pop(), but
// retains the internal structure of the stack.  That is, it does not remove
// the top-most element.
int Stack::getTopElement() const {
    if (this->head->next == NULL) {
        cout head->next->value;
    }
}

// Returns the number of elements currently in the stack.
int Stack::size() const {
    return this->num_elements;
}


Header file:

#ifndef STACK_H_
#define STACK_H_

class Stack {
public:
    Stack();
    ~Stack();
    void push(int);
    int pop();
    int getTopElement() const;
    int size() const;

private:
    struct node {
        int value;
        node* next;
    };

    node* head;
    int num_elements;
};

#endif // STACK_H_

Solution

-
Do not use using namespace std.

-
In the constructor and push(), it looks like you're calling new on a function. It should be node.

-
Prefer nullptr to NULL and 0 for the node pointers if you're using C++11. Otherwise, you should choose either NULL or 0 for node pointers and keep it consistent.

-
You don't need this-> everywhere as the code already has the current object in scope.

-
It's needless to call pop() in the destructor. For one thing, pop() returns a value. You should just be using delete.

Instead, have head point to the first element, loop through the stack (stopping when NULL is hit) and call delete on each node. With this, you won't need to delete head at the end.

-
pop() and getTopElement() might be best as void. Here, a 0 is returned if the list is empty. Sure, no errors happening there. But then the calling code will take it as an element 0, never knowing whether or not the stack is empty. If you make them void, you can simply return if the stack is empty. As for retrieving an element, that's why you have getTopElement(). Once you get the value with getTopElement(), call pop() after that.

Regarding checking for an empty stack, you could make a public empty() function for calling code. The two functions above should still check for an empty stack, but containers like this usually utilizes such a function. This could go in the header:

bool empty() const { return num_elements == 0; }


-
You could make your class more useful by using templates. This will allow you to populate the stack with any type instead of just ints.

Code Snippets

bool empty() const { return num_elements == 0; }

Context

StackExchange Code Review Q#32210, answer score: 6

Revisions (0)

No revisions yet.