2
\$\begingroup\$

I wrote my implementation to Queue Linked List based. And I need a review for it to improve it and improve my coding skill. I also will put this implementation on my GitHub account.

//======================================================
// Author      : Omar_Hafez
// Created     : 30 July 2022 (Saturday)  8:02:31 AM
//======================================================

#include <iostream>
#include <memory>

template <class T>
class Queue {
    
    private:
        struct Node {
            T value;
            std::shared_ptr<Node> next;
            Node (T data, std::shared_ptr<Node> ptr) : value(data), next(ptr) {}
        };
        
        int size_value = 0;
        std::shared_ptr<Node> back;
        std::shared_ptr<Node> front;

    public:
        enum QueueOpStatus { FailedQueueEmpty = -1, FailedQueueFull = -2, OK = 1 };

        //  this constructor is to keep the consistency with the array based implementation
        Queue(int MAX_SIZE = 1000000) {}

        QueueOpStatus push(T const& t) {
            if(full()) return FailedQueueFull;
            if(front) {
                back->next = std::make_shared<Node>(t, nullptr);
                back = back->next;
            } else {
                front = std::make_shared<Node>(t, nullptr);
                back = front;
            }
            size_value++;
            return OK;
        }

        QueueOpStatus pop() {
            if (empty()) return FailedQueueEmpty;
            front = front->next;
            size_value--;
            return OK;
        }

        bool empty() const { return size_value == 0; }

        bool full() const { return 0; }

        int size() const { return size_value; }

        T top() const { 
            if(empty()) {
                throw "Queue is empty";
            }
            return front->value; 
        }

        void clear() {
            while(!empty()) {
                pop();
            }
        }
};
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • push could be streamlined: back will always point to the new node no matter what:

      auto new_node = make_shared<Node>(t, nullptr);
      if (front) {
          back->next = new_node;
      } else {
          front = new_node;
      }
      back = new_node;
    
  • The emptiness of the queue is tested differently in push (which tests if (front)) versus pop/top (which tests empty() aka size_value == 0). Technically nothing is wrong, but it gives an uneasy feeling. Better use an uniform test.

  • top throwing an exception could be an overkill. Consider returning a std::pair<bool, T>, or std::optional<T>.

  • size_value shall be size_t.

  • To repeat another review, enqueue/dequeue are far better than push/pop.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.