Sign in to follow this  
homi576

c++ list iterator not dereferencable when using switch statement

Recommended Posts

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++;
	}

Share this post


Link to post
Share on other sites

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.

Edited by Lactose!

Share this post


Link to post
Share on other sites

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 :)

Share this post


Link to post
Share on other sites
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);
}

Share this post


Link to post
Share on other sites

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.

Edited by homi576

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this