Min and max of numbers read from a file

Question

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 <iostream>
#include <fstream>
/*
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 < min)
 min = n;
 else 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?

Answer 1

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:

1. 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.

2. 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())
   

3. 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).

4. 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 <cstdlib>.

5. 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.

6. 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.

7. 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.

• cst1992: It seems the OP is using DevCpp. That's the only place where I've seen system("PAUSE") being used to wait for the user. I'm unsure whether getchar() works or not.
• Angew is no longer proud of SO: @cst1992 Which changes nothing on the fact that it's colossally bad practice :-) And with the plethora of free IDEs available, including Visual Studio (via Community Edition), a bad IDE should not be an excuse for bad code.
• cst1992: Agree. I haven't used DevCpp in a while; I either use Netbeans or Eclipse or plain Vim and gcc. Also, I don't know why, but DevCpp has abysmal error messages.

Answer 2

Performance: Fine

Since you asked whether there was a more efficient way to do this: basically, not in a significant sense.

Looking at asymptotic performance first: the problem is essentially linear (O(n)): to find the maximum and minimum of a set of numbers, you have to examine all the numbers at least once. And your solution is indeed linear. So your solution is asymptotically as good as it can be.

On a micro level, you aren't doing anything particularly daft which would cost you performance. You might be able to micro-optimise things with profiling (maybe on some platforms pulling the conditional on the value of i out of the loop would have a tiny improvement in performance, but meh). And maybe if you were hilariously clever you could beat the performance of the stdlib integer-conversion functions, but that's a silly place to go to.

It's hard to imagine a situation in which improving the performance of this program further would make sense.

Given that you also asked for "any improvements?", I've listed some more general reviewing below:

Bug/misfeature: Zero terminates input

As it stands, your loop will stop reading numbers as soon as it reads a number which is zero (if (!n) break;). This seems like an odd behaviour, as zero is a reasonable input integer. The program also needs the input to be zero-terminated (so---at least on my machine---the example input produces the plz you even tryin' error message).

Bug/misfeature: Incorrect EOF handling

Using fin.good() to check for errors means that you will produce an error message if the program reached the end of the input file. The program reaching the end of the input file seems like intended behaviour. You can use the more fine-grained error messages (such as fin.eof(), fin.failbit(), etc) to determine why the fin >> n operation failed and broke you out of the loop.

Weirdness: Unconditionally returning 0xDEADBEEF

There's nothing to stop you returning 0xDEADBEEF. And it's a very witty return value. But it's conventional for programs to return 0 when they execute successfully, and nonzero otherwise.

• cst1992: Your post seems to be incomplete. Please complete it.

Answer 3

Here are some things that may help you improve your program.

Avoid hardcoding filenames

This program would be a bit more usable if it either read from std::cin or was passed a filename as a parameter. Right now, it's quite inflexible because the input filename is hardcoded.

Fix the bug

Unless it's specifically disallowed to have the number 0 in the input stream, this line is a bug and should simply be deleted:

if (!n) break;

Initialize variables when they're declared

In C++, it's usually advantageous to initialize variables when they're declared. This allows us to always count on some kind of rational value for it, regardless of what happens in the rest of the program.

Don't use system("PAUSE")

There are two reasons not to use system("cls") or system("PAUSE"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or Color, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and color() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() {
 getchar();
}

Omit the return statement in main

Generally, (although not guaranteed by the standard), a nonzero return value is understood by the operating system to mean that an error has occured, while in this case, the code always returns a non-zero value. Better would be to simply delete the return. When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Use standard algorithms

There are std::min and std::max which could be used for this program.

Putting it all together

Here's one way to rewrite the code so that it uses all of the suggestions above:

#include <iostream>
#include <limits>
#include <algorithm>
int main() {
 unsigned n{0};
 int min{std::numeric_limits<int>::max()};
 int max{std::numeric_limits<int>::min()};
 int number;
 while (std::cin >> number) {
 ++n;
 min = std::min(min, number);
 max = std::max(max, number);
 }
 std::cout << "numbers: " << n << "\n";
 if (n) {
 std::cout << "minimum: " << min 
 << "\nmaximum: " << max 
 << '\n';
 }
}

Answer 4

• Assign min and max to avoid the need for special treatment during the first iteration:

  int min = std::numeric_limits<int>::max();
  int max = std::numeric_limits<int>::min();
  

  Carefully crafted initial values are almost always superior to additional branches, which increase the complexity of the program in the eyes of the reader and potentially decreases performance drastically, if performance is critical and the CPU cannot accurately predict the branch to take based on the branching code generated by the compiler.

• Your program stops reading and counting numbers once it encounters the number 0:

  if (!n) break;
  

  Is that intentional? The task description doesn't mention any special treatment of 0.

• Avoid the use of system(3) (or Microsoft's implementation, since pause is a CMD command). It's unportable, since you don't know which command interpreter you'll encounter, and prone to security issues when paired with user input. There are better, yet platform dependent ways (or even cross-platform with the help of libraries) to spawn a sub-process or even run a carefully crafted command in an explicitly selected shell interpreter.

  Since I used Visual Studio myself to write console applications in the past, I think I know what you want to achieve here. Instead of adding a hack to your program that will hamper its use outside of Visual Studio, you should look for a solution to this VS-related issue. How about this StackOverflow question: How to keep the console window open in Visual C++?

• It is customary to use the return status, i. e. the value returned from main, to report the success of the requested operation. 0 means success, any other value represents a potential issue.

  You should return a non-zero value if the check for fin.good() fails and 0 otherwise.

• Angew is no longer proud of SO: You should mention that the limit-based initialisation actually requires dropping the else from the OP's if (<) else if (>) construct.

Answer 5

I'd factor out the algorithm into a function, unfortunately you can't use std::minmax_element as this only works for ForwardIterator, but you can write your own algorithm for InputIterator easily enough. I've posted an implementation of an algorithm for finding the minimum & maximum of an input iterator range (and hence stream input). You can use it like:

#include <fstream>
#include <iostream>
bool is_empty(std::istream& in)
{
 return in.peek() == std::istream::traits_type::eof();
}
int main()
{
 std::ifstream file {"input.txt"};
 if (!is_empty(file)) {
 const auto minmax = minmax_stream<int>(file);
 std::cout << "min is " << minmax.first << std::endl;
 std::cout << "max is " << minmax.second << std::endl;
 } else {
 std::cout << "file is empty" << std::endl;
 }
}