4
\$\begingroup\$

I've made a program which reads from the console and writes it in a .txt file. This programm should write in a file with \n.

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

int main() {
    ofstream fich("file.txt");

    if (!fich)
        cerr << "Error!\n";

    char fst = 'l';
    string c;
    while (fst != '\0') {
        getline(cin, c);
        fst = c[0];
        fich << c << endl;
    }

    fich.close();

    return 0;
}

This program ends when the user inputs nothing and then a \n, because, doing that, the string c will only have a \0 end of string element.

I was wondering if there is any way of doing this more efficiently, maybe without needing a "void \n" line to finish the program.

\$\endgroup\$
9
  • \$\begingroup\$ I guess main2() is a typo that should be main(), right? \$\endgroup\$ – uli May 21 at 6:37
  • 1
    \$\begingroup\$ c[0] on an empty string is not guaranteed to be \0! Your test should be: while(getline(std::cin, c)) { Then you can end input by typing the EOF control code for your platform. <ctrl>-D on linux. This also makes it work nicely with files piped to the standard input. \$\endgroup\$ – Martin York May 26 at 22:40
  • \$\begingroup\$ @uli It was a typo, i corrected it, aprecciate it \$\endgroup\$ – And9686 May 29 at 15:46
  • \$\begingroup\$ @MartinYork Isn't the string empty when I create it? I thought that if it's empty then the \0 element should be in string[0]. \$\endgroup\$ – And9686 May 29 at 15:54
  • 1
    \$\begingroup\$ You are thinking of a C-String. The C++ std::string class does not use a null terminator. Accessing at size() using operator[] is undefined behavior. Now there is a method that will give you a null terminated string that looks like a C-String call c_str() this guarantees a null terminated string. But why not just call empty() to test for an empty string. \$\endgroup\$ – Martin York May 29 at 16:50
4
\$\begingroup\$

Error Handling

You check whether the output file was opened correctly, but all you do is output a message (you don't even say what's wrong!) and then you keep on ignoring it. What if file.txt is a directory or read-only? I guess you haven't tested these cases. By default, throw an exception if you can't continue at some point. In this case, throw std::runtime_error("failed to open output file").

Loop Condition

You are reading until the line is empty. This is okay-ish, but why don't you just check whether c.empty() and then break from the loop? Another case is that the end of the input is reached. In that case, the streamstate is set to failed. I'm not sure what happens in getline() then (read the docs at cppreference.com), but if it just doesn't change the string passed by reference, you may end up in an endless loop.

As an alternative I would use getline(cin, c) as loop condition. If this succeeds, you have received a line which can then write to the output file. Otherwise, you either had an error or you reached the end of the input.

Closing Filestreams

If you didn't call fich.close(), when would it be closed? However, consider the possibility that the file can't be written fully. You'll never notice this, neither with your code nor if you let the destructor do its work. So, what I'd rather do there is a call to fiche.flush() followed by a check of the streamstate.

Efficiency

I wouldn't bother with that at the moment. Point is, you're still learning to walk, so don't try to run yet. Still, what you're doing is inefficient. Firstly, writing in size of lines is useless, because you're just moving bytes from one file to the other. Secondly, endl implies a newline and a flush. So this is not really a cheap operation! In addition, there is a lot of stuff going on in C++ IOStreams that there's a whole book about their internals. Lastly, copying files can sometimes be done more efficiently using OS features which will avoid looking at the data more than necessary.

\$\endgroup\$
3
  • \$\begingroup\$ Yes a file will be closed correctly. The std::ofstream destructor calls close. In "most" cases not calling close is the correct action. Also manually calling flush is "usually" the wrong thing to do. The system implementation will auto flush when needed; manually calling flush is the cause of most speed issues in beginners C++ application. \$\endgroup\$ – Martin York May 26 at 22:41
  • \$\begingroup\$ Speed issues in beginners' applications? Uselessly flushing is a source for bad performance, that much I agree with. However, if you don't flush your stream after you're done writing, you don't know whether the data was written. If there is buffered data that doesn't fit on the storage, you will only get corrupted data and you may not even notice until it's too late. \$\endgroup\$ – uli May 27 at 6:48
  • 1
    \$\begingroup\$ "I'm not sure what happens in getline() then (read the docs at cppreference.com), but if it just doesn't change the string passed by reference, you may end up in an endless loop." The string is cleared first. So if there is an EOF or other problem, you will read an empty string. \$\endgroup\$ – JDługosz May 27 at 13:48
3
\$\begingroup\$

The \0 does not show up as a legal element of the string. In an empty string, doing c[0] is an error!

You don't need fst at all. You should use c.empty() to check for an empty string.

If you're not on a Unix-like system, be sure to open the file in Text mode, or you'll end up with stray \r characters in your string.

Your names, fst, fich, are confusing. Using c for a string is unusual and confusing to experienced programmers.

Don't write using namespace std;.

You can use a mid-decision loop to simplify your variables and logic. Something like:

for (;;) {
    getline (cin, s);
    if (s.empty()) break;
    outfile << s << '\n';
}

As as already been pointed out, the whole idea of reading and writing a line at a time is not "efficient". It might be what you need, though, for an interactive program where the user is typing something. If you are implementing something like cat though it is less efficient than reading entire blocks using low-level primitives. The iostream implementation reads the file in a low-level way, and getline goes through that data (that's already been read into memory) just to locate the end-of-line characters; then copies that to another string. If you just wanted to copy everything, that's clearly extra overhead.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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