Sign in to follow this  

Selfmade simple config parser

This topic is 3593 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 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 <cstdlib>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <map>
#include <StringTokenizer/StringTokenizer.cpp>
#include <boost/algorithm/string.hpp>
#include <algorithm>

#include <chjees/include.h>
using namespace std;
using namespace boost;

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

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

include.h
/*
Make sure to include these:

#include <fstream>
#include <sstream>
#include <string>
#include <algorithm>
#include <StringTokenizer/StringTokenizer.cpp>
#include <boost/algorithm/string.hpp>
#include <map>

*/
//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<string, string> 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 )
    {
        //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);
              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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by c4c0d3m0n
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.


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

Share this post


Link to post
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:

[source][/source] instead of [code][/code]
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<string, string> 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 this post


Link to post
Share on other sites
Interesting read :).

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 this post


Link to post
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
//Prototype



int 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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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.

Share this post


Link to post
Share on other sites
Quote:
Original post by ChJees
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.

*** Source Snippet Removed ***
There were a number of suggestions made earlier that you haven't yet incorporated into your code. I'd recommend going back and taking another look at those.

Also:

1. Declare variables near first use, not at the top of the function.
2. Declare and initialize variables in the same statement rather than in different statements.
3. In the following code excerpt:
//New code
string *TempVar;
TempVar = &Variables[prefix];
*TempVar = suffix;

//Variables[prefix] = suffix; <- Obsolete
You actually had it right the first time (the use of pointers gains you nothing here).

Share this post


Link to post
Share on other sites
Quote:
Original post by jykYou actually had it right the first time (the use of pointers gains you nothing here).

Wrong, this lets me use any Map i want. Not just the predefined one i used before this Pointer solution.

Give me some time to implement and optimise all the suggestions. I do not code all day long you know :P.

I try to do as much as i can myself so i actually learn something when coding. Later when i start making real games so am i going to consider allready developed libraries and snippets.

Give a learning newb a break. (Yes, i consider myself a newbie STILL.)

Share this post


Link to post
Share on other sites
Quote:
Original post by ChJees
Quote:
Original post by jyk
You actually had it right the first time (the use of pointers gains you nothing here).

Wrong, this lets me use any Map i want. Not just the predefined one i used before this Pointer solution.
Hm, are you sure about that? Unless I'm missing something really obvious, your pointer-based code does the exact same thing that the commented-out line does, only in a more roundabout way.

Share this post


Link to post
Share on other sites

This topic is 3593 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