using iterators, ifstream, ofstream the way it's meant to be done

Question

I have a txt file containing a bunch of words, one per line. I need to read this file and put each word in a list then the user will be able to modify this list when done editing, the program will write the modified list in a new file.

Since it's object orinted c++, I'm gonna have two classes, one to read/write to file, and one to edit/mess with the list and the user.

with this approach in mind, here's my read function from the first class:

bool FileMgr::readToList(list<string> &l)
{
if(!input.is_open())
    return false;

string line;
while(!input.eof())
{
    getline(input, line);
    l.push_back(line);
}
return true;
}

keep in mind: input is opened at constructor. questions: is there a less redundant way of getting that damn line from istream and pushing it back to l? (without the 'string' in between). questions aside, this functions seems to work properly.

now the output function:

 bool FileMgr::writeFromList(list<string>::iterator begin, list<string>::iterator end)
{
    ofstream output;
    output.open("output.txt");
    while(begin != end)
    {
        output << *begin << "\n";
        begin++;
    }
    output.close();
    return true;
}

this is a portion of my main:

    FileMgr file;
list<string> words;
file.readToList(words);
cout << words.front() << words.back();
list<string>::iterator begin = words.begin();
list<string>::iterator end = words.end();
file.writeFromList(begin, end);

thanks for the help, both functions now work. Now regarding style, is this a good way to implement these two functions? also the getline(input,line) part I really don't like, anyone got a better idea?

Answer 1

As written, your input loop is incorrect. The eof flag is set after a read operation that reaches eof, so you could end up going through the loop one too many times. In addition, you fail to check the bad and fail flags. For more information on the flags, what they mean, and how to properly write an input loop, see the Stack Overflow C++ FAQ question Semantics of flags on basic_ios.

Your loop in readToList should look like this:

std::string line;
while (std::getline(input, line))
{
    l.push_back(line);
}

For a more C++ way to do this, see Jerry Coffin's answer to How do I iterate over cin line-by-line in C++? His first solution is quite straightforward and should give you a good idea of how this sort of thing looks in idiomatic, STL-style C++.

For your writeFromList function, as Tomalak explains, you need to take two iterators. When using iterators in C++, you almost always have to use them in pairs: one pointing to the beginning of the range and one pointing to the end of the range. It is often preferable as well to use a template parameter for the iterator type so that you can pass different types of iterators to the function; this allows you to swap in different containers as needed.

You don't need to explicitly call output.close(): it is called automatically by the std::ofstream destructor.

You can use std::copy with std::ostream_iterator to turn the output loop into a single line:

template <typename ForwardIterator>
bool FileMgr::writeFromList(ForwardIterator first, ForwardIterator last)
{
    std::ofstream output("output.txt");

    std::copy(first, last, std::ostream_iterator<std::string>(output, ""));
}

Answer 2

writeFromList should take a start iterator and an end iterator. This range-based approach is what the iterators are designed for, and it's how the stdlib uses them.

So:

bool FileMgr::writeFromList(list<string>::iterator it, list<string>::iterator end)
{
    ofstream output("output.txt");

    for (; it != end; ++it)
        output << *it;

    return true;
}

and

file.writeFromList(words.begin(), words.end());

You'll notice I improved your stream usage a little, too.

Ideally writeFromList would be generic and take any iterator type. But that's future work.

Also note that your .eof is wrong. Do this:

string line;
while (getline(input, line))
    l.push_back(line);