Min and max of numbers read from a file

I had to create this program, and I did it fine, all is working and stuff but I was wondering, what is better way of doing such thing? I want more efficient method.

Problem assignment:


In the file is unknown amount of numbers. Create a program, which
output is amount of numbers, the lowest number and the highest number.

#include 
#include 

/*
INPUT EXAMPLE:
12
15
-1
2
20
-4

OUTPUT EXAMPLE:
numbers: 6
minimum: -4
maximum: 20
*/
int main(){
    std::ifstream fin("input.txt");
    if (fin.is_open()){
        int n;
        int min,max;
        int i = 0;
        while (fin >> n){
            if (!n) break;
            if (i++ == 0){
            min = n; max = n;
            }
            if (n  max)
                max = n;
        }
        if (fin.good())
         std::cout << "numbers: " << i << '\n' << "minimum: " << min << '\n' << "maximum: " << max << '\n';
        else std::cout << "plz you even tryin'\n";
    }
    system("PAUSE");
    return 0xDEADBEEF;
}

Any improvements?

As far as performance is concerned, there are no problems in this code. It's using C++ streams in the idiomatic way, which is good.

If you're experiencing performance issues, you could try detaching from C streams at the start of your program:

int main(){
    std::ios_base::sync_with_stdio(false);

When you do this, you must then not mix C++ I/O (IO streams) with C I/O (printf and friends) in your program, but mixing these is bad practice anyway.

Here are some other comments to your code:

-
The names n and i don't say anything about the variables' use. I had to read through most of your code to figure out what i means. Always use descriptive names for your variables, at least for those whose value is supposed to persist for extended periods of time. I would be willing to accept the n, as it is basically one-shot, but i should be renamed at least to count or inputCount or something more descriptive.

-
Your looping and termination logic seems odd. You are using the correct construct while (fin >> n), looping while there's valid input left. But then you have this block:

if (!n) break;

This will cause the loop to terminate when a 0 is entered in the input. This behaviour does no match the assignment, and hence it represents a bug. Based on the assignment, you should only terminate when input ends, which will be correctly caught by the while loop. The if (!n) line should therefore be removed.

Related, your after-loop test for fin.good() is incorrect. If the loop is (correctly) terminated by being unable to read more numbers because the file has ended, both eofbit and failbit will be set on fin. The condition find.good() will therefore fail. You might want to augment the condition to check for EOF:

if (find.good() || fin.eof())

-
Indentation in the post-loop conditional is inconsistent with the rest of the program, and should be fixed. The same holds for the if (i++ == 0){ block (unless that was just screw-up of the code formatter here on Stack Exchange).

-
The return value of main is interpreted by the operating system as the return code of the program. On all OSes I know, 0 indicates success, non-zero indicates failure. You should therefore return 0 on successful program termination. Or just omit the return altogether, return 0; is implied in the main function in C++.

Or, if you want to be really beyond reproach, and comaptible even with OSes where 0 does not necessarily mean success, you can change it to return EXIT_SUCCESS;; this requires #include .

-
Error output (such as your "plz you even tryin'\n" message) should be sent to std::cerr (standard error stream, usually file descriptor 2), not to std::cout (standard output stream, usually file descriptor 1). A more helpful error message would be in order as well. Something like


Error when reading file 'input.txt', possibly encountered a non-number.

When printing this message, you should consider returning 1 (or EXIT_FAILURE) from main, instead of allowing fallback to the default success.

-
The outer if (fin.is_open()) could be removed. If the file is not open, fin >> n will fail, and you would at least print an error message instead of failing silently.

-
system("PAUSE"); is a very non-portable and extremely cumbersome solution looking for a problem. You're calling an external program just to wait for a key press. You should get rid of it altogether: your program is not interactive at all, it could run just fine as part of a script. Requesting input like this ruins that. Just drop that line altogether.

StackExchange Code Review Q#119261, answer score: 5