2
\$\begingroup\$

I wrote the following code to read a config file line by line and split each line into 2 parts and store them separately. The split is done on the basis of a delimiter =:

std::ifstream configfile(path_string.c_str());
std::string line;

std::vector<std::string> linesOfConfigFile;
while (std::getline(configfile, line))
    linesOfConfigFile.push_back(line);

configfile.close();

std::string currentItem;
char delimeter = '=';

std::vector<std::string> RHS;
std::vector<long long> LHS;

bool flag = false;
for (int i =0; i<linesOfConfigFile.size(); i++)
{
    std::stringstream singleLine(linesOfConfigFile[i]);

    while(std::getline(singleLine, currentItem, delimeter))
    {
        if(flag == true) 
        {
            RHS.push_back(currentItem);
            flag = false;
        }
        else if (flag == false)
        {
            long long orderbookId = 0;
            <some library class>::toNumeric(currentItem, orderbookId);
            LHS.push_back(orderbookId);
            flag = true;
        }
    }
}

for(int i=0; i<RHS.size(); i++)
    StorageMap.insert( std::pair<long long, std::string>( LHS[i], RHS[i]) );

The review comment I got was this:

The below code can be written in a better way, Need not use a Boolean flag to differentiate RHS and LHS. The outer for loops should be sufficient.

I cannot understand what difference he wishes to have. Can someone please help me?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Have you tried asking him? \$\endgroup\$ – Gareth Rees May 20 '13 at 15:10
  • 1
    \$\begingroup\$ As an aside: when branching on booleans, you don't have to explicitly test whether they're true or false. Instead: if (flag) { ... } else { ... } flag = !flag. Note that flipping flag state is a common operation and can be done outside the conditional. \$\endgroup\$ – amon May 21 '13 at 7:01
  • 1
    \$\begingroup\$ what you did is an Antipattern: you perform a test with predictable outcome. You know that first comes LHS, and then comes RHS. Your code should reflect this explicitly, not pretend that LHSs and RHSs come in all kinds of different orders. They don't. The change here would be loop unrolling - performing two getlines on each step, first with a "=" delimiter, the second without it. Thus the code will explicitly and clearly reflect the nature of the situation; the knowledge won't be hidden in the obscure mechanics of getline stopping on end-of-line too, having failed 2 find the delim. \$\endgroup\$ – Will Ness Jun 20 '13 at 9:18
  • \$\begingroup\$ @WillNess Thanks, why don't you add that as an answer. \$\endgroup\$ – Chani Jun 21 '13 at 5:26
3
\$\begingroup\$

I wrote the following code to read a config file line by line and split each line into 2 parts and store them separately.

So there are two parts on each line, the stuff before the =, which is stored as a string, and the stuff after the =, which shall be parsed as an integer and stored as a long long.

Thus you can handle each line on its own completely, without using a flag to determine at which part of the line you currently are:

for (int i =0; i<linesOfConfigFile.size(); i++)
{
    std::stringstream singleLine(linesOfConfigFile[i]);

    // get first part of the line
    std::getline(singleLine, currentItem, delimeter);
    // store it
    RHS.push_back(currentItem);

    // Now get the next part of the line
    std::getline(singleLine, currentItem, delimeter);
    // parse it
    long long orderbookId = 0;
    <some library class>::toNumeric(currentItem, orderbookId);
    // and store the result
    LHS.push_back(orderbookId);

    // done with this line :D
}

This supposes of course that the config file is well-formed, and the format requires that each name = value is on a separate line.

If the config file is malformed, or the file format is not as simple, this version may fail where yours wouldn't (for example

name =
value

would work with yours), but neither version does robust error handling, and both would fail on most malformed inputs. Config files can generally be assumed to follow the required format, so the absence of error handling probably isn't a serious problem.

\$\endgroup\$
2
  • \$\begingroup\$ when I call std::getline(singleLine, currentItem, delimeter); for the second time; wont it again return me the same string it returned the first time ? \$\endgroup\$ – Chani May 24 '13 at 12:31
  • \$\begingroup\$ No, singleLine is a stringstream, it keeps trace of what it has already used, and delivers the next item each call (while there is one, of course). \$\endgroup\$ – Daniel Fischer May 24 '13 at 13:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.