3
\$\begingroup\$

I am never to sure when it comes to ifstream and reading lines. I am often confused with the good(), bad(), eof() and so on.

Could anyone tell me if I am doing it right?

int parseLine(std::ifstream & _file)
{  

  while( std::getline( _file, line, '\n' ).good() && !_file.eof() )
  {
     // some treatment
  }

  return 0;
}
\$\endgroup\$
11
\$\begingroup\$

Too complicated. Just say:

for (std::string line; std::getline(_file, line); )
{
    // process "line"
}

The function std::getline, like most iostreams operations, returns a reference to the stream object itself, which can be evaluated in a boolean context to tell you whether it is still good, i.e. whether the extraction operation succeeded.

Also, the function shouldn't be called "parseLine", but "parseAllLines" or "parseStream".

\$\endgroup\$
  • 1
    \$\begingroup\$ I like it, easy and super clear \$\endgroup\$ – qdii Aug 19 '12 at 8:29
6
\$\begingroup\$

I prefer:

void parseLine(std::ifstream& file)
{  
    std::string line;
    while(std::getline(file, line))
    {
       // some treatment
    }
}

Comments on your code:

int parseLine(std::ifstream & _file)
                        //   ^^^  be careful with identifiers that start with _

Do you know all the rules? They are non trivial so best to just avoid '_' as a leading character.

This while loop does not buy you anything.

while( std::getline( _file, line, '\n' ).good() && !_file.eof() )

The result of std::getline() is a reference to a stream. When used in a boolean context it will be converted to a bool automatically (using the cast operator). This conversion will call good() so there is no point in calling it manually.

If the file is good() then eof() will not fail.

If you always return the same value

return 0;

Then why have a return value.

One thing about code it should be obvious what the code is doing with the need for extra comments. Comments mean the code is complex and needs additional explanation. But to make the code easy to read you should also use identifiers that convey some meaning.

void parseLine() // looks like it will read one line.

The term parseLine() is actually misleading here. As you don't parse a line. You parse the whole file.

\$\endgroup\$
  • \$\begingroup\$ Yes I do know the rules you are referring to :) my habit is to name _ variable the input variables and variable _ the output one. I pay great attention not to use capital letters there :) \$\endgroup\$ – qdii Aug 17 '12 at 21:03
  • 2
    \$\begingroup\$ @gdii break your habit. I don't know anyone that wouldn't frown on that. \$\endgroup\$ – David Aug 18 '12 at 3:16
  • \$\begingroup\$ @Dave And I can’t see why. Using underscores to prepend variable names is used in other conventions to signal class attributes. Is it bad to distinguish output parameters from input parameters? \$\endgroup\$ – qdii Aug 19 '12 at 8:28
  • \$\begingroup\$ @qdii For the most part output params are dated. That's what return values are for. That's not a 100% rule, but I only find myself making an output param maybe once a year. You're solving a problem with that notation that doesn't exist. Even if output params were common, functions should be simple enough (and split apart if they aren't) and named well enough so that it's immediately evident what each param is for. \$\endgroup\$ – David Aug 19 '12 at 13:19
5
\$\begingroup\$

At least IMO, the best way to read all the lines from a file is to start with a small proxy class:

class line { 
    std::string data;
public:
    operator std::string() { return data; }
    friend std::istream &operator>>(std::istream &is, line &d) { 
        return std::getline(is, d.data);
    }
};

Then read your lines with an istream_iterator, such as:

std::vector<std::string> lines((std::istream_iterator<line>(infile)),
                                std::istream_iterator<line>());

Or:

std::transform(std::istream_iterator<line>(infile),
               std::istream_iterator<line>(),
               some_output_iterator,
               some_function);
\$\endgroup\$
  • \$\begingroup\$ +! Not bad - I've always missed a "getline"-type istream iterator! \$\endgroup\$ – Kerrek SB Aug 19 '12 at 8:14
  • \$\begingroup\$ By the way, what's the point of the data member? \$\endgroup\$ – Kerrek SB Aug 19 '12 at 8:15
  • 1
    \$\begingroup\$ @KerrekSB: It temporarily holds the line you just read, until the line gets converted to a string, which returns it. Rereading it, I've left out an important point. Will edit. \$\endgroup\$ – Jerry Coffin Aug 19 '12 at 8:17
  • \$\begingroup\$ By the way, your vector is temporary... is that intentional? \$\endgroup\$ – Kerrek SB Aug 19 '12 at 8:34
  • \$\begingroup\$ @KerrekSB: No, not particularly. \$\endgroup\$ – Jerry Coffin Aug 19 '12 at 8:35

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.