Implementing CircularQueue using Linked List in C++

I'm currently doing a project, requiring me to implement a CircularQueue data structure in C++:

```
template class CircularQueue {

template struct CQNode {
Type head;
CQNode* tail;

CQNode() {
this->head = NULL;
this->tail = NULL;
}

CQNode(Type head, CQNode* tail = NULL) {
this->head = head;
this->tail = tail;
}

};

private:

CQNode* front;
CQNode* rear;
CQNode** circularQueue = NULL;
int MAX = 0;
int currentSize = 0;

protected:

public:

/****/
/ CONSTRUCTORS /
/****/

/**
* @brief
*
* [DETAILED DESCRIPTION]
*
* @param capacity integer size capacity of CircularQueue object to be instantiated
*/
CircularQueue(int capacity) {

// Set the maximum queue capacity to the size passed to constructor
this->MAX = capacity;

// Locally set the limit (to be used as array of CQNode size) to MAX
const int limit = this->MAX;

// Create a pointer array of CQNode with size limit and assign to this->circularQueue
this->circularQueue = new CQNode*[limit];

// Loop over queue capacity, implementing CircularQueue type structure
for (int i = 0; i circularQueue[i] = new CQNode();

// if the value of i is greater than 0, assign the tail of the (i-1)'th element
// of circularQueue to i'th element of circularQueue, thereby implementing the Queue
// FIFO type data structure
if (i > 0) {
this->circularQueue[i - 1] = this->circularQueue[i];
}

// otherwise, if i is equal to the capacity - 1 (i.e. the last elementt of circularQueue array),
// then set the tail of the last element of circularQueue to the first element of circularQueue,
// thereby implementing the circular nature of this queue data structure
else if (i == (limit - 1)) {
this->circularQ

A few other points:

-
Since you have defined everything inline inside the class declaration, it would be nice to add one level of indentation to everything inside the class, otherwise the methods don't look like they belong to a class.

-
MAX is not a great name. Actually, the constructor assigns a capacity variable to it, so why not rename MAX to capacity and clearly indicate that this is the maximum capacity of the backing array? Also, if you properly initialize your member data in the constructor initializer list, you can declare capacity (or former MAX) as a constant (const int capacity;), which is nice, since it doesn't seem like you've devised this data structure to ever change capacity after construction. Quick example:

// This is what a constructor initializer list looks like.
// It is a comma separated list of members. This is unique to C++
// and the recommended way of initializing member data in a constructor.
CircularQueue(int cap)
    : capacity(cap)
    , currentSize(0)
    , circularQueue(new CQNode*[capacity])
{
    // stuff ...
}

-
this-> qualifying member access should be avoided. It's not mandatory as it is in some other languages, so it's only adding verbosity to your code. But also, this qualifying member access can hide problems of name shadowing, which is a very bad practice. Each entity should have a unique name.

-
Prefer using nullptr instead of NULL. Very likely that your compiler is C++11 compliant. If not, see to update it, there are several free alternatives out there. nullptr is the default null pointer literal for C++11 and above.

-
As mentioned by @Zulan, declaring a template class inside a template class with the same parameter name for the template is incorrect (i.e.: template for both CircularQueue and CQNode). If I recall, the Visual Studio compiler did allow this kind of malformed code in the past, so perhaps that's why it is working for you... You don't have to make CQNode a template. It can directly access the parent Type from CircularQueue, so this is an easy fix.

-
protected section of your class is empty, so you can remove that tag.

-
Those header comments are way too verbose. Unless you really have a requirement for that kind of mechanical code commenting, don't do it. It's just distracting from the actual code.

-
You never freed memory in the destructor, which would be the place for it, so your class is leaking resources. However, in modern C++ it is rare to manually manage memory like that. The standard library offers containers meant to keep track of mechanical and error-prone resource management for you. You can either use a std::vector for the node backing store or at the very least a std::unique_ptr with array storage, to make cleanup automated.

-
Methods that don't modify member data should be const to allow them to be called on a const object and to also clearly signify that to readers. You can learn more about const member functions in here.

-
A huge architectural improvement and code cleanup would come from dropping this linked list setup and using a plain array. Since the queue is fixed size, you can very easily implement the ring buffer with an array of values and a head and tail indexes. This will make your code much simpler and more efficient, since you'll get rid of a ton of pointer indirections.

StackExchange Code Review Q#109463, answer score: 4