Finding the greatest number from three numbers

Question

The user is prompted to enter three numbers. The function finds the largest number and outputs it to the user.

What do you think of my overall program flow and style?

#include <iostream>
using namespace std;
int maxNumber(int num1, int num2, int num3); //function prototype/declaration
int main() { 
int a, b, c, final;
cout << "Enter first number";
cin >> a;
cout << "Enter sec number";
cin >> b;
cout << "Enter third number";
cin >> c;
final = maxNumber(a, b, c);
cout << "Largest: " << final;
cin.get();
return 0;
}
//function def
int maxNumber(int num1, int num2, int num3)
{
 int result;
if (num1 > num2 && num1 > num3) {
 result = num1;
}
else if (num2 > num1 && num2 > num3) {
 result = num2;
}
else if (num3 > num1 && num3 > num2) {
 result = num3;
}
return result;
}

• Justin: @Incomputable It's not scary. Just a base case and variadic templates. It's not really metaprogramming. Example: coliru.stacked-crooked.com/a/cd6137926330f8a4 .
• kfx: cout << std::max(a, std::max(b, c));
• Justin: @Incomputable Easy
• Justin: @Incomputable It's not a good idea in general to take the parameters by reference because it becomes incredibly easy to get dangling references. Notice that even std::max with an initializer list does not. Other things: "no recursion version" is not needed at all. With literally any level of optimization, the exact same assembly is generated. My point was that it's not too much work to write an n-ary max function as long as you kind of understand how templates work; you don't need tons of metaprogramming ability. Creating a perfect function is usually unneeded.
• Justin: @Incomputable Recursive version, iterative version

Answer 1

I see a number of things that could help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Fix the formatting

I don't know if the indentation problems are a cut-and-paste error or really how your code looks, but it should be fixed.

Avoid using C++ keywords as variable names

The keyword final is being used here as a variable name. While that's technically not an error, it's not good practice. I'd suggest using some other name for that.

Simplify your code

The maxNumber function is more complex than it needs to be. Here's an alternative that minimizes the number of comparisons made:

int maxNumber(int num1, int num2, int num3) {
 if (num1 > num2) {
 return num1 > num3 ? num1 : num3; 
 }
 return num2 > num3 ? num2 : num3; 
}

Omit return 0

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.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

• Robot_enthusiast: Thanks, that's great feedback, I'll try to implement that advice.
• user14393: I'm not sure I think the example of simplified code is actually simpler than the original!
• user14393: There's a difference between "It's bad to omit the return 0;" and "It's bad to insist that an existing return 0; be removed".
• Michael: @Hurkyl: I agree, I always get a headache when I encounter the ? operator. I think some programmers like it because it looks more compact (just like omitting the {} for single line statements does).
• Džuris: @Michael exactly, the braces should be excluded in this answer :)

Answer 2

The implementation of maxNumber() is broken. What happens when we try to evaluate maxNumber(0, 0, 0)?

• The first if condition is false.
• The second else-if condition is false.
• The third else-if condition is false.

Hence the variable result never gets assigned. An uninitialized variable is being returned at the end of the function. This is undefined behavior and can make demons fly out of the user's nose.

The best way to fix this is to weaken all your comparisons from > to >=.

• Incomputable: Great catch! I've read some of your articles. Its great to see you on the site.
• Nayuki: Thanks! I feel like I publish niche topics, so running into a reader is always a wonderful coincidence. Haha, one of my more recent articles is actually about undefined behavior
• Edward: I really hate it when demons fly out of my nose.

Answer 3

If you define a helper function max taking two arguments, the code becomes trivial:

int max(int a, int b) {
 return a > b ? a : b;
}
int maxNumber(int a, int b, int c) {
 int maxab = max(a, b);
 return max(maxab, c);
}

You don't even need to define the max function, since the C++ standard library provides one. Just write:

#include <algorithm>

