Queue implementation without using built-in data structures

In an effort to brush up my algorithms and data structure skills I have a done a queue implementation without using built in data structures, i.e. using only arrays. Any comments on this code regarding issues, bad/ good practices and good style would be greatly appreciated.

```
#include
#include
#include
#include

class QueueError : public std::runtime_error {
public:
QueueError(std::string str) : std::runtime_error(str) {};
};

class Queue {
struct LLNode {
int val;
LLNode* next;
};
private:
LLNode** queue = 0;
int limit;
int currSize = 0;
LLNode* first;
LLNode* last;
void swap(const Queue& that);
void destroy();
public:
Queue(int size);
void enqueue(int val);
int dequeue();
virtual ~Queue();
Queue& operator=(const Queue& that);
Queue(const Queue& that);
int size();

};

Queue::Queue(int size) {
if (size limit = size;
const int l = this->limit;
queue = new LLNode*[l];
// make it circular
for (int i = 0;i = 0) {
queue[i-1]->next = queue[i];
}
}
queue[l-1]->next = queue[0];
first = 0;
last = 0;
}

void Queue::enqueue(int val) {
if (currSize == limit) {
throw QueueError(std::string("Queue size cannot exceed limit of queue."));
}
++currSize;
if (last != 0) {
last = last->next;
} else {
first = last = queue[0];
}
last->val = val;
}

int Queue::dequeue() {
if (currSize == 0) {
throw QueueError(std::string("Queue size is 0. Queue cannot be dequeued"));
}
currSize--;
int returnVal = first->val;
if (currSize != 0) {
first = first->next;
} else {
first = last = 0;
}
return returnVal;
}

int Queue::size() {
return currSize;
}

void Queue::destroy() {
if (queue == 0) return;
const int l = this->limit;
for (int i = 0;i limit = that.limit;

First thing I would change is the ordering of fields in the class declaration. I find it better to place public fields first, since this is what users of the class are interested on, most of the time. Then place protected fields in the sequence, since these are externally accessible via inheritance, so their encapsulation is less strong. Lastly place private fields that are only important to the class maintainer.

Unnecessary virtual destructor: Don't make a destructor virtual unless you intend to extend the class. In this case, I don't see why anyone would want to extend a queue implementation. Any use or extension of the queue can be done with composition, which is generally preferred over inheritance.

Default initialize all fields, not just some. Do it either in the declaration or in the constructor.

LLNode** queue = nullptr;
LLNode* first  = nullptr;
LLNode* last   = nullptr;
int limit      = 0;
int currSize   = 0;

Also notice the use of nullptr for the pointers. This is the standard since C++11.

Methods that don't mutate member state should be const. size() is an example:

int size() const;

Instead of returning the removed element in int dequeue(), perhaps provide a int front() const method that returns the element without removing. Then make dequeue() void. This is the interface of std::queue, so it would be a good idea to somewhat follow the convention people are most used to.

You don't need that explicit string cast when throwing the exceptions:

throw QueueError(std::string("Queue size is 0. Queue cannot be dequeued"));

All that is doing is creating an extra copy of the string. Some compilers might not be smart enough to optimize that out. It should be just:

throw QueueError("Queue size is 0. Queue cannot be dequeued");

If you separate your code into header file and .cpp implementation, make sure to only leave the necessary includes in the header file. You don't have to include ` and in the header, just in the .cpp`. Being careful with your includes will help you keeping compile times under control.

StackExchange Code Review Q#68609, answer score: 6