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

Simple implementation of a generic stack

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

Problem

I'm fairly new to C++ programming. To help I've been writing my own data structures for practice. I'd liked this to be judged as if it was written by a professional, and receive honest feedback. I already have a few concerns.

-
Am I managing memory correctly? I don't believe I have any leaks. A lot of advice I get when using pointers is to use smart pointers, but raw pointers are fine when the class encapsulates all pointers correct?

-
I'm unhappy my `

#include 

template 
class BBStack {
public:
BBStack(T type) {
    head = new Node(type);
}

virtual ~ BBStack() {
    Node* temp = head;
    while (head != nullptr) {
        temp = head->next;
        delete head;
        head = temp;
    }
    delete temp;
}

void push(T type) {
    Node *newNode = new Node(type, head);
    head = newNode;
}

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

T pop() {
    if (head == nullptr) {
        std::coutdata;
        head = head->next;
        delete temp;
        return result;
    }
}

std::ostream& peekAll(std::ostream& out) const {
    if (head == nullptr) return out data next;
    }

    delete temp;
    return out;
}

private:
    struct Node {
        Node(T type, Node* _next = nullptr) : data(type), next(_next) {}
        Node(Node* node) : data (node->data), next (node->next) {}

        T data;
        Node* next;
    };
    Node* head;

};

template 
std::ostream& operator& stack) {
    return stack.peekAll(out);
}

Solution

Ownership

Yes raw pointers are fine if you are not transferring ownership (Is the class responsibility to delete them).

Pop function

Also I am not sure if this will compile for any data other than pointers and types convertible to 0 since you return NULL. (Also is better to use nullptr consistently).

T pop() {
    if (head == nullptr) {
        std::coutdata;
        head = head->next;
        delete temp;
        return result;
    }
}


IMO it will be better to throw a underflow exception since is the client responsibility to check that is not empty before. Also avoid returning the data since we will be throwing an exception (For more info on this check out this article http://ptgmedia.pearsoncmg.com/images/020163371x/supplements/Exception_Handling_Article.html)

void pop() {
    if (head == nullptr) 
        throw std::underflow_error("underflow");

    Node *temp = head;
    head = head->next;
    delete temp;   
 }


So the client will have to call peek before pop to get the same effect.

Parameters:

In most parameter like this (T type) you should change it to (const T& type)
to avoid copying. Even better would be to provide an overload for rvalue references like this (T&&)

peekAll function

This function seams kind of weird. Why are you deleting temp? . It seams that after the while loop temp should always be nullptr. And aren't you just writing the stack on the stream. I would just omit this function and move it to the << operator overload.

Also you should implement an empty function.

Code Snippets

T pop() {
    if (head == nullptr) {
        std::cout<<"Error stack is empty"<<std::endl;
        return NULL;
    } else {
        Node *temp = head;
        T result = temp->data;
        head = head->next;
        delete temp;
        return result;
    }
}
void pop() {
    if (head == nullptr) 
        throw std::underflow_error("underflow");

    Node *temp = head;
    head = head->next;
    delete temp;   
 }

Context

StackExchange Code Review Q#105216, answer score: 4

Revisions (0)

No revisions yet.