Arrange three integers from least to greatest

I'm a C++ beginner and I was told to make a program to arrange three integers from least to greatest. How did I do?

#include <iostream>

using namespace std;

int main()
{
int x = 0;
int y = 0;
int z = 0;
int first = 0;
int second = 0;
int third = 0;
cout<<"Enter three integers to be arranged from least to greatest.\n";
cin>>x>>y>>z;

if(x <= y)
{
if(x <= z)
{
first = x;
if(y <= z)
{
second = y;
third = z;
}
else
{
third = y;
second = z;
}
}
else
{
second = x;
if(y <= z)
{
first = y;
third = z;
}
else
{
third = y;
first = z;
}
}
}
else
{
if(x <= z)
{
second = x;
if (y <= z)
{
first = y;
third = z;
}
}
else
{
third = x;
if ( y <= z)
{
first = y;
second = z;
}
else
{
second = y;
first = z;
}
}
}
cout<<first<<second<<third;
}

• I think you need to use some sorting algorithm like bubble sort. This code will not work with more than three variable. Oct 31, 2015 at 8:51
• @paritosh Declined. It's not a duplicate.
– Mast
Oct 31, 2015 at 12:59
• @paritosh On Code Review, it takes a lot to be a duplicate Oct 31, 2015 at 13:58

Input/output

You need to output a space between the numbers, otherwise they will look like one long number. It's also customary to finish the output with a newline.

You should verify that you actually received three inputs successfully.

Lint

using namespace std; is considered a bad habit.

Your code would be more readable if you put some spaces with your operators. For example, cin>>x>>y>>z; would look better as std::cin >> x >> y >> z;

Only initialize what you need to initialize. For example, int x, y, z; is enough — in fact it's safer, because the compiler can then warn you if you subsequently use one of those variables without assigning a value.

Strategy

You've basically manually written out all the possible comparisons that a sorting algorithm would do, the long way. I suggest the following short solution (which doesn't generalize beyond three numbers).

#include <algorithm>
#include <iostream>

int main()
{
int x, y, z;
std::cout << "Enter three integers to be arranged from least to greatest.\n";
if (!(std::cin >> x >> y >> z))
{
std::cerr << "Error in input.\n";
return 1;
}

int first = std::min({x, y, z});
int third = std::max({x, y, z});

// Toss x, y, z into a bag, and take out the min and max.
// What's left is the middle number.
int second = x ^ y ^ z ^ first ^ third;

std::cout << first << ' ' << second << ' ' << third << std::endl;
}


Note that std::min({x, y, z}) uses C++11. For older compilers, you can use std::min(x, std::min(y, z)).

• Good point about superfluous initialization, and interesting different way to sort... even if that's now the fifth answer, and with my vote the fifth at score 1 ;-) Oct 31, 2015 at 13:41
• Absolutely love the bag-and-remove idea. It would work no matter what the type of the variables is, correct? Oct 31, 2015 at 17:38
• @Praveen Yes, the technique works for any bunch of bits. Oct 31, 2015 at 17:48
1. Don't define a variable before you need it. Remember you need to keep them all in mind when reading the code.

2. Don't initialize a variable before you have data to put there, as that suppresses compiler-warnings.

3. Don't try processing inputs you didn't actually receive, so check the state of the stream before sorting (You only need to do it once after trying to read all inputs).

if(!std::cin) {
return EXIT_FAILURE;
}

4. For sorting such a small and fixed number of inputs, a sorting-network is best (unless swapping is expensive, in which case you would first generate the minimal number of swaps):

static void swap(int& a, int& b) {
int t = a;
a = b;
b = t;
}
static void sort2(int& a, int& b) {
if(a > b)
swap(a, b); // Normally use std::swap from <utility>
}

sort2(first, second);
sort2(first, third);
sort2(second, third);


Generally, you would use a library-routine for sorting a range: std::sort.

5. I suggest you print some separator between the numbers you output, probably a single space:

std::cout << a << ' ' << b << ' ' << c << '\n';


Anyway, congratulations on using good formatting, even if not my preferred style. Too many beginners don't get how much that eases understanding and writing code.

I've done something similar to this in a introduction to C++ class, so in case you need a very simple answer without using some of the more complex functions, I would have condensed it into something like this.

I find this to be a helpful way because it helped me to use complex relational operations (such as &&, ||, !=) and it helps simplify some of the conditional statements so they don't end up becoming nested enough times for them to become confusing.

I think using those compound relational operations at the end to determine the smallest of the integers is helpful because it eliminates some of the comparisons in the original code, i.e. if x is not the smallest AND x is not the middle therefore x is the smallest.

#include <iostream>

using namespace std;

main () {

int x, y, z;
int largest, smallest, middle;

cout << "Enter three integers separated by spaces: ";
cin >> x >> y >> z;

if (x > y) {
if (x > z)
largest = x;
else
if (x < z)
middle = x;
}

if (y > x) {
if (y > z)
largest = y;
else
if (y < z)
middle = y;
}

if (z > y) {
if (z > x)
largest = z;
else
if (z < x)
middle = z;
}

if (y != largest && y != middle)
smallest = y;
if (x != largest && x != middle)
smallest = x;
if (z != largest && z != middle)
smallest = z;

cout << "Smallest = " << smallest << endl;
cout << "Middle = " << middle << endl;
cout << "Largest = " << largest << endl;

}


Excess code

You have some excess code in places where certain if statements are guaranteed to be true. The first is when z < x < y, you reach this code:

    else
{
second = x;
if(y <= z)
{
first = y;
third = z;
}
else
{
third = y;
first = z;
}
}


