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

C++ Stack using template

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

Problem

I'm learning C++, so I wrote a stack with the help of templates. I'm wondering if there is anything I can improve or if I did something vastly wrong.

Stack.h

#pragma once
#include 

template 
struct Node {
    Node(Type data, Node* next) 
        : data(data), next(next) {}
    Node* next;
    Type data;
};

template 
class Stack
{
public:

    Stack() : length(0), topNode(NULL) {
    }

    ~Stack() {
        while (!isEmpty()) {
            pop();
        }
    }

    void push(Type data) {
        Node* newNode = new Node(data, topNode);
        topNode = newNode;
        ++length;
    }

    Type pop() {
        if (!isEmpty()) {
            Node* popped = topNode;
            Type poppedData = popped->data;
            topNode = popped->next;
            --length;
            delete popped;
            return poppedData;
        }

        throw new std::exception("Stack underflow!");
    }

    bool isEmpty() {
        return length == 0;
    }

    void print() const {
        Node* tempTop = topNode;
        while (tempTop != NULL) {
            std::cout data next;
        }
    }

    int count() const {
        return length;
    }

private:
    Node*         topNode;
    int                 length;

};


Main.cpp

#include 
#include "Stack.h"

using namespace std;

int main() {
    Stack myStack;

    // Push some values
    myStack.push(2);
    myStack.push(4);
    myStack.push(8);
    myStack.push(16);
    myStack.push(32);

    // We pop the 32
    myStack.pop(); 

    // Display count after the pop
    int lastPopped = myStack.pop();
    cout << "Popped value: " << lastPopped << ", Count: " << myStack.count() << endl; 

    // Print whole stack
    cout << endl << "Stack print: " << endl;
    myStack.print(); 

    // Exit program when the 'any' key is pressed.
    system("PAUSE");
    return 0;
}

Solution

If you provide a destructor you should handle copying and assigning. In other words follow the Rule of 3 (or 5 if you care about move semantics). You can also disallow them but then there should be move constructor and assignment.

Otherwise you will get in trouble with double freeing when calling a function void foo(Stack s).

empty() doesn't change the stack. So it should also be const.

Consider adding a peek() method that returns a reference to the top value of the stack (with const and non-const version).

T& peek()
{
    return topNode->data;
}

const T& peek() const
{
    return topNode->data;
}


The reason for using the reference is to avoid a unnecessary copy.

Having a print method interacting with the console is not a good idea. What if you want to show the contents in a gui instead?

Instead add iterators to be able to inspect what is on the stack without having to capture the output that print() generates.

Code Snippets

T& peek()
{
    return topNode->data;
}

const T& peek() const
{
    return topNode->data;
}

Context

StackExchange Code Review Q#102393, answer score: 11

Revisions (0)

No revisions yet.