Sign in to follow this  

Problem with linked list

This topic is 3106 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I am trying to program a Workout log, compiling it step by stet to see if there is any problem. As always, there is one. When I build it, it gives me no errors, and I am able to compile it. But during runtime it stops executing and the system crashes I am not sure why. However I did debug it found the problem lies with the vector string I created. The program crashes when it reaches the for loop, more specifically when I call for the vector.size(); here is the relavent code,although ask if you need to see more :

void workOutRoutine::AddNode()
{
	system("cls"); //Could not resist

	unsigned int i(0),j(0);

	//Used to add new node
	workOutRoutine *Temp = new workOutRoutine;
	//Used to determine the end of the list to add new node
	workOutRoutine *EndList; 

	//Dummy variable 
	//getline reads \n
	string dummy;
	getline(cin,dummy);

	//Get Day Of workout;
	cout<<"Enter the Day : ";
	getline(cin,Temp->Day);
	cout<<endl;
	//Get body thats being worked out
	cout<<"Which Body Part is Worked out on "<<Temp->Day<<"\t";
	getline(cin,Temp->bodyPart);
	cout<<endl;

	//Add the type of workout
	//on the unique body part
	//and the number of sets
	//each with certain weight
	for(i =0; i <= Exercise.size(); i++) //<--- PROBLEM LIES WITHIN "Exercise.size(). While degugging it crashes my program.
	{
		if(i == Exercise.size()-1)
		{
			cout<<"\nWould You like to add more sets to your "<<Temp->bodyPart<<" workout (y/n) ";
			char opt;
			cin >> opt;

			//NOT SURE IF THIS PART WORKS THE WAY I WANT IT TO,
			//BUT I'LL DEAL WITH THAT AFTER THIS BUG IS FIXED
			if(opt == 'y' || opt == 'Y') 
			{
				cout<<"\nAdding another Workout Element...";
				Exercise.resize(Exercise.size()+1,vector<string>(Exercise[i].size()+1));
				cout<<"\nDone adding another Element"<<endl;
			}
			
			else break;

		}

		cout<<"What type of "<<Temp->bodyPart<<" workout did you do : ";
		getline(cin,Exercise[i][0]); // [i][0] is the Identifier or type of workout
		cout<<"\n"<<endl;

			for(j = 1; i< Exercise[i].size(); j++)
			{
				cout<<"Enter the weight lifted for set # "<<++j<<"  :  ";
				getline(cin,Exercise[i][j]);
			}

	}

	//Get Additional Info
	cout<<"If you did any extra 'workout' the enter keywords with time like basketball(30.00 minutes),running(10.00 minutes)\n";
	cout<<"else enter NONE :  ";
	getline(cin,Extra);

	cout<<"\nWhat do you estimate the total time you took to workout :  ";
	getline(cin,TotalTime);
	
	

	//Check to see if any node has been alocated
	if(strt == NULL)
	{
		strt = Temp;
		strt->NextNode = NULL;
		return;

	}
	else
	{	//set endlist pointer to start of the list
		EndList = strt;

		//Move to the end of the list
		while(EndList->NextNode != NULL)
			EndList = EndList->NextNode;

		EndList = Temp; //Add new node
		EndList->NextNode = NULL;

	}
}

Regards, Tnutty

Share this post


Link to post
Share on other sites
Yes, I think so, here is the full code


#include<iostream>
#include<string>
#include<vector>
#include<windows.h>

using namespace std;

//Min number of Exercie that I will do per Workout
const unsigned int MIN_EXERCISE = 3;
//The number of sets/exercise done
//for a particular workout
const unsigned int MIN_SETS = 4;

//Class hold every info about the type of workout, which days it on
// which body part, how many sets, how much weight lifted per set, and
//the time it tool
class workOutRoutine
{
public:
//The day of workout
string Day;
//The body part thats being worked out
string bodyPart;
//The type of exercise begin used
//And how many times being done
//Exercise[x][0] is Exercise identifier
//Exercise[x][y] is the weight lifted
vector<vector<string> > Exercise;
//Any extra exercise done
string Extra;
//The time spent during a particular day of working out
string TotalTime;

workOutRoutine *NextNode;

public:
workOutRoutine();

void AddNode();
void PrintInfo();
bool DeleteFirstNode();
bool DeleteLastNode();
bool DeleteCurrentNode();
void MoveRight();
void ModeLeft();
bool SaveToFile();

} *strt,*curr;

//The constructor
workOutRoutine::workOutRoutine()
{
//set strt so that its empty
strt = NULL;
curr = strt;
//Resize vector to the minimun number of exercise that
//I will do and to the minimum number of sets
Exercise.resize(MIN_EXERCISE,vector<string>(MIN_SETS,"NONE"));

}



