Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question

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

share|improve this answer
    
I thought it was simple enough that I didn't have to write a separate function. –  Dave 9 hours ago
    
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? –  EngieOP 9 hours ago
3  
You could just use templated std::swap, which accepts any types, not just ints. –  Jamal 7 hours ago
1  
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? –  EngieOP 7 hours ago

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.

share|improve this answer
    
I would recommend using std::swap instead of implementing it in swap_if_greater. –  R Sahu 2 hours ago

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 = atoi(argv[1]);
        num2 = atoi(argv[2]);
        num3 = atoi(argv[3]);
    } else {
        std::cout << "Enter three integers: " << std::endl;
        std::cin >> num1 >> num2 >> num3;
    }
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.