Polymorphic Queue

[metdos]

Hello Everyone,
I'm trying to implement a Polymorphic Queue.
Here is my trial:

QQueue <Request *> requests;

while(...)
	{
		QString line = QString::fromUtf8(client->readLine()).trimmed();

		if(...)){                      
			Request *request=new Request();
			request->tcpMessage=line.toUtf8();
			request->decodeFromTcpMessage(); //this initialize variables in request using tcpMessage
			if(request->requestType==REQUEST_LOGIN){
				LoginRequest loginRequest;
				request=&loginRequest;
				request->tcpMessage=line.toUtf8();
				request->decodeFromTcpMessage();
				requests.enqueue(request);
			}
			//Here pointers in "requests" do not point to objects I created above, and I noticed that their destructors are also called.
			LoginRequest *loginRequest2=dynamic_cast<LoginRequest *>(requests.dequeue());	
			loginRequest2->decodeFromTcpMessage();
		}
	}

Unfortunately, I could not manage to make work Polymorphic Queue with this code because of the reason I mentioned in second comment.
I'm open to any improvement of my code or a new implementation of polymorphic queue.

Thanks.

[Banfa]

The problem is at line 12 loginRequest is created on the stack, that means that it is automatically has the lifetime of the code block it is in which is the if statement.

You take pointers to it and put them on your queue but that doesn't change the fact that the original object is deleted when its lifetime expires.

You need an object whos lifetime is not limited to the code block it is created in. That would be an object allocated from the heap using new.

Change how you use your queue so that the pointers it contains always point to objects allocated using new. When you remove items from the queue remember to delete them once you are finished with them. Possibly look into using std::auto_ptr or a smart pointer or handle class.

Is there any reason you didn't use a std::queue? Is QQueue at QT class?

[metdos]

Yes it is a QT class, I used it just for coherence.

[metdos]

Change how you use your queue so that the pointers it contains always point to objects allocated using new. When you remove items from the queue remember to delete them once you are finished with them. Possibly look into using std::auto_ptr or a smart pointer or handle class.

Can you give an example?

[thelamb]

You have already done it...

Look at how you declare your 'request':

Request *request=new Request();

So 'request' is now allocated on the heap.. means you are responsible for cleaning it up (delete request; somewhere in the code).

Then, you say.. ok I want 'request' to point to a LoginRequest.

LoginRequest loginRequest;
request=&loginRequest;

So you are overwriting the previous pointer to 'new Request();' with a pointer to a local variable loginRequest.
Since loginRequest is local, it will be destroyed when it goes out of scope. Now your 'request' pointer points to garbage and there is no way that you can delete the original new Request()(remember you overwrote the pointer), so you have yourself a memory leak.

So I dont really understand why you do the first "new Request();".. doesnt this make more sense:

Request *request=new LoginRequest(); // I guess LoginRequests inherits from Request?
request->tcpMessage=line.toUtf8();
request->decodeFromTcpMessage(); 
if(request->requestType==REQUEST_LOGIN)
{
    requests.enqueue(request);
} else  // Request is not a LOGIN_REQUEST
{ 
     delete request;
}

// When you destroy the requests.. dont forget to delete the pointers.

P.S. think about your naming convention...
Is using 'requests' and 'request' a good idea? The names look very simmilar and just introduce confusion.

[metdos]

So I dont really understand why you do the first "new Request();".. doesnt this make more sense:

Because at that level I don't know which subclass is it, and if statements will continue like this:

#
if(request->requestType==REQUEST_LOGIN){
LoginRequest loginRequest;
request=&loginRequest;
requests.enqueue(request);
} else if(request->requestType==REQUEST_FIELD) {
FieldRequest fieldRequest;
request=&fieldRequest;
requests.enqueue(request);
}

so, I need to create subclass at lower scope, and keep it at higher scope.

P.S. Thanks for memory leak problem, and suggession about naming convention.

[Banfa]

Actually this piece of code is not really interested in the sub-class at all. It just needs a Request* in order to queue the request.

It looks like you could use a factory method. A factory method is often a static member of a base class that accepts data and uses the data to create a sub-class of that base but returns a base class pointer.

This keeps the code the knows how and when to create the sub-classes nicely encapsulated in the class your code you then look something like

Request *request=Request::requestFactory(line);

	if (request != NULL)
	{
		requests.enqueue(request);
	}

The factory method looks something like

Request::requestFactory(const QSTring& line)
{
	RequestTypeType requestType = <Decode type From Line>;
	Request* retval = NULL;

	switch(requestType)
	{
	case REQUEST_LOGIN:
		request= new LoginRequest;
		request->tcpMessage=line.toUtf8();
		request->decodeFromTcpMessage();
		break;

	case OTHER_REQUEST_TYPE:
		request= new OtherRequestType;
		request->tcpMessage=line.toUtf8();
		request->decodeFromTcpMessage();
		break;

	...
	}

	return request;
}

[metdos]

This version solved my problem.

QQueue <Request *> requests;

while(...)
	{
		QString line = QString::fromUtf8(client->readLine()).trimmed();

		if(...)){                      
			Request *request=new Request();
			request->tcpMessage=line.toUtf8();
			request->decodeFromTcpMessage(); //this initialize variables in request using tcpMessage
			if(request->requestType==REQUEST_LOGIN){
<strong>				delete request;
				request=new LoginRequest();</strong>
				request->tcpMessage=line.toUtf8();
				request->decodeFromTcpMessage();
				requests.enqueue(request);
			}
			//Here pointers in "requests" do not point to objects I created above, and I noticed that their destructors are also called.
			LoginRequest *loginRequest2=dynamic_cast<LoginRequest *>(requests.dequeue());	
			loginRequest2->decodeFromTcpMessage();
		}
	}

PS, Factory method seems usefull, I will try it, thanks.