🔒 Locked

problem reading from file

Started by A_B Apr 10, 2009 at 4:16 AM 7 replies 1.2k views

Original Post

Apr 10, 2009 at 4:16 AM

Hi, after several month, I return to this forum with another question. You were very helpful last time, s I hope for the same this time :p I want to read lines from a file in the form: name=value eg: CNFG_SCREEN_WIDTH=1152 CNFG_SCREEN_HEIGHT=864 To do this I wrote the following functions:

void GEclass::readDataFile(const char* filename)
{
	string line;
	string strname;
	string strvalue;
	
	DWORD Dname;
	float value;
	
	size_t equal;
	
	
	
	ifstream file (filename);
	if (file.is_open())
	{
		while(!file.eof())
		{
			getline(file, line);
			equal=line.find_first_of("=");
			
			strname = line;
			strname.erase(equal);
			
			strvalue = line;
			strvalue.erase(0, (equal+1));
			
			
			Dname = atoi(strname.c_str());
			value = (float)(atof(strvalue.c_str()));
			
			assignValueFromFile(Dname, value);
			
			line.clear();
			strvalue.clear();
			strname.clear();
		}
		
		file.close();
	}
	
	else
		MessageBox (NULL, L"ERROR - a configurationfile could not be opened.", L"VERSUTIA", MB_OK | MB_ICONEXCLAMATION);
	
	return;
}

void GEclass::assignValueFromFile(DWORD name, float value)
{
	switch (name)
	{
		case CNFG_SCREEN_WIDTH:
			screen_width = (int)value;
			break;
		
		case CNFG_SCREEN_HEIGHT:
			screen_height = (int)value;
			break;
	}
	
	return;
}

The DWORDS in assignValueFromFile are defined in another file like so: #define CNFG_SCREEN_WIDTH 111 #define CNFG_SCREEN_HEIGHT 112 I seem to be doing something wrong here though and I can't figure out what. There are no errors on compile, but the debugger show an access violation whenever I try to use the direct3d device, which is created in a function which uses values extracted from a file in the way described above. Before I implemented this file reading thing the program worked well. Thanks Alex

Aardvajk

Apr 10, 2009 at 5:17 AM

Maybe I misunderstand but you seem to be reading the string "CNFG_SCREEN_WIDTH", then passing it to atoi() and expecting atoi() to return the numeric value that CNFG_SCREEN_WIDTH is #defined to?

I'm a bit baffled. Are you running your config file through a preprocessor to replace the tokens with the numbers?

Xsy

Apr 10, 2009 at 7:13 AM

Well I think you simply misunderstood how 'atoi()' works.
'atoi' takes a string representation of a number and converts it to an integer.

  string str = "85";    int num = atoi(str.c_str());  //Value of num is now 85.

Now "CNFG_SCREEN_WIDTH" for example is just a string , and not a string number representation. What you can do is change the way you create your config file.
For example instead of : CNFG_SCREEN_WIDTH=1152 you could put :

  (CNFG_SCREEN_WIDTH)111=1152

When you read the line , you skip the whole brackets (they are for neatness or debugging ) and just read 111 as a string and than your value. When you atoi("111") you get 111 as an Integer value in you Dname variable.

Basically :

  //pseudo code  string str = ReadLine();  //Skips (CNFG_SCREEN_WIDTH) etc...  SkipBrackets(str);              //ReadName() returns "111" or "112"  as a string  int Dname = atoi(ReadName(str));     //ReadValue() returns "1152" or "864" as a string  float value = (float)atof(ReadValue(str));        assignValueFromFile(Dname,value)

Now in your assignValueFromFile the switch should correctly recognize 111 as CNFG_SCREEN_WIDTH or 112 as CNFG_SCREEN_HEIGHT.

Zahlman

Apr 10, 2009 at 7:51 AM

Or, instead of trying to have this incredibly messy 'assignValueFromFile' setup (BTW, did you know that C++ provides actual language syntax for constants so you don't have to use the preprocessor? It's just as easy to use, and lets your constant have a type and a scope, too), you could read the values into an associative array, and have the rest of the code use values from the associative array instead of checking all the different member variables.