This could be simplified to:

    else
{
first = z;
second = x;
third = y;
}


because the two if statements leading up to this block already guarantee this ordering.

Later on there is something similar where this code:

    if(x <= z)
{
second = x;
if (y <= z)
{
first = y;
third = z;
}
}


could be simplified to:

    if(x <= z)
{
first = y;
second = x;
third = z;
}


Since you are a beginner, I will only briefly mention that the code could be smaller and cleaner if you used more advanced things like arrays, loops, and functions.

In addition to what others have already said, I would like to point out that your program currently does not do any error handling. (Try entering silly things that are not numbers to see what I mean.)

An easy way to check whether input succeeded is to check the state of the stream after the operation. This does not catch all errors (For example, if the user enters four numbers, the program will happily use the first three and drop the last on the floor) but at least you can be sure that you have at least three valid integers to begin with.

#include <cstdlib>
#include <iostream>

int
main()
{
int a = 0;
int b = 0;
int c = 0;
std::cout << "Please enter three integers: ";
std::cin >> a >> b >> c;
if (!std::cin)
{
std::cerr << "error: not three valid integers\n";
return EXIT_FAILURE;
}
std::cout << "a = " << a << ", b = " << b << ", c = " << c << "\n";
return EXIT_SUCCESS;  // optional
}


On error, I'm returning the constant EXIT_FAILURE that reports unsuccessful termination to the operating system. You have to #include the <cstdlib> header for this constant to be available. On success, you can return EXIT_SUCCESS (or just 0) from main but this is what happens implicitly anyway so there is no real need to do it.

I know that many programming lectures want you to write programs that do interactive user input to no end and if your requirements are so, then that's what you have to comply with but anyway, I want to mention that such programs suck in real-life use.

Maybe you have already learned that you can declare your main function in a second way that takes two parameters: the number of command-line arguments and an array of their values (as pointers to char arrays). (If not, don't worry about it.) Note that the first argument by convention is the file name of your executable and not a “real” argument so the actual arguments start at index 1.

You can then use the function std::stoi from the <string> header to convert the arguments into integers. The good thing about this function is that it will report errors immediately. (It does this by throwing an exception of type std::invalid_argument but I believe you won't have learned about this yet. If you did, you can catch the exception and print a more helpful error message but if you don't, the default is that the run-time environment will immediately bring down your program and eventually print some error message which is good enough for a start.)

#include <cstdlib>
#include <iostream>
#include <string>

int
main(int argc, char * * argv)
{
if (argc != 4)
{
std::cerr << "error: wrong number of arguments\n"
<< "usage: print3int NUMBER NUMBER NUMBER\n";
return EXIT_FAILURE;
}
const int a = std::stoi(argv[1]);
const int b = std::stoi(argv[2]);
const int c = std::stoi(argv[3]);
std::cout << "a = " << a << ", b = " << b << ", c = " << c << "\n";
return EXIT_SUCCESS;
}


I find this second version not only more useful than the interactive program but also the source code easier to understand. (Provided you have already learned about arrays, that is. Otherwise, the argv[1] might look strange.)

As other answers nicely clarify other aspects, I will only focus on the sorting part. When you feel ready for it, have a look at the data structures provided by the language libraries. In particular, std::multiset is appropriate here (as it is possible for two or more values to be equal), and it does the dirty work for you.

Example:

#include <iostream>
#include <set>

int main()
{
int x = 0;
int y = 0;
int z = 0;
std::cout << "Enter three integers to be arranged from least to greatest.\n";
std::cin >> x >> y >> z;

std::multiset<int> s = { x, y, z };

for (int elem : s)
std::cout << elem << " ";
}


Namespace

using namespace std; is considered bad practice. Short code is not a requirement in C++; clear code is preferred.

Since you are swapping two variables multiple times in the code, it would be nice if you made a function call:

swap(int& a, int& b)


Extra variables

No need for those first, second, third. It can be done by trick of chain of XOR the target variables twice a ^= b ^= a ^= b; to swap them.

Here's the finished code:

#include <iostream>
#include <utility>

int main()
{
int x = 0;
int y = 0;
int z = 0;

std::cout << "Enter three integers to be arranged from least to greatest.\n";
std::cin >> x >> y >> z;

if (x > y)  std::swap(x, y);

if (y > z)  std::swap(y, z);

if (x > y)  std::swap(x, y);

std::cout << x << ' ' << y << ' ' << z << '\n';
}


My original cryptic XOR swap function, before @5gon12eder suggested using std::swap:

void swap(int& a, int& b)
{
a ^= b;
b ^= a;
a ^= b;
}

• Why are you suggesting that cryptic XOR swap instead of the ready-made std::swap or – if that is not an option for this particular homework – the good old use of a temporary variable? I think using XOR adds more confusion here and the performance gains on modern compilers and hardware are questionable anyway. Oct 31, 2015 at 11:49
• @5gon12eder there is not a particular reason for using cryptic over std::swap expect that to tell OP what might the C++ can offer to us (by swapping without temp variable as std::swap do).. and regarding to performance gains, absolutely you are right, it is indeed insignificant. Oct 31, 2015 at 12:18
• Your XOR swap has undefined behaviour. Oct 31, 2015 at 14:09
• @AntonGolov this what i though before but after i got answer regarding XOR swap. it seems legal in c++. here what compiler generate for XOR swap codereview.stackexchange.com/questions/76976/… Oct 31, 2015 at 15:42
• @MORTAL: See the answer codereview.stackexchange.com/a/77192/6313 ; what the compiler happens to generate this time doesn't matter, the code can do anything. Oct 31, 2015 at 16:15