• ### What is your GameDev Story?

Public Group

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

## Recommended Posts

I want to have some criticism about my Config parser about it's performance and the look of it. And i am thinking of slapping it together in a class later and be able to set values. Language: C Plus Plus Compiler: The standard one with Bloodshed Dev-C Plus Plus And this is somewhat related to a earlier topic i made. (A problem wich i solved now.) Anyway, beware of page height rape: main.cpp
#include &lt;cstdlib&gt;
#include &lt;iostream&gt;
#include &lt;fstream&gt;
#include &lt;sstream&gt;
#include &lt;string&gt;
#include &lt;map&gt;
#include &lt;StringTokenizer/StringTokenizer.cpp&gt;
#include &lt;boost/algorithm/string.hpp&gt;
#include &lt;algorithm&gt;

#include &lt;chjees/include.h&gt;
using namespace std;
using namespace boost;

////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////

//gVariables
int main(int argc, char *argv[])
{
int work = ParseConfig("test.ini");
if ( work = 1){ cout &lt;&lt; "Success!"; }
cout &lt;&lt; endl;
system("PAUSE");
return EXIT_SUCCESS;
}


include.h
/*
Make sure to include these:

#include &lt;fstream&gt;
#include &lt;sstream&gt;
#include &lt;string&gt;
#include &lt;algorithm&gt;
#include &lt;StringTokenizer/StringTokenizer.cpp&gt;
#include &lt;boost/algorithm/string.hpp&gt;
#include &lt;map&gt;

*/
//Defined: gVariables
//Function: ParseConfig(string File)
#ifndef _CHJEES_CONFIG
#define _CHJEES_CONFIG 1
#include "chjees/ini.cpp"
#endif


ini.cpp
using namespace std;
using namespace boost;

//Global variable map.
map&lt;string, string&gt; gVariables;

//Rest
int ParseConfig(string File); //Prototype

bool ParseCheck( string Text, string Text2 )
{
//Check if special
if ( Text == "Include" ){ ParseConfig( Text2 ); return 1; }
return 0;
}

int ParseConfig(string File)
{
//Variables
fstream fConfigFile( File.c_str(), ios::in );
string CurrentLine;
string fContent;
//Open File

//Check if open
if( fConfigFile )
{
while ( getline(fConfigFile,CurrentLine) ) //getline(fConfigFile,CurrentLine)
{
//If '//' ignore.
if ( (CurrentLine.empty() == 1) || (( CurrentLine.at(0) == '/' )  && ( CurrentLine.at(1) == '/' )) )
{
//Ignore, pick next line
//                 cout &lt;&lt; "Ignored" &lt;&lt; endl;
}else{
fContent += CurrentLine + "\n";
//fContent.append(fContent);
}
//            cout &lt;&lt; CurrentLine &lt;&lt; endl &lt;&lt; fContent &lt;&lt; endl;
//            system("PAUSE");cout &lt;&lt; endl;
}
fConfigFile.close(); //Close File
}else{
//Or else Fail!
return 0;
}

//Start parsing and assigning variables (Also include other files.)
stringstream ContentStream;
ContentStream.str(fContent);
string prefix,suffix;
int space_pos;
bool special;

//While loop...
while ( getline(ContentStream,CurrentLine) )
{
//Check if not empty
if ( !CurrentLine.empty() )
{
//Is not empty, continue

//Step 1: Split '='
StringTokenizer ConTok(CurrentLine,"=");
prefix = ConTok.nextToken();
suffix = ConTok.nextToken();

//Step 2: Clean up spaces.

//Prefix
trim_left(prefix);
trim_right(prefix);

//Suffix
trim_left(suffix);

//Check if not special, otherwise add to a map.
special = ParseCheck(prefix,suffix);
if (special == 0) { gVariables[prefix] = suffix; }
}
}
return 1;
}

External Dependencies: http://www.partow.net/programming/stringtokenizer/ http://www.boost.org/ Oh and here is a example config file:
//Test

