What's wrong with this?

Started by
9 comments, last by Gregor_Samsa 23 years, 5 months ago
What''s wrong with this code? #include //#include "List.cpp" //using namespace std; //introduces namespace std class Bullet { public: Bullet *prev; Bullet *next; Bullet () {}; Bullet (/*POINT location,*/ double direction, int type); void doAI(); //Bullet returnNext(); //Bullet returnPrev(); }; Bullet::Bullet (/*POINT location,*/ double direction, int type) { cout << "new Bullet created" << direction << type; } void Bullet::doAI() { cout << "Doing AI..."; } /*Bullet Bullet::returnNext() { return *next; } Bullet Bullet::returnPrev() { return *prev; }*/ class List { private: Bullet *FirstBullet; Bullet *LastBullet; public: List(); List(Bullet *aBullet); ~List(); void doAI(); Bullet* AddBullet(/*LPoint location,*/ double direction, int type); void RemoveBullet(Bullet *aBullet); }; List::List() { FirstBullet = NULL; LastBullet = NULL; } List::List(Bullet *aBullet) { FirstBullet = aBullet; LastBullet = aBullet; } void List::doAI() { Bullet *temp_bullet; Bullet *temp_next; temp_bullet = FirstBullet; while(temp_bullet != NULL) { temp_next = temp_bullet->next; temp_bullet->doAI(); temp_bullet = temp_next; } } Bullet* List::AddBullet(/*POINT location,*/ double direction, int type) { Bullet *newBullet; newBullet = new Bullet(/*location,*/ direction, type); if (LastBullet == NULL) { FirstBullet =LastBullet = newBullet; } else { LastBullet->next = newBullet; newBullet->prev = LastBullet; LastBullet = newBullet; } return newBullet; } void List::RemoveBullet(Bullet *aBullet) { if (FirstBullet == aBullet) { Bullet *temp_next; temp_next = aBullet->next; temp_next->prev = NULL; FirstBullet = temp_next; delete aBullet; } else if (LastBullet == aBullet) { Bullet *temp_prev; temp_prev = aBullet->prev; temp_prev->next = NULL; delete aBullet; } else { Bullet *temp_prev; Bullet *temp_next; temp_prev = aBullet->prev; temp_next = aBullet->next; temp_prev->next = temp_next; temp_next->prev = temp_prev; delete aBullet; } } List::~List() { Bullet *temp_bullet; Bullet *temp_next; temp_bullet = FirstBullet; while(temp_bullet != NULL) { temp_next = temp_bullet->next; RemoveBullet(temp_bullet); temp_bullet = temp_next; } } void main () { Bullet *temp_bullet; /*LPoint tempPoint;*/ int temptype, tempX, tempY; double tempDouble; List myList; cout << "How many Bullets to create? "; int input; cin >> input; for (int i = 1; i<=input; i++) { cout << "\nInput x location for bullet#" << i; cin >> tempX; cout << "\nInput y location for bullet#" << i; cin >> tempY; /*tempPoint.x = tempX; tempPoint.y = tempY;*/ cout << "\nInput direction for bullet#" << i; cin >> tempDouble; cout << "\nInput type number for bullet#" << i; cin >> temptype; temp_bullet = myList.AddBullet(/*tempPoint, */tempDouble, temptype); } myList.doAI(); //return 0; } It performs an illegal operation (try compiling it). I have no idea what could be the problem because it seems right to me. E-mail: chaos1111@hotmail.com me (Mark)
"When i was a child I caught a fleeting glimpse, out ofthe corner of my mind. I turned to look, but it was gone, I cannot put my finger on it now. The child hasgrown, the dream has gone." -Pink Floyd
Advertisement
In your AddBullet, you do LastBullet->next = newBullet, as well as LastBullet = newBullet...Im not sure why you do this...

this would create a loop when you try to traverse the list and get to the LastBullet....
Waramp.Before you insult a man, walk a mile in his shoes.That way, when you do insult him, you'll be a mile away, and you'll have his shoes.
The point of that code was:
1)LastBullet->next = newBullet, is supposed to mean that the Bullet object that LastBullet points to now has a link to newBullet.

2)LastBullet = newBullet, is supposed to make last Bullet now point to a different Bullet object. ie. newBullet.

Is this idea flawed somehow?

"When i was a child I caught a fleeting glimpse, out ofthe corner of my mind. I turned to look, but it was gone, I cannot put my finger on it now. The child hasgrown, the dream has gone." -Pink Floyd
I think your list insertion code is fine. The problem looks to me that you never initialize your Bullet.prev and Bullet.next pointers in your link list to NULL. You should do this in your Bullet constructor. Without it, you''ll get an infinite loop in doAI, I believe.

I didn''t actually test your code, so I may have overlooked something.

HTH
I just tried setting Bullet.prev and Bullet.next to NULL in the constructor. It didn''t work. It is "good programming practice", but it''s not the problem. Your on the right track though...

"When i was a child I caught a fleeting glimpse, out ofthe corner of my mind. I turned to look, but it was gone, I cannot put my finger on it now. The child hasgrown, the dream has gone." -Pink Floyd
It''s not just "good programming practice", it''s definitely required in your case, because you query those variables to terminate your links. Class member vars are not initialized to 0 as they are for statics.

Too bad that didn''t fix the problem, though! I notice you have two constructors: are you initializing the variables in both of them?

I would suggest booting up your debugger and just stepping through the procedure.

Goodl uck!
Ok, I''ve been investigating with the debugger. I''ve discovered that the problem is with the RemoveBullet function, which is called by the List destructor. The exact line in the RemoveBullet function that it stops on is: temp_prev->next = temp_next;. This is in the final else statment.

Also, the doAI() function in the Bullet class is supposed to output text, but this never happens. This would suggest that the destructor is being called early. This really confuses me.I discovered that if I remove the List destructor entirely that the program works perfectly.

Now, I''m wondering, if you use dynamic memory, don''t you have to delete it? Why is it that when I try to delete it i get errors and when i don''t it works perfectly?

"When i was a child I caught a fleeting glimpse, out ofthe corner of my mind. I turned to look, but it was gone, I cannot put my finger on it now. The child hasgrown, the dream has gone." -Pink Floyd
I think your problem is here:

Bullet *temp_next;
temp_next = aBullet->next;
temp_next->prev = NULL;
FirstBullet = temp_next;
delete aBullet;


You are not checking to see if there is ONLY 1 bullet.

If you are deleting the first bullet, aBullet->next would be null.

therefore calling temp_next->prev = NULL; would cause an error, as being null, temp_next has no data to access.

---------------------------Hello, and Welcome to some arbitrary temporal location in the space-time continuum.

But wouldn''t my if statments take care of that problem:
if (FirstBullet = aBullet), else if (Last Bullet == aBullet)?

I think for some reason that these statments don''t work properly, so the final else statement is always being executed, but i don''t know what''s wrong

"When i was a child I caught a fleeting glimpse, out ofthe corner of my mind. I turned to look, but it was gone, I cannot put my finger on it now. The child hasgrown, the dream has gone." -Pink Floyd
First, both constructors need to set prev and next to NULL.

Second, in RemoveBullet:
No way it could know if its the last. Change

Bullet *temp_next;
temp_next = aBullet->next;
temp_next->prev = NULL;
FirstBullet = temp_next;
delete aBullet;

To:

Bullet *temp_next;
temp_next = aBullet->next;
if (temp_next != NULL) temp_next->prev = NULL;
FirstBullet = temp_next;
delete aBullet;

Runs without a hitch now

This topic is closed to new replies.

Advertisement