void workOutRoutine::AddNode()
{
system("cls");

unsigned int i(0),j(0);

//Used to add new node
workOutRoutine *Temp = new workOutRoutine;
//Used to determine the end of the list to add new node
workOutRoutine *EndList;

//Dummy variable
//getline reads \n
string dummy;
getline(cin,dummy);

//Get Day Of workout;
cout<<"Enter the Day : ";
getline(cin,Temp->Day);
cout<<endl;
//Get body thats being worked out
cout<<"Which Body Part is Worked out on "<<Temp->Day<<"\t";
getline(cin,Temp->bodyPart);
cout<<endl;

//Add the type of workout
//on the unique body part
//and the number of sets
//each with certain weight
for(i =0; i <=Exercise.size(); i++)
{
if(i == Exercise.size()-1)
{
cout<<"\nWould You like to add more sets to your "<<Temp->bodyPart<<" workout (y/n) ";
char opt;
cin >> opt;

if(opt == 'y' || opt == 'Y')
{
cout<<"\nAdding another Workout Element...";
Exercise.resize(Exercise.size()+1,vector<string>(Exercise[i].size()+1));
cout<<"\nDone adding another Element"<<endl;
}

else break;

}

cout<<"What type of "<<Temp->bodyPart<<" workout did you do : ";
getline(cin,Exercise[i][0]); // [i][0] is the Identifier or type of workout
cout<<"\n"<<endl;

for(j = 1; i< Exercise[i].size(); j++)
{
cout<<"Enter the weight lifted for set # "<<++j<<" : ";
getline(cin,Exercise[i][j]);
}

}

//Get Additional Info
cout<<"If you did any extra 'workout' the enter keywords with time like basketball(30.00 minutes),running(10.00 minutes)\n";
cout<<"else enter NONE : ";
getline(cin,Extra);

cout<<"\nWhat do you estimate the total time you took to workout : ";
getline(cin,TotalTime);



//Check to see if any node has been alocated
if(strt == NULL)
{
strt = Temp;
strt->NextNode = NULL;
return;

}
else
{ //set endlist pointer to start of the list
EndList = strt;

//Move to the end of the list
while(EndList->NextNode != NULL)
EndList = EndList->NextNode;

EndList = Temp; //Add new node
EndList->NextNode = NULL;

}
}






void workOutRoutine::PrintInfo()
{
workOutRoutine *Temp = strt;

//Check if any node exist
if(Temp == NULL)
{
cout<<"No routine at this point\n\n";
return;
}

//Print all Info
while(Temp != NULL)
{
cout<<"Day of Week "<<Temp->Day<<endl;
cout<<"Body Part worked out : "<<Temp->bodyPart<<endl;
for(unsigned int i = 0; i<Exercise.size(); i++)
{
//Exercise[i][0] represents Workout Type/ID
cout<<"Type of Workout #"<<i<<"\t\t"<<Temp->Exercise[i][0]<<endl;

for(unsigned int j = 1; j< Exercise[i].size(); j++)
{
//Exercise weight lifted
cout<<"\t Set #"<<i<<" : "<<Exercise[i][j]<<endl;
}
}

cout<<endl<<endl;

}



}

int Options(int opt)
{
cout<<"1 -- Add Day to workout"<<endl;
cout<<"2 -- Move Node forward"<<endl;
cout<<"3 -- Move Node Backward"<<endl;
cout<<"4 -- Delete current Node"<<endl;
cout<<"5 -- Delete First Node"<<endl;
cout<<"6 -- Delete Last Node"<<endl;
cout<<"7 -- Save Info to File"<<endl;
cout<<"8 -- Load Info to File"<<endl<<endl;
cin >> opt;

return opt;

}
int main()
{
int opt = -1;
//Call the constructor
workOutRoutine();

do
{
system("cls");

strt->PrintInfo();
cout<<"\n\n"<<endl;

//Get uses input
opt = Options(opt);

switch(opt)
{
case 1: strt->AddNode(); break;
case 2: break;
case 3: break;
case 4: break;
case 5: break;
case 6: break;
case 7: break;
case 8: break;
}


}while(opt != 0);

}

Share this post


Link to post
Share on other sites
the Ctor

workOutRoutine::workOutRoutine()
{
//set strt so that its empty
strt = NULL;
curr = strt;
//Resize vector to the minimun number of exercise that
//I will do and to the minimum number of sets
Exercise.resize(MIN_EXERCISE,vector<string>(MIN_SETS,"NONE"));

}


Share this post


Link to post
Share on other sites
I tried this before with another linked list program and it gave no problem.

If set it to null in main, it gives the same problem.

Here is the error:

Unhandled exception at 0x008132e9 in ExerciseRoutine.exe: 0xC0000005: Access violation reading location 0x0000004c.


Share this post


