Sort three input values by order

Question

This is a program to sort three inputted integers in order, from least to greatest. I would like to have it reviewed.

#include <iostream>
void Sort(int &a, int &b, int &c){
 if(a>b){
 int tmp = a;
 a = b;
 b = tmp;
 }
 if(a>c){
 int tmp = a;
 a=c;
 c = tmp;
 }
 if(b>c){
 int tmp = b;
 b=c;
 c=tmp;
 }
 return;
}
int main(){
 std::cout << "Enter three integers: " << std::endl;
 int num1;
 int num2;
 int num3;
 std::cin >> num1 >> num2 >> num3;
 int output1 = num1;
 int output2 = num2;
 int output3 = num3;
 Sort(output1,output2,output3);
 std::cout << num1 << " " << num2 << " " << num3 << " "
 << " in sorted order: ";
 std::cout << output1 << " " << output2 << " " << output3 << std::endl;
 return 0;
}

Answer 1

Things you should change:

1. Sort() is explicitly returning at the end (return;). The return statement is implicit once you reach the end of a void function, so should not appear.

2. Pay attention to the spacing between the = sign. In some places you have b=c; while in others you have a = b; Be consistent with the spacing. I suggest the latter, as it seems more readable to me.

3. You don't have to return 0 from main. For main(), the return zero is implicit if not added.

---

This is also a great opportunity for you to learn about C++ templates. I will suggest you a base implementation that you can study, test, and better understand the awesomeness of this language feature:

template<typename T>
void swap_if_greater(T& a, T& b)
{
 if (a > b)
 {
 T tmp(a);
 a = b;
 b = tmp;
 }
}
template<typename T>
void sort(T& a, T& b, T& c)
{
 swap_if_greater(a, b);
 swap_if_greater(a, c);
 swap_if_greater(b, c);
}

Just as in the example provided by @EngieOP, I've added a swap function to reuse more code. I've also better separated common code by moving the conditional inside the swap function (swap_if_greater()), since it is common to all inputs.

sort() is now a template function, meaning is can operate on any type, including int, float, double, char, ... It is a generic function, just as many of the functions in the standard C++ library. This concept promotes great code reuse and low redundancy of functionality.

• R Sahu: I would recommend using std::swap instead of implementing it in swap_if_greater.
• Pete Becker: Concerning return 0; from main, I've always thought that leaving it out looks wrong. That feature is there to make hello, world simple for beginners, but there's no harm in putting the return statement in, and no need to remove it if it's there.
• glampert: @PeteBecker, Adding the return 0 on main always seemed to me like adding the return at the end of a void function. Seems like boilerplate code to me. But I'll agree that opinions may vary on the subject.
• Pete Becker: @glampert - but main isn't a void function; it returns int, and the returned value is detectable by the caller. Leaving out the return is a convenience.

Answer 2

You can use std::swap to swap values, and it will shorten the Sort method by quite a bit:

void Sort(int &a, int &b, int &c){
 if (a > b) {
 std::swap(a, b);
 }
 if (a > c) {
 std::swap(a, c);
 }
 if (b > c) {
 std::swap(b, c);
 }
}

Tip: the program will be a little bit easier to test if you allow the input as command line arguments too, in addition to stdin:

int main(int argc, char ** argv) {
 int num1, num2, num3;
 if (argc >= 4) {
 num1 = std::stoi(argv[1]);
 num2 = std::stoi(argv[2]);
 num3 = std::stoi(argv[3]);
 } else {
 std::cout << "Enter three integers: " << std::endl;
 std::cin >> num1 >> num2 >> num3;
 }

Thanks to @glampert for recommending the modern std::stoi instead of atoi!

• glampert: I would recommend replacing atoi in your example with std::stoi, which is the new standard. It is very likely that the OP will have a C++ compiler by now.
• Laurent LA RIZZA: @glampert: well, you can tell by the OP's use of references in the question.
• glampert: @LaurentLARIZZA, that was a typo, I meant a C++11 compiler. Of course the code in question was C++ :)

Answer 3

Notice how the function Sort() is performing the same operation on three different pairs of variables? I suggest writing a separate function called Swap(). This shortens and simplifies the code.

Swap() can be something like this:

void Swap(int &x, int &y){
 int tmp = x;
 x = y;
 y = tmp;
 return;
}

Your Sort() function should look like this:

void Sort(int &a, int &b, int &c){
 if(a>b){
 Swap(a,b);
 }
 if(a>c){
 Swap(a,c);
 }
 if(b>c){
 Swap(b,c);
 }
 return;
}

Instead of declaring three more integers to hold the output, do the following. You're passing references to avoid copying anyway.

std::cout << num1 << " " << num2 << " " << num3 << " in sorted order: ";
Sort(num1,num2,num3);
std::cout << num1 << " " << num2 << " " << num3 << std::endl;

I'm sure other reviews may show examples of using C++11 to simplify things.

• user54356: I thought it was simple enough that I didn't have to write a separate function.
• Quaxton Hale: Okay. For a program this simple, you can do without writing a separate function. But do you really want to do all that extra typing? Plus, it looks cleaner don't you think?
• Jamal: You could just use templated std::swap, which accepts any type, not just ints.
• Quaxton Hale: Yes. :) But since we're including algorithm, might as well just use std::sort. To what extent do we hold back for a beginner tagged question?
• Jamal: @EngieOP: Sure, a beginner can learn how swap works, but using the STL version results in less code and less to maintain.

Answer 4

First off this is pretty good for a beginner. These are some notes that mostly say use the build in standard library which you are not familiar with yet.

• I dislike the variable declarations

  int num1;
  int num2;
  int num3;
  

  Those 3 numbers belong together: std::array<int, 3> numbers;.
  An alternative would be int numbers[3];.
  std::array has some advantages such as having a size method and it can be passed as a parameter. Build in arrays will decay to pointers instead.

• You do not need to invent your own sorting algorithm, use std::sort.
• You do not check for read errors. If std::cin fails to read the numbers your program will output garbage.

Complete program:

#include <array>
#include <algorithm>
#include <iostream>
int main(){
 std::cout << "Enter three integers:\n";
 std::array<int, 3> numbers;
 for (auto &n : numbers){
 if (!(std::cin >> n)){
 std::cout << "Failed reading number\n";
 return -1;
 }
 }
 for (auto &n : numbers){
 std::cout << n << ' ';
 }
 std::cout << " in sorted order: ";
 std::sort(std::begin(numbers), std::end(numbers));
 for (auto &n : numbers){
 std::cout << n << ' ';
 }
 std::cout << '\n';
}