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

Simple stack implementation

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

Problem

I am not experienced in C++, and wrote this code for implementing a stack data structure using a linked list.

Can you please point out any flaws, errors, stupid mistakes, general improvements etc that can make this code better?

#include                                                                       
using namespace std;

struct Node;

struct Node {
    int data;
    Node* next;
};

class Stack {
public:
    Node* first;
    Node* last;

    Stack() {
        first = 0;
        last = 0;
    }

    ~Stack() {
         while (first != 0) { pop(); }
    }

    Stack& push(int value) {
        Node* temp = new Node;
        temp->data = value;
        temp->next = 0;

        if (first == 0) {
            first = temp;
            last = temp;
        } else {
            last->next = temp;
            last = last->next;
        }

        cout data next != last) {
                temp = temp->next;
            }
            cout data next = 0;
        }
        return *this;
    }
};

int main() {
    Stack s;
    s.pop();
    s.push(1);
    s.push(2);
    s.pop();
    s.push(3).push(4);
    s.pop().pop();
    return 0;
}

Solution

A few comments, wrt style.

#include                                                    
using namespace std;


You don't need a forward declaration of struct Node.

//struct Node;


You could add a constructor here just like in class. So your initialization
is simpler.

struct Node {
    int data;
    Node* next;
    Node(int d):data(d), next(0){}
};


It may be a good idea to write an abstract class as an interface, and then write
the concrete class to conform to it. Another idea is to try to templatize Node so that the data can be any type. Also look at the stack class in STL

You may also want top() and empty() methods

on logic: If you are going to implement a stack using a linked list, you don't really need to keep a first and a last. Just keep the reference to the top element. On push, create a new node, set its next to the current top, and set it to top. On pop, set the top to the next of current top, and delete the node. Something along the lines of: (may contain bugs.)

struct Node {
    int data;
    Node* next;
    Node(int v, Node* n):data(v),next(n) {}
};

class Stack {
  public:
    Node* top;
    Stack():top(0){}
    ~Stack() {
      while (top != 0) pop();
    }
    Stack& push(int value) {
      top = new Node(value, top);
      return *this;
    }    
    Stack& pop() {
      if (!top) throw "No nodes to pop.";
      Node* t = top;
      top = top->next;
      delete t;
      return *this;
    }
};

Code Snippets

#include <iostream>                                                   
using namespace std;
//struct Node;
struct Node {
    int data;
    Node* next;
    Node(int d):data(d), next(0){}
};
struct Node {
    int data;
    Node* next;
    Node(int v, Node* n):data(v),next(n) {}
};

class Stack {
  public:
    Node* top;
    Stack():top(0){}
    ~Stack() {
      while (top != 0) pop();
    }
    Stack& push(int value) {
      top = new Node(value, top);
      return *this;
    }    
    Stack& pop() {
      if (!top) throw "No nodes to pop.";
      Node* t = top;
      top = top->next;
      delete t;
      return *this;
    }
};

Context

StackExchange Code Review Q#12155, answer score: 6

Revisions (0)

No revisions yet.