unable to sort an array, wrong output

Question

#include<bits/stdc++.h>
using namespace std;

 
int
main ()
{   
    int a[3] = { 3, 2, 1 }, i, j, t = 0, k = 0;
    int m;
  
    for (int i = 0; i < 2; i++)
    {
        m = a[i];
      
        for (int j = i++; j < 3; j++)   
        {
            if (m > a[j])
             {
                 m = a[j];
               k = j;
            }
      }
      t = a[i];
      a[i] = m;
      a[k] = t;
   }
        for (int i = 0; i < 3; i++)
        {
            cout << a[i];
        } 
    return 0;
}

Please format your code. It's very difficult to read in this state. — cigien, Jan 20 '21 at 08:24
Also read [Why should I not `#include `?](https://stackoverflow.com/Questions/31816095/Why-Should-I-Not-Include-Bits-Stdc-H.) and [Why is `using namespace std;` considered bad practice?](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) — Ted Lyngmo, Jan 20 '21 at 08:34
The `int j = i++` in the nested `for` loop is suspicious, since it changes `i` - which is used to control the outer loop. In any event, you've used the abomination `` which has the effect of including most (all?) standard C++ headers and therefore allows you to use almost anything from the standard C++ library. Why not simply use `sort(begin(a), end(a))` to sort `a`? Fair bet it will achieve the job more effectively than your code (even if you get it correct) will. — Peter, Jan 20 '21 at 08:35
`std::sort(a, a + 3);` If your teachers are trying to get you to reinvent the wheel as a less efficient wheel, get better teachers. — Jasper Kent, Jan 20 '21 at 08:37
Also, don't use magic numbers (like `2` and `3`). `std::size(a) - 1` and `std::size(a)` would be clearer. `std::array a = {3, 2, 1};` and then using `a.size() - 1` and `a.size()` would even better. — Ted Lyngmo, Jan 20 '21 at 08:44
@jasperkent what if the teacher is not teaching programming language, but sorting algorithms? — JHBonarius, Jan 20 '21 at 08:49

score 1 · Answer 1 · edited Jan 20 '21 at 09:40

You seem to want to implement an insertion sort algorithm by hand. For this task, you should consider using std::sort. I refactored your code such that it basically does the same thing as you wanted and included some tips to make the code more readable and easier to debug for you and others:

#include <algorithm> //only include necessary headers
#include <iostream>
//do not use "using namespace std"

int main ()
{   
    int a[3] = { 3, 2, 1 }; 
/* 
Declare local variables at the first point they are used an as local as possible. 
It is much easier to read, if a no longer necessary variables leaves scope.
*/
  
    for (int i = 0; i < 2; i++) { 
        int min = a[i]; //use names that tell something about what you are doing
        int bestIndex = i;
        for (int j = i+1; j<3; j++) { //here was your major bug: i++ also increcments i
            if (a[j] < min){
               min = a[j];
               bestIndex = j;
            }
        }
        std::swap(a[i], a[bestIndex]); 
//try to use standard algorithms as often as possible. They document your code, are optimized and easier to read.
    }
    for (int i = 0; i < 3; i++){
        std::cout << a[i];
    } 
    return 0;
}

You got my vote but you could improve it by not using the magic numbers `2` and `3` anywhere. — Ted Lyngmo, Jan 20 '21 at 09:41
Yeah, that is correct, but then you basically need to introduce containers and iterators, right? — Henk, Jan 20 '21 at 09:49
The C++17 function `std::size()` is in ``. If C++17 isn't available: `template constexpr size_t arrsize(const T(&)[N]) { return N; }` should do. But sure, using a container like `std::array` and then using its `size()` member function would be preferable. Without using any of that, one could make `3` into a `constexpr size_t array_size = 3;` and use that `array_size` and `array_size - 1` everywhere for clarity. — Ted Lyngmo, Jan 20 '21 at 10:05

score 0 · Answer 2 · answered Jan 20 '21 at 08:37

Look at this:

for (int i = 0; i < 2; i++)
{      
    for (int j = i++; j < 3; j++)   
    {
    }
}

On the first iteration of the outer loop:

i is 0.
Then the inner loop increments it to 1.
Then the outer loop increments it to 2.

Then the outer loop is done, after just one iteration.

You want i+1 in the inner loop, not i++.

score 0 · Answer 3 · answered Jan 20 '21 at 09:24

#include<bits/stdc++.h>
using namespace std;



int main ()
{
    int a[3] = { 3,2,1},  t = 0, k = 0;
    int m=0;
    bool flag_change =false;

    for (int i = 0; i < 2; i++)
    {
        m = a[i];

        for (int j = i+1; j < 3; j++)
        {
            if (m < a[j])  //changing sing  (<>) sort by ascending/sort by descending order
            {
                m = a[j];
                k = j;
                flag_change = true;
            }
        }
        if(flag_change)
        {
            t = a[i];
            a[i] = m;
            a[k] = t;
            flag_change = false;
        }
    }
    for (int i = 0; i < 3; i++)
    {
        cout << a[i];
    }
    return 0;
}

I'm done correct you code. Code is working.But it is not good implementation.

unable to sort an array, wrong output

3 Answers3