Is my sorting question in ascending order a good solution?

Question

#include <iostream>
using namespace std;

int main() {
// Problem #2 Ascending Order
int num1, num2, num3, small, medium, large;
cin >> num1 >> num2 >> num3;

if(num1 <= num2 && num1 <= num3) {
    small = num1;
    if(num2 <= num3) {
        medium = num2;
        large = num3;
    } else {
        large = num2;
        medium = num3;
    }
} else if(num2 <= num1 && num2 <= num3) {
    small = num2;
    if(num3 <= num1) {
        medium = num3;
        large = num1;
    } else {
        large = num3;
        medium = num1;
    }
} else {
    small = num3;
    if(num2 <= num1) {
        medium = num2;
        large = num1;
    } else {
        large = num2;
        medium = num1;
    }
}

cout << small << " " << medium << " " << large;
return 0;

/* This code is to sort 3 numbers in ascending order, this is the solution I came up with. Im not sure if its a good answer, is their any better way to do this? Should I be happy with this solution? Any thoughts? Im new to programming and want to know if the solutions Im coming up with are realisitic? Like are they good enough for me to get a job if I problem solve like this? Does this show that Im a bad programmer? */

So, the code is working and you want a [code review](https://codereview.stackexchange.com/)? — Ted Lyngmo, May 19 '21 at 19:32
You won't get a job with code like this because it's such an inflexible solution. For improvements I'd suggest (1) be able to sort numbers in an array, so that you can sort *any* number of values (2) write tests that exercise the sort method, before implementing the sort method. — Den-Jason, May 19 '21 at 19:35
@Den-Jason you also wont get the job when you take a week to develop a generic sorting algorithm when asked to sort 3 numbers. — 463035818_is_not_a_number, May 19 '21 at 20:15

463035818_is_not_a_number · Answer 1 · 2021-05-19T20:22:33.170

No. Your code is too complicated. Seriously, it takes me some effort to read it and to understand every detail. It is very repetitive and has lots of potential for simple mistakes ruining correctness. Thats a too high price to pay to sort three numbers.

Don't reinvent a wheel and know your <algorithm>s.

You want the smallest number (std::min), the biggest (std::max) and the other number:

#include <algorithm>
#include <iostream>

int main(){
    int a = 42;
    int b = 2;
    int c = -13;

    int smallest = std::min({a,b,c});
    int biggest = std::max({a,b,c});
    // int middle = a+b+c-smallest-biggest;      // might overflow
    int middle = a ^ b ^ c ^ smallest ^ biggest; // no overflow

    std::cout << smallest << " " << middle << " " << biggest << "\n";
}

Live Demo

Credits for pointing out the overflow and providing the non-overflow solution goes to PatrickRoberts.

Even if, eg. as an exercise, you would avoid to use standard algorithms, you should still use functions. They need not be complicated. Already a int max(int a,int b) would help to simplify your code a lot.

I managed to think a little longer about the solution ;) You can use `int middle = a ^ b ^ c ^ smallest ^ biggest;` and avoid overflow entirely. — Patrick Roberts, May 19 '21 at 20:18

Thomas Matthews · Answer 2 · 2021-05-24T17:30:44.483

3

Why so many variables?

int small, medium, large;
std::cin >> small >> medium >> large;

// Force "small" to be the smallest.
if (small > medium) std::swap(small, medium);
if (small > large)  std::swap(small, large);

// Now to deal with "medium" and "large"
if (medium > large) std::swap(medium, large);

Did I miss something?

edited May 24 '21 at 17:30

answered May 19 '21 at 20:30

Thomas Matthews

52,985
12
85
144

1

Good eyes! Thanks! I reviewed it many times and stepped through it, and didn't catch the issue. – Thomas Matthews May 24 '21 at 17:31

score 1 · Answer 3 · answered May 19 '21 at 19:32

Your code is not good because

using namespace std; is used.
You wrote too much lines for sorting, which is very commonly used algorithm and a standard library exists for doing that.

Writing much code produces much chance to create bugs.

I'd prefer using std::sort for sorting in C++.

#include <iostream>
#include <algorithm>

int main(void) {
    constexpr int N = 3;
    int num[N];
    for (int i = 0; i < N; i++) {
        if (!(std::cin >> num[i])) {
            std::cerr << "read error" << std::endl;
            return 1;
        }
    }
    std::sort(num, num + N);
    for (int i = 0; i < N; i++) {
        if (i > 0) std::cout << ' ';
        std::cout << num[i];
    }
    return 0;
}

I agree that writing too many lines is a problem as it create many chances of introducing bugs. However, I would also hesitate on using `std::sort` to sort three items. Above shown many ways you can achieve it within 3~5 lines of code, which I think are more appropriate for this task. — Ranoiaetep, May 19 '21 at 21:29

Vlad from Moscow · Answer 4 · 2021-05-19T21:05:43.870

If you are not allowed to use arrays and to change the inputted values for the variables num1, num2, and num3 then I can suggest the following solution with only three if statements

#include <iostream>
#include <functional>

int main() 
{
    int num1, num2, num3;
    auto small = std::cref( num1 ), medium = std::cref( num2 ), large = std::cref( num3 );

    std::cin >> num1 >> num2 >> num3;
    
    if ( medium.get() < small ) std::swap( medium, small );
    if ( large.get() < medium ) std::swap( large, medium );
    if ( medium.get() < small ) std::swap( medium, small );
    
    std::cout << "small = " << small
              << ", medium = " << medium
              << ", large = " << large
              << '\n';
              
    return 0;
}

If to enter for example the following values

5 3 4

then the output will be

small = 3, medium = 4, large = 5

Another approach similar in some respects to your approach is the following

#include <iostream>
#include <functional>
#include <algorithm>

int main() 
{
    int num1, num2, num3;
    int small, medium, large;

    std::cin >> num1 >> num2 >> num3;
    

    if ( not ( num2 < num1 ) and not ( num3 < num1 ) ) 
    {
        small = num1;
        std::tie( medium, large ) = std::minmax( num2, num3 );
    }       
    else if ( not ( num3 < num2 ) ) 
    {
        small = num2;
        std::tie( medium, large ) = std::minmax( num1, num3 );
    } 
    else 
    {
        small = num3;
        std::tie( medium, large ) = std::minmax( num1, num2 );
    }

    std::cout << "small = " << small
              << ", medium = " << medium
              << ", large = " << large
              << '\n';
              
    return 0;
}

Again if to enter the same sequence of values as shown above

5 3 4

then the output will be

small = 3, medium = 4, large = 5

As fot your original solution then you could reduce the number of sub-expressions in conditions of the if statements. For example

#include <iostream>

int main() 
{
    int num1, num2, num3;
    int small, medium, large;

    std::cin >> num1 >> num2 >> num3;
    

    if ( not ( num2 < num1 ) and not ( num3 < num1 ) ) 
    {
        small = num1;
        if ( not ( num3 < num2 ) )
        {
            medium = num2;
            large = num3;
        }
        else
        {
            medium = num3;
            large = num2;
            
        }
    }       
    else if ( not ( num3 < num2 ) ) 
    {
        small = num2;
        if ( not ( num3 < num1 ) )
        {
            medium = num1;
            large = num3;
        }
        else
        {
            medium = num3;
            large = num1;
            
        }
    } 
    else 
    {
        small = num3;
        if ( not ( num2 < num1 ) )
        {
            medium = num1;
            large = num2;
        }
        else
        {
            medium = num2;
            large = num1;
            
        }
    }

    std::cout << "small = " << small
              << ", medium = " << medium
              << ", large = " << large
              << '\n';
              
    return 0;
}

score 0 · Answer 5 · answered May 19 '21 at 19:38

It's not a bad solution to the problem of sorting three numbers. It is reasonably easy to follow, it performs reasonably well. and it is reasonably easy to verify its correctness.

But you should be very surprised — in the real world or in academia — if the next thing you're asked to do isn't sort four numbers, or sort n numbers. Your approach is not applicable to any other value of n, and can't really be extended.

Is my sorting question in ascending order a good solution?

5 Answers5