Cheese = 1
Lalala = zomg string!
[Edited by - ChJees on February 11, 2008 3:05:17 PM]

##### Share on other sites
0) use source tags
1) don't include C++ source files
2) don't use names beginning with an underscore followed by an uppercase letter
3) the test for equality is ==
4) don't say if(bool == 1), say if(bool) (or at least, if(bool == true))
5) StringTokenizer?
6) check is a line has at least two characters before looking for '//'
7) pass std::string by const reference.
8) use std::ifstream, instead of std::fstream(const char *,std::ios::in)
9) std::fstream objects close() themselves, you don't have to
10) you don't propagate errors caused by included files
11) you suck the file line by line into a string, place it in a stringstream, then parse it line by line? Why not parse as you read?
12) why use a global? why not return a map representing the data read, or possibly pass a reference to a map to read the data into?
13) in a function that returns a bool, return true or false, not 1 or 0.

This is based on a quick scan of the source.

##### Share on other sites
Alright, I haven't read the full code, but I already see a mistake in your main() function. You#re always going to get "Success", because of if ( work = 1). You need if( work == 1 ). Also, since 1 represents true and if() parses for true or false, you could also just write if( work ). Any other value than 0 will make the if() execute.

##### Share on other sites
Quote:
 Original post by c4c0d3m0nAlright, I haven't read the full code, but I already see a mistake in your main() function. You#re always going to get "Success", because of if ( work = 1). You need if( work == 1 ). Also, since 1 represents true and if() parses for true or false, you could also just write if( work ). Any other value than 0 will make the if() execute.

Fixed that small 'typo' now. And code executes flawlessly. Waiting for more criticism though.

##### Share on other sites
Quote:
 Compiler: The standard one with Bloodshed Dev-C Plus Plus

The compiler should not matter if you are writing standards-respecting code.
Quote:
 beware of page height rape:

Quote:
 #include "chjees/ini.cpp"

Bad mojo. Your average make system would compile this file on its own anyway, which would result in duplicate symbol definitions.
Quote:
 //Global variable map.map gVariables;

So, you can't have more than one set of variables. Why this restriction? Why not simply have the ParseConfig function return the map?
Quote:
 bool ParseCheck( string Text, string Text2 )/* ... */ return 1; /* ... */ return 0;

C++ has literals 'true' and 'false' which are more readable and also are of the correct type. Also, pass your std::strings by constant reference instead of by value.
Quote:
 CurrentLine.empty() == 1

This could have been simply written as "CurrentLine.empty()", the additional "== 1" is both superfluous and is comparing a boolean to an integer.
Quote:
 CurrentLine.at(0) == '/'

This can be more simply written as "CurrentLine[0] == '/'".
Quote:
 CurrentLine.at(1) == '/'

This will explode if a line is equal to "/", because you'll be accessing a non-existing element.
Quote:
 Or else Fail!return 0;

In the C++ world, failure is represented with exceptions. If you expect failures to occur regularly enough, you should represent failure through the absence of returned value (using, for instance, boost::optional), though this certainly isn't the case in a configuration parser, where failure is indeed exceptional.

Ignoring for a moment that using return codes to indicate failure causes problems with functions that return values, and are generally only used when compatibility with C is required (a silly notion for a function which takes an std::string as argument), the C convention is to return 0 when the function is successful, and use non-zero error codes to indicate the cause of failure.
Quote:
 stringstream ContentStream; ContentStream.str(fContent);

stringstream ContentStream(fContent);
Quote:
 //While loop... while ( getline(ContentStream,CurrentLine) )

I'm glad you told me this is a "While loop", I certainly wouldn't have guessed myself!
Quote:
 special = ParseCheck(prefix,suffix);if (special == 0) { gVariables[prefix] = suffix; }

Note that 'ParseCheck' is a very generic and vanilla name, which forces you to comment the pair of lines. You could use a more explicit name instead:

if(IsVariableAssignment(prefix,suffix)) gVariables[prefix] = suffix;

