🔒 Locked

Untitled

Started by Dark_Glitch Apr 7, 2009 at 2:36 AM 10 replies 2k views

Original Post

Apr 7, 2009 at 2:36 AM

Ok, so I've got this nice program that I'm building slowly and surely and once again, something I wrote that I believe should work, isn't and I need your help again Gamedev.net. NOW, I'll try to explain this better than I usually do. First, I thought I'll need to create a function that can be called in my game that will get an item's information from the database file, this way I won't have to hardcode each and every item in the game into the source files, which we all know is a boneheaded thing to do right? Here's the problem: The function I wrote is not working:


 void Item::GetItemInfo(string itemName, Item &item)
{
	ifstream fin;
	
	string tmpFilename = "C:/Game/Items/MasterItemList.lis";
	fin.open(tmpFilename.c_str());
	

	string szNameIn;
	string szWord;
	string filename;
	bool bLoaded = false;

	while(!bLoaded)
	{
		if(fin.fail())
		{
			cout << "File Input Failed!"
				 << endl
				 << "Find out why!"
				 << endl;
		}
                // Here is where I believe the problem is. The Master Item List
                // contains the name and the location of each item file, like so
/*
Item: ItemName
Location: C:/GameFolder/Items/ItemName.itm

Item: ItemName2
Location: C:/You Get The picture/ItemName2.itm
*/

                // The function I wrote that is supposed to create items and
                // Their .itm file works. It even updates the Master Item List
                // So the problem isn't there.

                // Read in one word at a time, one line at a time
		fin >> szWord >> szNameIn;
                // Read in one word at a time, one line at a time
		fin >> szWord >> filename;
                // Read in the next blank line
		fin >> szWord;

                // If this next part checks to be FALSE, shouldn't it keep
                // checking till it hits the end of the file?
		if(szNameIn == itemName)
		{
			// Go open the new file.
			fin.close();

			string szWord = "NULL";
			string tmpLabel = "NULL";
			int tmpNum = 0;

			fin.open(filename.c_str());
			fin.seekg(NULL, ios::beg);

			fin >> szWord;
			fin >> szWord;
			fin >> tmpLabel;

			item.SetItemName(tmpLabel);

			fin >> szWord >> szWord >> tmpLabel;
			item.SetItemType(tmpLabel);

			cout << "Item Loaded." << endl;
			Sleep(1000);
			bLoaded = true;
		}

		else
			cout << "Item Name not Found.";
			Sleep(1000);
			break;
	}

	system("cls");
} // End of GetItemInfo()

powell0

Apr 7, 2009 at 2:47 AM

Quote:
Original post by Dark_Glitch
// If this next part checks to be FALSE, shouldn't it keep
// checking till it hits the end of the file?

No it won't because in the else statement you have a "break", which will cause your code to leave the while loop. So your function will end after reading in the first three lines.

What you should be doing is checking for an end of file condition, and only reporting that the item wasn't found if you reach the end of the file. Something like:

if (fin.eof()){    cout << "Item Name not Found.";    break;}

powell0

Apr 7, 2009 at 2:54 AM

And actually it would probably be best to add the end of file check to the while loop condition itself:

while ( !bLoaded && !fin.eof() ){    // Do your processing here}

And then you can add error reporting after the loop.

Dark_Glitch

Apr 7, 2009 at 5:56 AM

Thanks and Thanks.

Zahlman

Apr 7, 2009 at 1:21 PM

Actually you're doing a few strange things in there.

0) If the initial file name is hard-coded, there's no point to creating a temporary std::string object for it. There's also almost never a point to using the .open() and .close() member functions. Just use the constructor to open the file and rely on the destructor to close it. When you find your "match", just open a separate ifstream for the other file.

1) If any of your path names have spaces in them, they'll only be read up to the first space. Use std::getline() to get the rest of a line.

2) Your variable names are very inconsistent. Don't bother with 'sz' prefixes. They're actually not the "proper" Hungarian prefix for std::string instances, anyway.

