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

Question

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

vector<string> CReadFileTest::ReadFile( const char * filename )
{
 ifstream ifs(filename, ifstream::in);
 if (ifs)
 {
 vector<string> list ;
 while (ifs.good())
 {
 string s;
 std::getline(ifs, s,'0円');
 list.push_back(s);
 }
 ifs.close();
 return list;
 }
 return vector<string>();
}

1. Does this code snippet follows good coding practices?
2. Is it correct to return a vector type and an empty vector in case of errors?

Answer 1

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<T> type
vector<string> 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<vector<string>, bool> or pair<vector<string>, 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<string> 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.}

• aybe: Thank you, one thing however, can you explain why std::getline(ifs, s,'0円') return value only works when inside the while() statement ? When I try to check its value with something like auto i = std::getline(ifs, s,'0円'); it tells me that it is inaccessible. None of the docs I've looked at do mention that it returns the numbers of characters read but rather the input stream. (Msdn, Cppreference.com, Cplusplus.com)
• Yuushi: Sorry, I've updated my post - it doesn't return how many characters it reads, it returns a reference to the stream as you've said.