c++ list iterator not dereferencable when using switch statement

Started by
5 comments, last by Khatharr 8 years ago

Hello its me again :blink:

I have an error which keeps cropping up with a but of code, I have put a switch statement into a draw class and depending on which case I select it will draw the corresponding shape (normal, rotated, scaled etc) but Im getting list iterator not dereferencable when I select case 2.


void Shape::drawShape(int select)
{
	this->select = select;
	// plots each vertex and draws a line between them using Bresenham's algorithm
	switch (select)
	{
	case 1:
	{
		list<Vertex>::iterator current = vertices.begin();
		list<Vertex>::iterator previous = vertices.begin();
		while (current != vertices.end())
		{
			Console::gotoXY((*current).getX(), (*current).getY());
			cout << "*";
			if (current != vertices.begin())
				drawLine((*current).getX(), (*current).getY(), (*previous).getX(), (*previous).getY());
			previous = current;
			current++;
		}
		previous = vertices.begin();
		drawLine(vertices.back().getX(), vertices.back().getY(), vertices.front().getX(), vertices.front().getY());
		break;
	}

	case 2:
	{
		list<Vertex>::iterator rotate_current = rotated.begin();
		list<Vertex>::iterator rotate_previous = rotated.begin();
		while (rotate_current != rotated.end())
		{
			Console::gotoXY((*rotate_current).getX(), (*rotate_current).getY());
			cout << "*";
			if (rotate_current != rotated.begin())
				drawLine((*rotate_current).getX(), (*rotate_current).getY(), (*rotate_previous).getX(), (*rotate_previous).getY());
			rotate_previous = rotate_current;
			rotate_current++;
		}
		rotate_previous = rotated.begin();
		drawLine(rotated.back().getX(), rotated.back().getY(), rotated.front().getX(), rotated.front().getY());
		break;
	}
  }
}

if i select the first case it work fine but not with the second?

Here is where the function is called


list<Shape*>::iterator itr = shapes.begin();
	while(itr!=shapes.end())
	{
		(*itr)->drawShape(1); //draws fine 
		(*itr)->drawShape(2); //get error 
		itr++;
	}
Advertisement

As a general note, please always include the exact error message (with error code) when posting an error.

Marking the line of code which produces the error can also be beneficial.

EDIT: I mistakenly took this to be a compile error. Marking the line with the error is still helpful, though.

I would guess that the rotated list is empty/uninitialized.

Hello to all my stalkers.

As a general note, please always include the exact error message (with error code) when posting an error.

Marking the line of code which produces the error can also be beneficial.

EDIT: I mistakenly took this to be a compile error. Marking the line with the error is still helpful, though.

I would guess that the rotated list is empty/uninitialized.

Yes thats correct, I wasnt calling the rotation function to fill the list before calling the rotation draw case, Thank you :)


list<Vertex>::iterator rotate_current = rotated.begin();

can be


auto rotate_current = rotated.begin();

Which is easier on you and also makes it easier to change the container type.

Which raises the question: Why are you using a list rather than a vector? vector is superior in the overwhelming majority of cases, most especially for iteration, which I see you doing plenty of here.

Also, I mentioned DRY and switches in your other thread. You've got some of that going on here. You're doing the same thing in both cases, but with different containers. Why not make that process a function:


void doStuff(std::list<Vertex>& container);

and then just


switch(select) {
  case 1: doStuff(vertices); break;
  case 2: doStuff(rotated);  break;
}

You can also take advantage of the new for-each syntax, except in the case where you're using the 'previous' iterator (though you could just retain a pointer to the element there):


for(auto shape : shapes) {
  shape->drawShape(1);
  shape->drawShape(2);
}
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

I would love to be using vectors over list but the use of lists is part of my assignment, if I had the choice of would of used vectors all the way.

Thanks for the great advice, that makes the code much more manageable and logical thank you.


list<Vertex>::iterator rotate_current = rotated.begin();

can be


auto rotate_current = rotated.begin();

Which is easier on you and also makes it easier to change the container type.

Which raises the question: Why are you using a list rather than a vector? vector is superior in the overwhelming majority of cases, most especially for iteration, which I see you doing plenty of here.

Also, I mentioned DRY and switches in your other thread. You've got some of that going on here. You're doing the same thing in both cases, but with different containers. Why not make that process a function:


void doStuff(std::list<Vertex>& container);

and then just


switch(select) {
  case 1: doStuff(vertices); break;
  case 2: doStuff(rotated);  break;
}

You can also take advantage of the new for-each syntax, except in the case where you're using the 'previous' iterator (though you could just retain a pointer to the element there):


for(auto shape : shapes) {
  shape->drawShape(1);
  shape->drawShape(2);
}

with the switch statements I have taken you advice and put them in the draw function and select which case based on preference, the messy part is trying to implement a crude menu, such as:

draw shapes? 1=yes, 2=No.

1. rotate shape

2. output shape stats

3. scale shape

So due to all the multiple choices say after you select rotate, you might want the stats to show on the newly rotated shape so again is another nested switch, I like the concept of a navigation menu but I'm not sure if switching is the best for this as the code is just becoming convoluted.

Why are you trying to nest again? If an option leads to more options then make the sub-options their own function.
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

This topic is closed to new replies.

Advertisement