3) "NULL" is a strange value to use for initializing a std::string instance. Just use the default (i.e. an empty string).

4) In general, putting in extra words in your file format which you then skip over is a bad idea. If you want to "label" data within the file, you should do it in a way that allows the program to read the labels to ensure that things are what they're supposed to be (or even to allow the file creator some flexibility, e.g. to re-order the lines).

5) 'tmpNum' doesn't seem to be used anywhere.

6) Seeking to the beginning of a newly opened file is unnecessary. If the goal is to reset any error flags from a previous file opened with that stream, well... that's why you don't use .open() and .close() like that :)

7) Deliberately pausing all the time like that probably isn't such a good idea. Eventually you'll want to be able to load large files quickly.

8) Instead of storing data into an existing Item, have you considered constructing and returning and Item from the function? Also, instead of reading the whole file each time to construct a new Item, you might want to either (a) read the "catalog" file once and store this information somewhere else, or (b) write the function so that it creates every item in one go (you could then use the items as "prototypes" to create more, by duplicating them with the copy constructor). The code below illustrates both approaches, as well as using the constructor of Item to set up the Item instead of loading the "info" later. :)

9) Duplicating name info between the 'catalog' and the per-item file data seems like a bad idea. If one is wrong, who do you trust? I've fixed this by assuming it's removed from the catalog.

class Item {  // everything else  private: static map<string, Item> prototypes;  public: void loadPrototypes;};map<string, Item> Item::prototypes;Item::Item(ifstream& is){	std::string ignored_word;	is >> ignored_word >> ignored_word;	getline(is, m_name);	is >> ignored_word >> ignored_word;	getline(is, m_type);}void Item::loadPrototypes() {	ifstream fin("C:/Game/Items/MasterItemList.lis");	string ignored_word, item_filename;	// Use file I/O for loop conditions, instead of checking the	// stream state explicitly in a loop. See	// http://www.parashift.com/c++-faq-lite/input-output.html#faq-15.4	// and onward.	while (fin >> ignored_word)	{		getline(fin, item_filename);		if (!fin) { throw ios::failure(); } // file is corrupt.		// Construct a special fstream for the Item that will throw		// an exception if any of the expected file reading fails.		ifstream item_data(item_filename);		item_data.exceptions(ios::failbit | ios::badbit | ios::eofbit);		Item from_file(item_data);		prototypes[from_file.name] = from_file;	}}Item createItemNamed(const string& name) {	return prototypes[name]; // makes a copy automatically because we return by value.}

Dark_Glitch

Apr 8, 2009 at 3:54 PM

0) If the initial file name is hard-coded, there's no point to creating a temporary std::string object for it. There's also almost never a point to using the .open() and .close() member functions. Just use the constructor to open the file and rely on the destructor to close it. When you find your "match", just open a separate ifstream for the other file.

- The initial file name, I assume you mean the Master Item List, is hard-coded cause I want that to always be where the program will look for the locations of defined items. Maybe I'm looking at this wrong, but if I have the Item class constructor open that file whenever a new item is used, won't that cause a memory leak or something, or just waste resources? Also, with the default destructor automatically close the file or do I have to define a destructor to do that?

1) If any of your path names have spaces in them, they'll only be read up to the first space. Use std::getline() to get the rest of a line.

- This program is supposed to read one word at a time. I don't really like messing with getline(). It seemed easier that way to me.

2) Your variable names are very inconsistent. Don't bother with 'sz' prefixes. They're actually not the "proper" Hungarian prefix for std::string instances, anyway.

- I'm using my own prefixes, and I haven't cleaned up my code yet. :D

3) "NULL" is a strange value to use for initializing a std::string instance. Just use the default (i.e. an empty string).

- I initialized this variable to null last week cause I thought that was what was causing the problem. I know empty strings don't really cause problems as the compiler won't put anything into them, unlike int's and the like.

