0
\$\begingroup\$

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";

}
\$\endgroup\$
2
\$\begingroup\$
  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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – BlooB Jun 25 '17 at 0:14
  • \$\begingroup\$ 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 \$\endgroup\$ – BlooB Jun 25 '17 at 0:26

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.