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?

Fix the organisation rather than apologising for the lack of it: you have all the time in the world before submitting your question. — Lightness Races in Orbit, Mar 19 '11 at 03:29
is this a good way to one-line the readToString function? copy(istream_iterator(input), istream_iterator(), inserter(l, l.end())); — Edoz, Mar 19 '11 at 05:06

score 7 · Answer 1 · edited May 23 '17 at 12:14

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, ""));
}

Jerry Coffin's answer is a perfect example of "how to make your code into cool, modern, C++... and make it disgusting and illegible in the process". Completely unnecessary when this one-line loop body will do the job. — Lightness Races in Orbit, Mar 19 '11 at 03:34
(Well, ok, his answer covers more than what the one-line loop body does.) — Lightness Races in Orbit, Mar 19 '11 at 03:36
Useful sure. Utterly illegible and ultimately unnecessary in most cases? That too. — Lightness Races in Orbit, Mar 19 '11 at 17:01

Lightness Races in Orbit · Answer 2 · 2011-03-19T03:36:11.257

3

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);

edited Mar 19 '11 at 03:36

answered Mar 19 '11 at 03:30

Lightness Races in Orbit

378,754
76
643
1,055

thanks, that was really helpful. so somethinf like writeFromList(startIter,endIter) and when i call it it's writeFromList(list.begin(), list.end()), right? – Edoz Mar 19 '11 at 03:32
@Edoz: I never suggested that you should reformat "the whole thing". I pointed out specific points of possible improvement. – Lightness Races in Orbit Mar 19 '11 at 03:35
no you were right, it sucked as i posted it. now it's better. – Edoz Mar 19 '11 at 03:49

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

2 Answers2