4) In general, putting in extra words in your file format which you then skip over is a bad idea. If you want to "label" data within the file, you should do it in a way that allows the program to read the labels to ensure that things are what they're supposed to be (or even to allow the file creator some flexibility, e.g. to re-order the lines).

5) 'tmpNum' doesn't seem to be used anywhere.

-It's used, I just didn't post where it gets used. I forgot to take that out.

6) Seeking to the beginning of a newly opened file is unnecessary. If the goal is to reset any error flags from a previous file opened with that stream, well... that's why you don't use .open() and .close() like that :)

- Ok, I'm confused here, how else am I supposed to open/close the file if I don't use .open()/close()? Are you saying that I should've just created a new stream?

7) Deliberately pausing all the time like that probably isn't such a good idea. Eventually you'll want to be able to load large files quickly.

- Those pauses were put in there so I can see if the item info was loaded or not. I plan on taking them out and adding a more efficient error checking/handling method when I work my way up there :D

8) Instead of storing data into an existing Item, have you considered constructing and returning and Item from the function? Also, instead of reading the whole file each time to construct a new Item, you might want to either (a) read the "catalog" file once and store this information somewhere else, or (b) write the function so that it creates every item in one go (you could then use the items as "prototypes" to create more, by duplicating them with the copy constructor). The code below illustrates both approaches, as well as using the constructor of Item to set up the Item instead of loading the "info" later. :)

- I just came to the conclusion earlier before reading this post that I should've just read in the catalog file and stored the info somewhere, but thanks for pointing that out. Also, thanks for the example, it helps. :)

9) Duplicating name info between the 'catalog' and the per-item file data seems like a bad idea. If one is wrong, who do you trust? I've fixed this by assuming it's removed from the catalog.

- The catalog only contains the name of the item, and its location in the game folder. The actual item file contains the item's info like stats, usage, etc.

private: static map<string, Item> prototypes;

What is that? I have never seen such a thing before.

This idea of mine was supposed to allow me to use this on a MUD or maybe one day on an online rpg, and allow me to make changes to items by either doing it in-game with 'admin' commands or just editing the database files manually.

Twisol

Apr 8, 2009 at 4:41 PM

// Quote: Dark_Glitch		else			cout << "Item Name not Found.";			Sleep(1000);			break;	}	system("cls");} // End of GetItemInfo()

I'm not sure if you spotted this already, but you need to have { and } around those three statements if you want them all to be a part of the else. Otherwise, it assumes only the output is part of the else, and the Sleep() and break statements are executed unconditionally.

Dark_Glitch

Apr 8, 2009 at 9:22 PM

It works when I compile it.

joshuanrobinson2002

Apr 8, 2009 at 11:09 PM

Right, it will compile. There is nothing wrong with it that the compilier will complain about. The problem is, only the first statement after "else" will be evaluated when the else condition occours. In other words:

//...// If this next part checks to be FALSE, shouldn't it keep                // checking till it hits the end of the file?		if(szNameIn == itemName)		{			// Go open the new file.			fin.close();			string szWord = "NULL";			string tmpLabel = "NULL";			int tmpNum = 0;			fin.open(filename.c_str());			fin.seekg(NULL, ios::beg);			fin >> szWord;			fin >> szWord;			fin >> tmpLabel;			item.SetItemName(tmpLabel);			fin >> szWord >> szWord >> tmpLabel;			item.SetItemType(tmpLabel);			cout << "Item Loaded." << endl;			Sleep(1000);			bLoaded = true;		} //end if//When szNameIn != itemName, your else condition is evaluated. But, without the //curly braces around all three statements, only the first one will be evaluated//The other two (Sleep(1000) and break) will always execute. Even when// szNameIn == itemName		else			cout << "Item Name not Found."; 		Sleep(1000); //<- this line is not part of the else condition.		break;       //<- neither is this one.	} //end ... while?

Also, as powell0 pointed out, as soon as you hit your break statement you exit the while loop regardless of bLoaded's state. The way it's written now, it looks like at the end of your very first loop iteration you're breaking out. Even when szNameIn == itemName.

