patterncppMinor
Simple implementation of a generic stack
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 `
-
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).
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)
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.
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.