Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my first attempt to read the contents of a file that consists 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>();
}

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

share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

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.
share|improve this answer
    
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) –  Aybe Feb 7 '13 at 12:55
1  
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. –  Yuushi Feb 7 '13 at 12:59
    
Okay great, thanks a lot for your explanations ! –  Aybe Feb 7 '13 at 13:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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