If you add the curly braces around all three statments below else

//Like this...else{ //<-added   cout << "Item Name not Found.";    Sleep(1000);    break;       } //<-added

you're still going to hit that break statement and exit the while loop as soon as szNameIn != itemName.

The keyword you were probably looking for is actually "continue" and not "break." Continue will reevaluate the loop condition (!bLoaded) and run through the loop again from the top. Although, honestly, it really isn't necessary here since it's at the very end of your loop anyway.

Hope that helps.

[Edited by - joshuanrobinson2002 on April 8, 2009 11:09:53 PM]

Zahlman

Apr 8, 2009 at 11:53 PM

Quote:
Original post by Dark_Glitch
- The initial file name, I assume you mean the Master Item List, is hard-coded cause I want that to always be where the program will look for the locations of defined items.

What I meant was, there is no point to doing:

std::string filename = "this is always the same file name.txt";ofstream file(filename.c_str());

When you could just do:

ofstream file("this is always the same file name.txt");

If you wanted to declare the filename as a constant at a wider scope (perhaps global), either to make it easier to find (for maintenance work) or to reuse it (maybe other functions have to work with the same file, but it's still always just the one file that you use), then there is a point to having a separate variable for it. :) But as it stands, all you gain is the ability to label the text "this is always the same file name.txt" as the 'filename' - which is already obvious from context.

Quote:
Maybe I'm looking at this wrong, but if I have the Item class constructor open that file whenever a new item is used, won't that cause a memory leak or something, or just waste resources?

If would waste time, and possibly cause problems in a multithreaded program. System resources, not really. After every constructor call, the stream destructor is called, and the file is thus closed and all the memory associated with the stream is deallocated.

Quote:
Also, with the default destructor automatically close the file or do I have to define a destructor to do that?

The destructor of an fstream will close the file. That's what I was going on about when I was saying not to bother with .close(). "default destructor" is meaningless; a class can only have one destructor, and it takes no arguments. (Consider: If C++ allowed the destructor to have arguments, where would they come from?)

Quote:

6) Seeking to the beginning of a newly opened file is unnecessary. If the goal is to reset any error flags from a previous file opened with that stream, well... that's why you don't use .open() and .close() like that :)

- Ok, I'm confused here, how else am I supposed to open/close the file if I don't use .open()/close()? Are you saying that I should've just created a new stream?

I am saying just use a new stream, yes. Each stream will have its own scope and operate on its own schedule.

Quote:

9) Duplicating name info between the 'catalog' and the per-item file data seems like a bad idea. If one is wrong, who do you trust? I've fixed this by assuming it's removed from the catalog.

- The catalog only contains the name of the item, and its location in the game folder. The actual item file contains the item's info like stats, usage, etc.

The point is that there's no reason to have the item's name in both files. Which file should have the information, then? Well, It Depends. :)

Quote:

*** Source Snippet Removed ***
What is that? I have never seen such a thing before.

A natural way to store the information from the catalog.

std::map is a class which associates 'keys' with 'values'. It is defined in . I have made it 'static' so that only one map is created; it is associated with the class, rather than with individual instances.

Quote:

This idea of mine was supposed to allow me to use this on a MUD or maybe one day on an online rpg, and allow me to make changes to items by either doing it in-game with 'admin' commands or just editing the database files manually.

Yes; having the data in-memory in the map will make this much easier. You can change the contents of the map while the game runs, and then just save out the data (in a way parallel to how it was read in) when the game exits (or perhaps at regular intervals, or in response to an admin command, or...).

Dark_Glitch

Apr 9, 2009 at 1:40 PM

Awesome, and thank you very much again. I appreciate you talking the time out to respond and help me. I'll study map and then rewrite this. Thanks again Zahlman!

Dark_Glitch

Apr 9, 2009 at 1:51 PM

Quote:

Hope that helps.

It does actually, I keep learning new keywords from you guys. I have a feeling I need to learn a whole lot more. Thanks!

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.