Also, it is useless to have 'class' in the name of a class, I can't even begin to imagine what "GE" stands for, it's useless and unidiomatic to 'return' at the end of a void function, variables should be scoped as tightly as possible and ideally declared near first use (which would also avoid the need for clumsy .clear() calls), there is no need to close files explicitly except in very unusual circumstances, in C++ we have much better string-to-number conversion routines than atoi() and friends, there's no reason to demand a char* style string for the filename, using .eof() to control a loop doesn't work the way you want it to, there are simpler ways to do the actual parsing, and deciding how to present an error to the user should normally be left up to the calling function rather than the called function (for reasons of flexibility and of separation of responsibility).

// We remove screen_width, screen_height etc. and replace with:// std::map<std::string, float> values;bool Game::readDataFile(const std::string& filename){	ifstream file(filename.c_str());	if (!file.is_open()) { return false; }	string line;	while (getline(file, line)) {		stringstream parser(line);		string name;		float value;		getline(parser, name, '=');		parser >> value;		if (!parser) { return false; } // file data is corrupt		char garbage;		if (parser >> garbage) { return false; } 		// some extra stuff was found after the value		values[name] = value;	}	return true;}

Of course, the associative array might not be the right approach for you. But this should still give you a few good ideas about how to parse the file neatly.

A_B

Apr 10, 2009 at 10:49 AM

Thanks all for the replies,

Aardvajk, I just read a tutorial which suggested using atoi() for conversion to DWORD, maybe a bad tutorial then.

A couple of minutes after I posted this I realized how stupid it is to convert everything to DWORD's, I guess I just liked having a list of clear and self-explanatory flags for my function. I just kept the variable name a string and it works just fine now.

Zahlman, I read some other posts by you and noticed you're very keen on clean and neat code :p, thanks a lot for the tips, they did come in handy in cleaning up this mess a bit :p. Unfortunately the associative array won't work for my program, but it is a very useful technique which I will surely try to remember.

Thanks
Alex

rip-off

Apr 10, 2009 at 11:21 AM

Quote:

Aardvajk, I just read a tutorial which suggested using atoi() for conversion to DWORD, maybe a bad tutorial then.

You can use atoi() for converting a string to a DWORD, after all a DWORD is an integer type.

But the problem is that you were expecting that atoi() would understand how to convert preprocessor defined strings into integers. The problem is that by the time the compiler sees your code the preprocessor has already replaced the values in the source code (preprocessing is a separate compilation stage).

Quote:
Original post by A_B
Unfortunately the associative array won't work for my program...

Are you sure?

class Config{    typedef std::map<std::string, std::string> Map;public:    Config(const istream &stream)    {        // populate the "defaults" map        // load the other settings from the stream    }    template<class T>    T get(const std::string &name) const    {        Map::const_iterator it = map.find(name);        if(it == map.end())        {            return boost::lexical_cast<T>(defaults[name]);        }        return boost::lexical_cast<T>(it->second);    }private:    // defaults can be populated in the constructor    // map is populated from the file.    Map map, defaults;};class Video{     Video(const Config &config)     {         screen_width = config.get<int>("screen_width");         screen_height = config.get<int>("screen_height");         background_image = config.get<std::string>("screen_background");         // ...     }};

This makes it easy to add new configuration settings. An approach that doesn't use strings will probably be quite difficult. What approach are you using?

A_B

Apr 10, 2009 at 7:48 PM

OK, I'm sorry, yesterday was late (in this part of the world ) and I probably wasn't thinking too clearly.

An associative array is probably the best option, I didn't know about the "map" class before, it's pretty neat :).

One final question would be where I could find some reference on the "stringstream parser" function suggested by Zahlman, I can't seem to find a lot of information on it?

Thanks
Alex

Zahlman

Apr 11, 2009 at 2:06 AM

Quote:
Original post by A_B
One final question would be where I could find some reference on the "stringstream parser" function suggested by Zahlman, I can't seem to find a lot of information on it?

Thanks
Alex

"parser" is only a variable name.

Results 1 - 10 of about 125,000 for stringstream. (0.14 seconds) The first two results even have "C++ reference" in the page title. But I would suggest skipping down a couple hits to this one.

A_B

Apr 11, 2009 at 3:58 AM

[oh] seems I've got to re-learn how to read...
Thanks guys I get everything now.

Alex

Topic Locked

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