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.

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!

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.

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';
}