Reading contents of a file consisting of strings separated by a null character

This is my first attempt at reading the contents of a file consisting of strings separated by a null character.

vector CReadFileTest::ReadFile( const char * filename )
{
    ifstream ifs(filename, ifstream::in);
    if (ifs)
    {
        vector list ;

        while (ifs.good())
        {
            string s;
            std::getline(ifs, s,'\0');
            list.push_back(s);
        }

        ifs.close();

        return list;
    }

    return vector();
}

• Does this code snippet follows good coding practices?

• Is it correct to return a vector type and an empty vector in case of errors?

A couple of issues:

while (ifs.good())

Don't loop on good() (or eof()). This is one of the more annoying parts of the C++ iostream library. This should be:

while(std::getline(ifs, s, '\0'))

std::getline returns a reference to its input stream (std::basic_istream&). How this actually checks the stream state and continues looping is pretty complicated* - suffice to say this is the canonical way of doing it. If you're really being good about your error checking, you'd then call ifs.readstate() after your while loop and check the fail and error flags aren't set.

Secondly, this creates a new string every time around the while loop. Instead, you probably want:

//Note you probably shouldn't call it list, because of the std::list type
vector v; 
string s;

while(std::getline(ifs, s, '\0')) {
    v.push_back(s);
}

There are a few ways of signalling that an error has occurred - throwing an exception is one possibility. Returning a pair, bool> or pair, int> indicating either success (for bool) or number of characters read (for int) is another possibility - this is basically an "error code" way of dealing with the problem. Error handling is always tricky, unfortunately.

Either way, currently you return a vector whatever happens, so you might as well construct it outside the if test:

ifstream ifs(filename, ifstream::in);
vector v;

if(ifs) { ... }

return v;

• What happens that it calls the ifstream's operator void*() which tests all error flags and eof, returning a nullptr if any of them are set, otherwise a non-zero pointer. See also std::getline returns.

StackExchange Code Review Q#21400, answer score: 3