Besides, note that special is never used before this line, and thus should have been initialized and defined here, instead of being defined one page above and initialized here. Besides, it's a boolean, so comparing it to zero makes no sense from a human standpoint, and the end result is more readable as if(!special) anyway.

##### Share on other sites

Anyway, i thought of passing Map as a argument when parsing and encasing this whole code into a class.

And a response to the second post.
Quote:
 you suck the file line by line into a string, place it in a stringstream, then parse it line by line? Why not parse as you read?
Tried before and did not find it efficent, and it is just a matter of preference.. (I failed horribly.)

##### Share on other sites
New code and i learnt how to use pointers in the process... (I am starting to like pointers :3.)

Still a lot of optimising to do though.

using namespace std;using namespace boost;//Global variable map.int ParseConfig(string File, map<string, string> &Variables);bool ParseCheck( string Text, string Text2, map<string, string> &Variables ){     //Check if special     if ( Text == "Include" ){ ParseConfig( Text2, Variables ); return 1; }     return 0;}//Rest //Prototypeint ParseConfig(string File, map<string, string> &Variables){    //Variables    fstream fConfigFile( File.c_str(), ios::in );    string CurrentLine;    string fContent;    //Open File        //Check if open    if( fConfigFile )    {        //Add everything except comments to fContent        while ( getline(fConfigFile,CurrentLine) ) //getline(fConfigFile,CurrentLine)        {            //If '//' ignore.            if ( (CurrentLine.empty() == 1) || (( CurrentLine.at(0) == '/' )  && ( CurrentLine.at(1) == '/' )) )            {                 //Ignore, pick next line//                 cout << "Ignored" << endl;            }else{                 //Add to global string                 fContent += CurrentLine + "\n";                 //fContent.append(fContent);            }//            cout << CurrentLine << endl << fContent << endl;//            system("PAUSE");cout << endl;        }        fConfigFile.close(); //Close File    }else{        //Or else Fail!        return 0;    }        //Start parsing and assigning variables (Also include other files.)    stringstream ContentStream;    ContentStream.str(fContent);    string prefix,suffix;    int space_pos;    bool special;        //While loop...    while ( getline(ContentStream,CurrentLine) )    {          //Check if not empty          if ( !CurrentLine.empty() )          {                //Is not empty, continue                            //Step 1: Split '='              StringTokenizer ConTok(CurrentLine,"=");              prefix = ConTok.nextToken();              suffix = ConTok.nextToken();                            //Step 2: Clean up spaces.                            //Prefix              trim_left(prefix);              trim_right(prefix);                            //Suffix              trim_left(suffix);                            //Check if not special, otherwise add to a map.              special = ParseCheck(prefix,suffix,Variables);              if (special == 0) {                            //New code              string *TempVar;              TempVar = &Variables[prefix];              *TempVar = suffix;                            //Variables[prefix] = suffix; <- Obsolete              }          }    }    return 1;}

##### Share on other sites
Quote:
 External Dependencies:...http://www.boost.org/

What's wrong with boost::program_options?

Why did you choose to write your own, rather than wrapping or utilizing that one?

##### Share on other sites
Quote:
Original post by Antheus
Quote:
 External Dependencies:...http://www.boost.org/

What's wrong with boost::program_options?

Why did you choose to write your own, rather than wrapping or utilizing that one?

Is it wrong to learn by making stuff ;) ? I see making this parser as a opportunity to learn the logic behind parsing.

##### Share on other sites
Quote:
 Original post by ChJeesI see making this parser as a opportunity to learn the logic behind parsing.

Formal grammar, see all related links in the article, as well as regular expressions.

If you cover that, you'll know almost everything there is to know about theory and practice behind parsing. It's one of the older and quite mature topics, simply because it forms basis for many everyday programming concepts.

• ### What is your GameDev Story?

In 2019 we are celebrating 20 years of GameDev.net! Share your GameDev Story with us.

• 13
• 9
• 15
• 14
• 46
• ### Forum Statistics

• Total Topics
634060
• Total Posts
3015300
×