Stack Template Implementation - Need Suggestions

[amitapm]

Hi All,

I am reviving my C++ knowledge; so to start off with, I have implemented a Stack template. The code for Stack template as well as a sample class to use the stack is given below. It throws a StackException when trying a pop operation on empty stack.

I would like to have suggestions from forum members on any improvements, optimizations, the design or any other aspects.


The Stack Template: (Stack.h)

#ifndef _STACK_H_
#define _STACK_H_

#include <iostream>
#include "Exception.h"

template <class T>
class Stack {
    public:
        Stack():top(0) {
            std::cout << "In Stack constructor" << std::endl;
        }
        ~Stack() {
            std::cout << "In Stack destructor" << std::endl;
            while ( !isEmpty() ) {
                pop();
            }
            isEmpty();
        }

        void push (const T& object);
        T pop();
        const T& topElement();
        bool isEmpty();

    private:
        struct StackNode {              // linked list node
            T data;                     // data at this node
            StackNode *next;            // next node in list

            // StackNode constructor initializes both fields
            StackNode(const T& newData, StackNode *nextNode)
                : data(newData), next(nextNode) {}
        };

        // My Stack should not allow copy of entire stack
        Stack(const Stack& lhs) {}

        // My Stack should not allow assignment of one stack to another
        Stack& operator=(const Stack& rhs) {}
        StackNode *top;                 // top of stack

};

template <class T>
void Stack<T>::push(const T& obj) {
    std::cout << "In PUSH Operation" << std::endl;
    top = new StackNode(obj, top);
}

template <class T>
T Stack<T>::pop() {
    std::cout << "In POP Operation" << std::endl;
    if ( !isEmpty() ) {
        StackNode *topNode = top;
        top = top->next;
        T data = topNode->data;
        delete topNode;
        return data;
    }
    throw StackException ("Empty Stack");
    //return 0;
}

template <class T>
const T& Stack<T>::topElement() {
    std::cout << "In topElement Operation" << std::endl;
    if ( !isEmpty() ) {
        return top->data;
    }
}

template <class T>
bool Stack<T>::isEmpty() {
    if (top == 0) {
        return true;
    }
    else {
        return false;
    }
}

#endif 

Example usage of Stack (StudentStack.cpp)

#include "Stack.h"

class Student {
    private:
        std::string name;
        std::string course;
        int age;

    public:
        Student(std::string n, std::string c, int a) : name(n), course (c) {
            //std::cout << "In STUDENT constructor" << std::endl;
            age = a;
        }

        ~Student() {
            //std::cout << "In STUDENT destructor" << std::endl;
        }

        std::string getName() {
            return name;
        }

        std::string getCourse() {
            return course;
        }

        int getAge() {
            return age;
        }
};

int main () {
    Stack <Student> studentStack;

    Student s1( "Student1" , "Course1", 21);
    Student s2( "Student2" , "Course2", 22);

    studentStack.push ( s1 );
    studentStack.push ( s2 );

    try {
        Student s3 = studentStack.pop();
        std::cout << "Student Details: " << s3.getName() << ", " << s3.getCourse() << ", " << s3.getAge() << std::endl;

        Student s4 = studentStack.pop();
        std::cout << "Student Details: " << s4.getName() << ", " << s4.getCourse() << ", " << s4.getAge() << std::endl;

        Student s5 = studentStack.pop();
        std::cout << "Student Details: " << s5.getName() << ", " << s5.getCourse() << ", " << s5.getAge() << std::endl;
    } catch (StackException& se) {
        se.what();
    }
}

The StackException class (Exception.h):

#include <iostream>
#include <string>

class StackException {
    public:
        StackException(std::string s) : str(s) {}
        ~StackException() {}
        void what() {
            std::cout << str << std::endl;
        }

    private:
        std::string str;
};

[kbw]

That looks pretty good to me. The comments are minor things.


If StackException derived from std::runtime_error, you wouldn't need to hold the string or implement what().

You ought ot catch by const ref rather than by ref.

This

bool Stack<T>::isEmpty() {
    if (top == 0) {
        return true;
    }
    else {
        return false;
    }
}

can be written as this:

bool Stack<T>::isEmpty() {
    return top == 0;
}

Don't provide implementations for the copy stuff as in:

        // My Stack should not allow copy of entire stack
        Stack(const Stack& lhs);

        // My Stack should not allow assignment of one stack to another
        Stack& operator=(const Stack& rhs);

[amitapm]

Thanks kbw.

The copy constructor and assignment operators are declared private. I'll provide the empty implementation.

I'll try out StackException to derive it from std::runtime_error.

Is there a scope of improvement in the pop method? It calls Student destructor twice - once while deleting topNode and second time when data goes out of scope on returning. The Student object - s3 and s4 will be copy constructed from the Student object returned from pop.

[amitapm]

kbw, I have implemented your suggestions and it looks much better now.

Oh... And I just saw that I have provided empty implementation to copy constructor and assignment operator. Did you mean to say not to provide implementation at all? Why is that so? What difference will it make?

[kbw]

It's sort of belt and braces.

When you declare them private, it causes the compiler to tell you when a copy is being attempted.

When you remove the implementation, it causes the linker to tell you that someone managed to use them (maybe a friend).

[amitapm]

Thanks!

I would also like to have some suggestions for the next step - implementing some complex scenario - something related to inheritance, virtual mechanism. Need some real world scenario ideas.