• Robot_enthusiast: Ah okay thanks, that would make things easier haha. As a random question, what would you say are the "prerequisites" for understanding how to use STL? Like what topics should I be comfortable with before proceeding to that?
• Incomputable: @Robot_enthusiast, iterators, and generally library concepts
• Matthieu M.: Thanks for the sanity call, I was getting desperate here with all those if and ?: being sprinkled everywhere. The simplest definition of the max of a sequence of N elements is that it is the max of the first N-1 elements and the Nth element. It's very understandable and is optimal in the number of comparisons (N-1).
• jamesqf: @Matthieu M.: You honestly think that's a simple definition? Or that it would lead to efficient & maintainable code?
• Matthieu M.: @jamesqf: I do think it's a simple definition, mathematically. As for efficient & maintainable, it's a simple loop! Hard to be more efficient, and simple enough that it should be maintainable.

Answer 4

std::max has the following overload (See http://en.cppreference.com/w/cpp/algorithm/max):

template< class T >
T max( std::initializer_list<T> ilist );

You can take advantage of that function and implement maxNumber as:

int maxNumber(int a, int b, int c) {
 return std::max({a, b, c});
}

Answer 5

This advice is likely to be somewhat controversial, but

Explicitly flush when output should be available

cout << "Enter first number" << flush;
cin >> a;
cout << "Enter sec number" << flush;
cin >> b;
cout << "Enter third number" << flush;
cin >> c;

While some would advicate relying on the implicit flushes coming from cin being tied to cout, my own observations are that:

• Usually it doesn't matter if you flush or not
• The infrequent programs that flush too much are easily fixed
• The infrequent edge cases in buffering are extremely confusing, and it is nearly impossible for someone who isn't familiar with it to figure out what's happening

So, in my opinion, the better habit is to flush explicitly rather than relying on implicit behavior.

(the opposing opinion is that one should be much more afraid of the second bullet than of the risk of confusing/erroneous output in the edge cases)

This program, I'm pretty sure, is an example of the first case, where it doesn't actually matter.

• Edward: There is special relationship between std::cin and std::cout due to the fact that by default, the two streams are tied together. For that reason, an explicit flush is not only not necessary but counterproductive.
• user14393: @Edward: Except, of course, in the edge cases where the implementation is allowed to eliminate the flush entirely. And if nothing happens shortly thereafter to force a flush, the results will be thoroughly confusing to anyone who is absolutely convinced that the call must flush. (and also to those who don't know about buffering)

Answer 6

Another way of doing it using vector and sort. Note: I am also a begineer.

#include <iostream>
#include <vector>
#include <algorithm>
int main()
{
 using std::cout;
 using std::cin;
 int a{0};
 int b{0};
 int c{0};
 cout << "Enter first number: ";
 cin >> a;
 cout << "Enter second number: ";
 cin >> b;
 cout << "Enter third number: ";
 cin >> c;
 std::vector<int> numbers={a,b,c};
 std::sort(numbers.begin(),numbers.end());
 cout<<"Maximum number is: "<<numbers[numbers.size()-1];
}

• jamesqf: You might want to think about the overhead involved in setting up a vector, and in starting a sort for three numbers. Sure, it doesn't matter in a trivial program like this, but if you get in the habit, someday you will do something similar inside a loop nested several levels deep, each level iterating a few million times.

Answer 7

You could do the maxNumber function in 3 lines:

max = (a > b) ? a : b;
max = (c > max) ? c : max;
return (max);

Also, if you think about your original code a bit, you will see that you could have problems if two of the inputs are equal.

Also, maybe it's just me, but I'd prefer to be prompted to enter all three of the numbers at the same time.

PS: For those who don't find ternary expressions readily understandably, an equivalent woud be

max = a;
if (b > max) max = b;
if (c > max) max = c;
return (max);

I think this has the advantage that it's easily generalized to e.g. finding the max of an array, and readily translates into most languages. And it eliminates the logic bug in the OP's code, which I think is fairly important :-)

• Michael: But good code is not about writing the least amount of lines, it’s about being understandable.
• Džuris: If you'd want to condense, you could delete the last line and replace max = in second line with a return. That wouldn't even improve unreadability.
• Bogdan Alexandru: This is not about shortest number of lines, but if it were, you're suggestion would be far from shortest: return (a >= b && a >= c) ? a : ((b >= a && b >= c) ? b : c);
• jamesqf: @Michael: Yes, so I think those three lines are about as understandable as you can get. Personally, I think shortening it further by putting an expression in the return statement makes for less readable code. And making it all one expression inside the return is really confusing.