Link to post
Share on other sites
The size function is 1..n not 0..n ( use less-than not less-than-equal)

for(i =0; i <= Exercise.size(); i++)


it tries to iterate one step further than the actual size

... that at least for a start.


Its one of the most common errors -- an 'off by one' error

Share this post


Link to post
Share on other sites
Quote:
Original post by wodinoneeye
The size function is 1..n not 0..n ( use less-than not less-than-equal)

for(i =0; i <= Exercise.size(); i++)


it tries to iterate one step further than the actual size

... that at least for a start.


Its one of the most common errors -- an 'off by one' error




I do this to make push_back func work
when (i) reaches vec.Size(), since it starts from 0, its 1 size passed it, so I
call push_back method if I want to add more sets.


if(i == Exercise.size())
{
cout<<"\nWould You like to add more sets to your "<<Temp->bodyPart<<" workout (y/n) ";
char opt;
cin >> opt;

if(opt == 'y' || opt == 'Y')
{
cout<<"\nAdding another Workout Element...";
Exercise.resize(Exercise.size()+1,vector<string>(Exercise[i].size()+1));
cout<<"\nDone adding another Element"<<endl;
}



Share this post


Link to post
Share on other sites
Quote:
Original post by tnutty
If set it to null in main, it gives the same problem.


Quote:
Original post by SiCrane
You may not realize this, but NULL is not a valid pointer value to call a member function on.


Share this post


Link to post
Share on other sites
Quote:
Original post by tnutty
So how would you try to solve this?


Initialise *all* the variables you want to use. You do not need to use globals here. Pointers need to be pointed at something, either via new or the address of a local.

Your code is confusing.

See where you "call the constructor"? You are creating a temporary here. However, you appear to want it to have side effects. This is not a great idea, it is very unclear.

You should be using values, you don't need to use pointers in your program. Use std::list<> as a linked list.

AddNode() should not be doing I/O. Generally, you want to separate I/O from program logic. Also, this would be a lot easier if you separated the container logic from the work out routine part. Again, std::list<> rather than writing your own container.
Quote:

//The type of exercise begin used
//And how many times being done
//Exercise[x][0] is Exercise identifier
//Exercise[x][y] is the weight lifted
vector<vector<string> > Exercise;

You should make a type to represent this information, rather than a raw vector<string>.

Here is a bug:

for(j = 1; i< Exercise[i].size(); j++)
{
// ...
}

And are you sure you want to increment "j" in this next line?

cout<<"Enter the weight lifted for set # "<<++j<<" : ";

Share this post


Link to post
Share on other sites
I realize that I could do this program in another way, its just for practicing
linked list. Please bare with me.

so in this code :

Here is a bug:

for(j = 1; i< Exercise[i].size(); j++)

{

// ...

}


I am not exactly seeing why the problem exist. I realize that the call
to Exercise[i].size() is the bug. But using a pointer(pointing to it) causes it
read the memory in a invalid area? Why so?

Share this post


Link to post
Share on other sites
Quote:
Original post by tnutty
I realize that I could do this program in another way, its just for practicing
linked list.

Practicing writing bad code will only teach you to write bad code.

Share this post


Link to post
Share on other sites
So what advice would you give me to write better code for this program.
Drop it and start with list,vectors,or queues, or rewrite this
program better?

Share this post


Link to post
Share on other sites
I would recommend actually reading and trying to understand comments made about your code in the thread already. If you're not going to bother trying to understand what people are telling you when you ask a question you're just wasting everyone's time.

Share this post


Link to post
Share on other sites
Quote:
Original post by tnutty
I realize that I could do this program in another way, its just for practicing
linked list. Please bare with me.


I found implementing my own templated linked list hugely educational and I would recommend it to others. However, what you're doing here is a mess. There is no separation between the container and the rest of your program.

If you want to learn about containers, I think you should write an implementation of the interface provided by the Standard C++ Library. http://www.cplusplus.com/reference/stl/list/

Share this post


Link to post
Share on other sites
Quote:
Original post by Cantos
However, what you're doing here is a mess. There is no separation between the container and the rest of your program.


I just started to learn linked list from online tutorial, and this was the way
he implemented it. But,since my program is such a catastrophe what guidelines would you provide for implementing a decent linked-list?

Share this post


Link to post
Share on other sites
Quote:
But,since my program is such a catastrophe what guidelines would you provide for implementing a decent linked-list?
Start a new program free of all cruft. You are not making anything but a linked list. You are not making a workout program. You are not making pretty text interfaces. Just a linked list implementation.

Then start by getting the data structures right. You need the syntax to represent a node in the linked list, and the syntax to represent the entire linked list itself. This is not a given. Focus on getting this right. Verify it on this forum if you need to do so. Then work on getting on adding algorithms to this structure, one by one.

Share this post


Link to post
Share on other sites
Quote:
Original post by tnutty
But,since my program is such a catastrophe what guidelines would you provide for implementing a decent linked-list?


Well, first off, why do you want to implement one? The things that you need to know in order to do it properly can be learned just as well in many other ways.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Quote:
Original post by tnutty
But,since my program is such a catastrophe what guidelines would you provide for implementing a decent linked-list?


Well, first off, why do you want to implement one? The things that you need to know in order to do it properly can be learned just as well in many other ways.


For practice, as other of my recent comment suggests.

You know, I think I picked up some bad habits from online tuts, like from this
one : http://richardbowles.tripod.com/cpp/linklist/linklist.htm


Share this post


Link to post
Share on other sites
Tripod is a site that lets the general public create small webpages. You shouldn't really trust it for quality, for that reason: you need to verify that the author knows what s/he is talking about. And I would guess, without even reading the page, from that colour scheme and background choice that the author of the tutorial in question is not exactly a professional. :)

Share this post


Link to post
Share on other sites
I will think as I write.

In the first box of code, I cannot find any instance where you actually define and set the variable "Exercise", hence it is a NULL pointer. The size of a NULL pointer is also undefined, so the loop accesses an undefined variable to determine how many times to loop and has no clue.

*****
//Add the type of workout
//on the unique body part
//and the number of sets
//each with certain weight
for(i =0; i <= Exercise.size(); i++) //<--- PROBLEM LIES WITHIN "Exercise.size(). While degugging it crashes my program.
*****

At this point you have "i" and "j" defined, "Temp" is a pointer to a workOutRoutine structure as also is the variable "EndList". You also have a string "dummy", (no I am not calling you a dummy),but no where is any variable mentioned "Exercise" until you access it in the loop. The program does not know if it is a string, integer or whatever. The address to this variable can be any number. It happens that it is near zero, which is protected memory where the operating system resides. That is why the access violation. The location, ie: memory address is 4C, that is a low number in the area of BIOS and other system memory.

I see in the full code that it is defined as an array of strings I think, but I have not used the vector function. I do know that when you setup some arrays, especially with strings it starts out with an undefined address until you first assign a value of some sort. I use a listing program in addition to debugger. It tells you a lot more. If you step through the debugger and look at what value "Exercise" is as it progresses, have the debugger stop just before the loop, check to see what "Exercise" is, the address and the value.

I have also never encountered a program that uses a for loop command with a variable in the loop command that is modified by the loop itself. I have always had the range, 1 to 6 or 1 to 10, set before starting the loop. If I needed to vary the number of iterations, I would use a repeat, or a while loop instead. These commands are designed to be variable, whereas the for loop is usually set before the iterations begin. At least that is the conformity that I stick to.

It looks like you are having the arrays set to 3 and 4, and the string initialized to "NONE" to start off with, if I understand the program. So, it looks like the array is an array of arrays, Exercise[3][4]. I think that the problem is with the size of the array. You really do wish to progress through the exercises, which is 3 and then set the string variable in each of the sets within each of the exercises. Problem? Does size return 12, or does it return 3 when you ask for the size of Exercise? You use the similar statement for each loop, in the "i" portion of the loop, ie: number of exercises and the "j" portion of the loop, the sets. If it returns 12 each time for the whole array, and you want to get 3, then.....

So, it will step through 12 times for each of the loops.

loop 1 = Exercise[0][0] (This is the name of the workout?)
loop 2 = Exercise[0][1] (This is the weight of each set?)
loop 3 = Exercise[0][2]
loop 4 = Exercise[0][3] Oops, Crash, not defined

it even wants to go all the way to:

loop 3 = Exercise[0][11]

I am not sure what the size function will return on the whole set of arrays, whether it returns the number of arrays, or the total number of elements. I have not been programming for a while. I think this is the problem. You need to run the debugger and ask to view the size of the variable Exercise, to see if it is 3, 4 or 12.

In the documentation it states that the size function "Returns the number of elements in the vector container". This would tell me that this number is 12.

Conclusion, set a stop just before the loop and investigate what the size value of the array is. If it is 12, then your loop is going too far.

I hope that I have been clear as mud. :)

Thank you.

John Barradale










Share this post


Link to post
Share on other sites
Quote:
Original post by Scharfschuetze
In the first box of code, I cannot find any instance where you actually define and set the variable "Exercise", hence it is a NULL pointer.

Incorrect: it's a vector of string vectors. See the code in his second post. There's nothing wrong with that - both vectors and strings have default constructors. Besides, pointers need to be dereferenced - but he's using the dot operator instead. So, no pointers there. As others already pointed out, he's probably calling that AddNode function on a bad pointer.

A vector is not a function, bytheway - it's a container class, and part of C++' standard library.

Share this post


Link to post
Share on other sites

This topic is 3106 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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