4
\$\begingroup\$

I've implemented a stack using a singly linked list and would like to know if there are any things I could improve on.

StackList.h:

#ifndef STACKLIST_H
#define STACKLIST_H

#include <iostream>

class Node
{
public:
    Node() : data(0), next(nullptr) {}
    Node(int theData, Node *newPtrToNext)
        : data(theData), next(newPtrToNext) {}
    Node* getNext() const { return next; }
    int getData() const { return data; }
    void setData(int theData) { data = theData; }
    void setNext(Node *newPtrToNext)
    {
        next = newPtrToNext;
    }
    ~Node() {}
private:
    int data;
    Node *next;
};

class StackList
{
public:
    //default constructor
    StackList();

    //copy constructor
    StackList(const StackList& otherStack);

    //overloaded assignment operator
    StackList& operator=(const StackList& otherStack);

    //destroyStack
    void destroyStack();

    //destructor
    ~StackList();

    //size
    int size() const;

    //push
    void push(int newVal);

    //pop 
    void pop();

    //top
    int top() const;

    //empty
    bool empty() const;

private:
    Node * topOfStack;
    int count;
};

#endif

StackList.cpp:

#include "StackList.h"

//default constructor
StackList::StackList()
{
    topOfStack = nullptr;
    count = 0;
}

//copy constructor
StackList::StackList(const StackList& otherStack)
{
    Node* temp = otherStack.topOfStack;
    topOfStack = new Node(temp->getData(), nullptr);
    Node * prev = topOfStack;
    temp = temp->getNext();

    while (temp != nullptr)
    {
        Node * current = new Node(temp->getData(), nullptr);
        prev->setNext(current);
        prev = prev->getNext();
        temp = temp->getNext();
    }
    count = otherStack.count;
}

//overloaded assignment operator
StackList& StackList::operator=(const StackList& otherStack)
{
    if (&otherStack != this)
    {
        destroyStack();
        Node* temp = otherStack.topOfStack;
        topOfStack = new Node(temp->getData(), nullptr);
        Node * prev = topOfStack;
        temp = temp->getNext();

        while (temp != nullptr)
        {
            Node * current = new Node(temp->getData(), nullptr);
            prev->setNext(current);
            prev = prev->getNext();
            temp = temp->getNext();
        }
        count = otherStack.count;
    }
    else
        std::cerr << "Error. Attempted assignment to itself." << std::endl;

    return *this;
}

//destroyStack
void StackList::destroyStack()
{
    Node  *temp = topOfStack;

    while (topOfStack != nullptr)
    {
        topOfStack = topOfStack->getNext();
        delete temp;
        temp = topOfStack;
    }

    count = 0;
}

//destructor
StackList::~StackList()
{
    destroyStack();
}

//size
int StackList::size() const
{
    return count;
}

//push
void StackList::push(int newVal)
{
    topOfStack = new Node(newVal, topOfStack);
    count++;
}

//top
int StackList::top() const
{
    return topOfStack->getData();
}

//pop
void StackList::pop()
{
    if (topOfStack != nullptr)
    {
        Node * temp = topOfStack;
        topOfStack = topOfStack->getNext();
        delete temp;
        count--;
    }
    else
        std::cerr << "Stack is empty. No value to pop." << std::endl;
}

//empty
bool StackList::empty() const
{
    return count == 0;
}
\$\endgroup\$
5
\$\begingroup\$
  • This only works with int. Did you consider taking a template parameter so it can work with other types too?

  • Don't compare to nullptr. Instead use if (node) or if (!node).

  • You're very inconsistent with how you declare pointers. You have
    Node* foo, Node * foo and Node *foo. Generally in C++ this is seen as part of the type so you should prefer the first variant.

  • You're using a member initialization list for Node but not for StackList. Why?

  • Prefer an early out when possible to avoid the build-up of arrow code.

  • Prefer prefix (++foo) over postfix (foo++) operator

  • Your comments add nothing to the code and can be removed.

  • Opinion based but you should not omit braces as it can lead to hard to find bugs.

  • The destructor can be implemented in terms of pop().

  • Node should be part of StackList as no one outside the class needs to access it. You could also simplify it by making it a struct and directly accessing the members.

  • You're violating the rule of three/five/zero by having a destructor/copy assignment operator but no move constructor or operator.

  • Your copy constructor does too much work. You can just iterate over the passed list and push(). Same for your assignment operator, look into the copy and swap idiom.

\$\endgroup\$
  • \$\begingroup\$ Good criticism. Though I would add that auto also has many benefits. \$\endgroup\$ – Deduplicator Apr 25 '18 at 10:55
  • \$\begingroup\$ I would further consider to introduce a NodePtr type to replace the Node* variants. And suggest to use std::unique_ptr<T> if possible. \$\endgroup\$ – moooeeeep Apr 25 '18 at 12:38
  • \$\begingroup\$ I personally prefer to see if (ptr != nullptr) as opposed to if (!ptr), could you explain your reasoning for your second comment? \$\endgroup\$ – musicman523 Apr 25 '18 at 14:11
  • \$\begingroup\$ @musicman523 What is your reasoning for using the verbose version? Enhanced readability due to not missing the negation? If so just use if (not ptr). \$\endgroup\$ – yuri Apr 25 '18 at 14:20
  • \$\begingroup\$ I guess I don't have a good reason, it's just what I've always done. To me it has been more clear because a pointer is not a boolean type and a conditional expression should result to a boolean \$\endgroup\$ – musicman523 Apr 25 '18 at 14:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.