C++ queue destructor problem?
I'm not sure about my destructor...... the queue works but is the destructor ok to use?
#ifndef BANKQUEUE_H
#define BANKQUEUE_H
#include<iostream>
using namespace std;
class BankQueue {
struct node {
int arrival;
int length;
node *next;
};
int size;
node *front,*rear;
public:
BankQueue();
~BankQueue();
bool queueIsEmpty();
void enqueue(int newArrival, int newLength);
void dequeue();
void getQueueFront(int &theArrival, int &theLength);
};
#include "BankQueue.cpp"
#endif
#include "BankQueue.h"
BankQueue::BankQueue() {
size = 0;
front = NULL;
rear = NULL;
}
BankQueue::~BankQueue() {
while (!queueIsEmpty())
dequeue();
}
bool BankQueue::queueIsEmpty() {
return (size==0);
}
void BankQueue::enqueue(int newArrival, int newLength) {
if(true) {
size++;
node *temp;
if (front==NULL) {
temp = new node;
front = temp;
front->arrival = newArrival;
front->length = newLength;
rear = front;
rear->next = NULL;
}
else {
temp = new node;
temp->arrival = newArrival;
temp->length = newLength;
rear->next = temp;
rear = temp;
}
}
else {
cout<<"Could not add data"<<endl;
return;
}
}
void BankQueue::dequeue() {
if (size!=0) {
size--;
node *temp = front;
front = front->next;
delete temp;
}
else
return;
}
void BankQueue::getQueueFront(int &theArrival, int &theLength) {
if (size!=0) {
theArrival = front->arrival;
theLength = front->length;
}
else {
cout<<"Queue is empty..."<<endl;
return;
}
}
Comments
I agree with the others. This looks ok to use, barring the edits already mentioned. I can't remember if anyone mentioned this, but when creating a new node in the queue, I prefer to explicitly set the new node's next pointer to NULL, something you haven't done here. Otherwise, it will be junk data and if you try to dereference it, something will probably crash and (more importantly) you may have headaches trying to track that bug down.
I would change enqueue() to:
void BankQueue::enqueue(int newArrival, int newLength) {
size++;
node* temp;
if (front == NULL) {
temp = new node;
front = temp;
front->arrival = newArrival;
front->length = newLength;
rear = front;
rear->next = NULL;
}
else {
temp = new node;
temp->arrival = newArrival;
temp->length = newLength;
temp->next = NULL; // * add this
rear->next = temp;
rear = temp;
}
}
Also, I would like to make sure you're using BankQueue::node.length as application data and not the length of the queue or something stupid like that.
It's OK to use. Reuse of existing methods is always a good idea, and since you'll never have to write such class in real life (C++ already has queues), it doesn't really matter how it's done -- you're supposed to show that you've learned whatever you've been studying at this point.
If I had to make something like this in real life, I would have taught Node to manage its child node's lifetime (by wrapping those pointers in auto_ptr/unique_ptr, except for Queue's "rear"), which would eliminate all destructors or calls to 'delete', thus achieving correct resource management and exception safety.
A few style improvements that I would suggest:
get rid of "using namespace" in the header file (they should never appear in header files)
use initialization list in the constructor
mark queueIsEmpty() const (it does not modify the queue, does it?)
in enqueue(), the else block of " if(true) { " will never execute,
in dequeue(), the else block is unnecessary
mark getQueueFront() const
make getQueueFront() return your data with a return statement, instead of writing to reference parameters
and.. what exactly is that #include "BankQueue.cpp" supposed to mean??
(oh, and listen to BeanCounter's suggestion, too, since your struct node doesn't have its own constructor, node::next's do have to be set to null in enqueue())
I don't see anything wrong with it. That's a pretty common way to write destructors.
It's not efficient for things like a double-linked list because you keep updating pointers on objects you're just going to delete. But in your case, it's not clear that a 'hand coded' loop would be any more efficient.
This is about the best you can do:
void BankQueue::~BankQueue() {
while(front!=NULL)
{
node *temp=front;
front=front->next;
delete temp;
}
}
The only real difference is that yours updates and tests 'size', which isn't needed. That's not really much of a difference.
Yours has the advantage that it has one less "tricky bit" than mine. And tricky bits can be sources of bugs, maintenance problems, and so on. (Though in this case, all the code is simple enough that this isn't such a big deal. I just bring it up because in many cases, that can be very important.)
wue.rst>ips::
add this in the beginins
and it will self repair
my friend taught me