0

I found a solution but I believe there are better ways to do it. How can I improve my code without using complicated tools?

string Read(string& file) {
    ifstream in;
    string text;
    string s;
    in.open(file, ios::in);
    try {
        while (!in.eof()) {
            text.append(s);
            in >> s;
        }
    }
    catch (exception& ex) {
        cout << ex.what();
    }
    in.close();
    return text; 
}
Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39
Elena
  • 17
  • 3
  • The condition `while (!in.eof())` may be `false` even when the previous statement `in >> s` succeeded. I suggest that you read this: [Why is iostream::eof inside a loop condition (i.e. `while (!stream.eof())`) considered wrong?](https://stackoverflow.com/q/5605125/12149471) – Andreas Wenzel Jun 05 '22 at 19:32
  • 1
    In your question, you wrote: "How can I improve my code" -- What exactly do you mean? Do you want the the output of your program to be different? Or do you want to keep the behavior of your program and just make the code cleaner and more efficient? – Andreas Wenzel Jun 05 '22 at 20:59
  • Definitely more efficient and cleaner. – Elena Jun 06 '22 at 11:30
  • "Definitely more efficient and cleaner." -- So you are happy with the current output of your program and do not want it to be changed? – Andreas Wenzel Jun 06 '22 at 11:42
  • I think you mean if I want my code to ignore whitespace. I'd prefer it if it didn't ignore it, although It's not a necessity. – Elena Jun 06 '22 at 11:52
  • @Elena _I'd prefer it if it didn't ignore it [whitespace], although It's not a necessity._ - that's a perplexing answer. A text file with whitespace removed is illegible. Removing whitespace from a binary file corrupts it. What do you use the return value of the function for? – Maxim Egorushkin Jun 06 '22 at 21:06
  • @Elena DoyouseewhatIamgettingat? – Maxim Egorushkin Jun 06 '22 at 21:11
  • Just for a simple cout. I'm using my code to study, not for a project. – Elena Jun 06 '22 at 22:20
  • To print file you can just do `std::cout << std::ifstream(file).rdbuf()`. – Maxim Egorushkin Jun 06 '22 at 23:59

3 Answers3

4

Your code reads whitespace-separated words, discards the whitespace, and concatenates all words into one string with no whitespace. You probably want to read the file content verbatim.


One way to read an entire file into a std::string without a loop is to use std::string constructor which takes two iterators - the constructor does the loop for you. Invoke it with std::istreambuf_iterator:

#include <string>
#include <fstream>
#include <iterator>
#include <stdexcept>

std::string read(std::string filename) {
    std::ifstream file(filename, std::ios_base::binary | std::ios_base::in);
    if(!file.is_open())
        throw std::runtime_error("Failed to open " + filename);
    using Iterator = std::istreambuf_iterator<char>;
    std::string content(Iterator{file}, Iterator{});
    if(!file)
        throw std::runtime_error("Failed to read " + filename);
    return content;
}

Another alternative is to map the file into memory (zero-copy method), e.g. using boost::iostreams::mapped_file, that's as clean and efficient as it ever gets. The method is faster for files larger than ~100kB, benchmark to get hard numbers.

An optimization would be to populate all the pages of the file mapping immediately, rather than with demand paging on first access.

Example:

#include <iostream>
#include <boost/iostreams/device/mapped_file.hpp>

int main() {
    using boost::iostreams::mapped_file;
    mapped_file content("/etc/lsb-release", mapped_file::readonly);
    std::cout.write(content.const_data(), content.size());
}
Maxim Egorushkin
  • 131,725
  • 17
  • 180
  • 271
  • Seems like the final `!file` check will always be true, as even when there's no error, reading the string will reach the end-of-file and set the eof indicator on the stream. – Chris Dodd Jun 05 '22 at 19:51
  • @ChrisDodd `!file` is unaffected by `file.eof()`, see [std::basic_ios::operator!](https://en.cppreference.com/w/cpp/io/basic_ios/operator!) (table row 5, 1-based). – Maxim Egorushkin Jun 06 '22 at 04:35
2

The simplest way (which often times means the best way) is to read the file one character at a time

string text;
char ch;
while (file.get(ch)) {
    text += ch;
}
john
  • 85,011
  • 4
  • 57
  • 81
1

The loop

while (!in.eof()) {
    text.append(s);
    in >> s;
}

is wrong, because the condition while (!in.eof()) may be false even when the previous statement in >> s succeeded. It would be better to write while (!in.fail()) instead of while (!in.eof()).

However, it would be clearer and slightly more efficient to write the loop like this:

while ( in >> s ) {
    text.append(s);
}

This loop condition indirectly uses in.operator bool(), which is equivalent to !in.fail().

The reason why this loop is a bit more efficient than the other loop is that in the first iteration of the other loop, an empty string was always appended. This is no longer the case in this loop.

Although you did not state this in the question, in the comments section of your question, you stated that it would be preferable for the whitespace characters to be retained instead of being discarded. In that case, you should not be using operator >> for reading input, which reads one word at a time, but you should rather be using std::getline, which reads one line at a time:

string Read(string& file) {
    ifstream in;
    string text;
    string s;
    in.open(file, ios::in);
    while ( getline( in, s ) ) {
        text.append( s );
        text.append( "\n" );
    }
    return text; 
}
Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39
  • @MaximEgorushkin: Your assumption of there being whitespace at the end of the file is somewhat justified, though. According to POSIX, files are supposed to end with a newline character: [Why should text files end with a newline?](https://stackoverflow.com/q/729692/12149471) – Andreas Wenzel Jun 05 '22 at 20:40
  • IMO, the author wants to read the file verbatim. I cannot think of a reasonable use case for reading a text file and discarding all whitespace. Apart from training a model to recover whitespace, but that requires both the original text and the text with no whitespace. – Maxim Egorushkin Jun 05 '22 at 20:53
  • @MaximEgorushkin: You are probably right. I have now asked OP for clarification. I suspect that OP is in the same class and has the same assignment as the person who asked [this similar question](https://stackoverflow.com/q/72510149/12149471) less than one hour beforehand. Unfortunately, the person who asked that question was also unclear in what they wanted exactly. – Andreas Wenzel Jun 05 '22 at 21:06
  • 1
    @MaximEgorushkin: According to OP's response, it appears you are right that OP does not want the whitespace to be discarded. – Andreas Wenzel Jun 06 '22 at 14:56