Is my sorting question in ascending order a good solution?

Viewed 113
#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? */

5 Answers

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.

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?

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

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

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.

Related