Queue/stack/node class Implementation

Question

Below there are 3 short header files for queue, stack, and node implementation. I do understand that the STL has this capability(I am just doing it for practice). Please criticize the coding style, and things that you might thing i can/should do better. thank you. Note: since they are pretty short and Node class is used in the other two i posted all in here.

Node.h:
----------------------------------------------------
// Node.h
// 1-way linked node for use in simple integer Queue
#ifndef NODE_H
#define NODE_H
class Node
{
public:
 Node();
 Node(int);
 int data;
 Node *next;
};
#endif

//-----------------------------//

 Node.cpp:
-----------------------------------------
#include "Node.h"
Node::Node()
{
 data = -1;
 next = nullptr;
}
Node::Node(int x)
{
 data = x;
 next = nullptr;
}

---

fmStack.h:
------------------------------------------------
#ifndef _FMSTACK_H
#define _FMSTACK_H
#include <iostream>
#include "Node.h"
namespace fm
{
 class fmStack
 {
 Node *top;
 public:
 fmStack();
 ~fmStack();
 void push(int);
 void pop();
 void dumpStack();
 };
}
#endif /* _FMSTACK_H */

//-------------------------------//

fmStack.cpp:
-----------------------------------------------
#include "fmStack.h"
using namespace fm;
//-------Private Methods----------
//-------Public Methods-----------
fmStack::fmStack()
{
 top = nullptr;
}
fmStack::~fmStack()
{
 top = nullptr;
}
void fmStack::push(int x)
{
 Node *node = new Node;
 node->data = x;
 node->next = nullptr;
 if (top != nullptr)
 node->next = top;
 top = node;
}
void fmStack::pop()
{
 Node *node;
 if (top == nullptr)
 std::cout << "Stack is empty" << std::endl;
 node = top;
 top = top->next;
 std::cout << "Poped from stack" << std::endl;
 delete node;
}
void fmStack::dumpStack()
{
 Node *node = top;
 if (top == nullptr)
 std::cout << "Stack is empty" << std::endl;
 while (node != nullptr)
 {
 std::cout << "data in stack at current position is" << node->data << std::endl;
 node = node->next;
 }
}

---

fmQueue.h:

---

#ifndef _FMQUEUE_H
#define _FMQUEUE_H
#include <iostream>
#include "Node.h"
namespace fm
{
 class fmQueue
 {
 Node *_head, *_tail;
 void clearbuf();
 public:
 fmQueue();
 ~fmQueue();
 void deQueue(); // uses front to access data, or remove data
 void enQueue(int); // uses back to sort data, or add data
 void dumQueue();
 //int peek(); // get a copy of the front data without removing it
 bool isEmpty();
 };
}
#endif /* _FMQUEUE_H */

//---------------------------------//

fmQueue.cpp:

---

#include "fmQueue.h"
using namespace fm;
//---------Private Methods--------
void fmQueue::clearbuf()
{
 _head = _tail = nullptr;
}
//--------Public Methods----------
fmQueue::fmQueue()
{
 clearbuf();
}
fmQueue::~fmQueue()
{
 clearbuf();
}
bool fmQueue::isEmpty()
{
 if (_head == _tail && _head == nullptr)
 return false;
 else
 return true;
}
void fmQueue::enQueue(int data1)
{
 Node *tempNode = new Node;
 tempNode->next = nullptr;
 tempNode->data = data1;
 if (_head == nullptr)
 {
 _head = tempNode;
 _tail = tempNode;
 }
 else
 {
 _tail->next = tempNode;
 }
 _tail = tempNode;
}
void fmQueue::deQueue()
{
 Node *tempNode = new Node;
 if (_head == nullptr)
 std::printf("NOOOOP, THE QUEUE IS EMPTY");
 else
 {
 tempNode = _head;
 _head = _head->next;
 std::cout << "the data dequeued is: " << tempNode->data; //add a print statment to see which node was deleted
 delete tempNode;
 }
}
void fmQueue::dumQueue()
{
 Node *tempNode = new Node;
 if (tempNode)
 while (tempNode->next != nullptr)
 {
 std::cout << "Queue :" << tempNode->data;
 tempNode = tempNode->next;
 }
 else
 std::cout << "Nothing to show";
}

Answer 1

1. You should realize, that your stack implementation leaks memory, if a non empty stack goes out of scope. In that case you have to recursively call delete on your nodes.

   To remedy this you should look into std::unique_ptr that handles the memory management for you.

2. Why does your stack doesnt have a function to retrieve the top node?

3. Is there any reason you defined int rather than making a template library?

4. dumpStack is a bad name, as dumping semantically involves throwing away. Maybe printStack?

5. Use correct constructors and initializer lists

   class Node
   {
    public:
    Node() {};
    Node(const int value)
    : data(value) 
    {}
    Node(const int value, Node* nextNode)
    : data(value)
    , next(nextNode) 
    {}
    int data = -1;
    Node *next = nullptr;
   };
   

   You can even simplify this itno a single one

   Node(const int value = -1, Node* nextNode = nullptr)
    : data(value)
    , next(nextNode) 
    {}
    int data;
    Node *next;
   };
   

   Then you push gets way easier

   void fmStack::push(int x)
   {
    Node *node = new Node(x, top); 
    top = node;
   }
   

6. You pop is seriously flawed. If the stack is empty, you still access top. In that case better throw a std::exception or at least do an early return

• BlooB: 1- I have to make a deconstruct method for my Node class. (I think) 2-I Believe in the pop method, node will be pointing to the top.
• BlooB: 4-I will change that 5-I have never looked into creating constructor/initializer that way. I will do so 6-I believe i have to wrap the staments in a else statment