1
\$\begingroup\$

I always wanted to ask this but couldn't for some reason.

I had written this chunk of code about 3 months ago when one of my teacher explained what selection sort was using flow chart. I had a basic understanding of what arrays were. So I sat down and wrote this:

#include <stdio.h>

#define MAX_NUMBER 10

void selection_sort(int [], int []);

void selection_sort(int num[], int sorted[])
{
    int i, high = 0, k, j = 9, store;

    for(k = 0; k < MAX_NUMBER; k++)
    {
      for(i = 0; i < MAX_NUMBER; i++)
      {
         if(num[i] > high)
         {
             high = num[i];
             store = i;
         }
      }

      sorted[j] = high; // Place the highest number in the end of array and decrement array
      j--;
      num[store] = 0; // Place 0 in the place of maximum number that was in the array
      high = 0; // again make high as zero, so as to compare zerpo again with all the numbers that are in the array
    }
}

int main()
{
    int num[MAX_NUMBER], sorted[MAX_NUMBER];
    int i;

    printf("Enter %d numbers: ", MAX_NUMBER);

    for(i = 0; i < MAX_NUMBER; i++)
    {
        scanf("%d", &num[i]);
    }

    selection_sort(num, sorted);

    for(i = 0; i < MAX_NUMBER; i++)
      printf("%d ", sorted[i]);

    return 0;
}

and I took almost 30 minutes to write this. But as you know the program in text books are a lot faster and smaller.

Can you please tell me what kind of programmer I can become? I have been programming for a good 6 months now.

This code review monster got into my head after reading Steve McConnell: Code Complete. Still I have a lot of pages to go through.

\$\endgroup\$
0
3
\$\begingroup\$
  • You don't need two arrays, it works with one too, simply swap the values. This needs less memory.

  • According to this:

    int i, high = 0, k, j = 9, store;
    

    j is always one less than the size of the array. Don't use magic numbers, use the constant.

  • If you want to reuse this selection_sort it would be adviseable to add a parameter for the size, so you don't need the global constant.

  • In the inner loop you also iterate through the array parts which also have been sorted. (This may also be the reason you choose to use two arrays)

  • The result may be wrong if you'd only have negative values in the array (due high = 0)

Here my improved implementation:

void selection_sort(int *num, size_t size)
{
    size_t k, i, j, store; // for indexes
    int high, swap; // for values

    // no need to sort if the array is short enough
    if(size <= 1) return;

    high = INT_MIN; // set the highest number to the minium
    j = size - 1;

    for(k = 0; k < size; k++, j--)
    {
      for(i = 0; i <= j; i++)
      {
         if(num[i] >= high)
         {
             high = num[i];
             store = i;
         }
      }

      // swap the highest number with the number at the last position
      // in the array
      swap = num[j]; 
      num[j] = num[store]; 
      num[store] = swap;
      high = INT_MIN; // reset the highest number
    }
}

There is also a similar implementation on Wikipedia

\$\endgroup\$
0
\$\begingroup\$

Apart from the other answer , i would suggest

int i;
int high = 0;
int k;
int j = 9;
int store;

instead of single line declarations.

int i, high = 0, k, j = 9, store;

The former way , may take more space , but is convenient for adding a comment to each declaration and for improves readability for subsequent modifications.

Also this answer , makes a valid reason not to prefer single line declarations.

One more point is that , generally in most code i have seen , functions are declared before main() and defined after main().

So your program must look like,

void selection_sort(int [], int []);                //Declaration


int main()
{

retrun 0;
}

void selection_sort(int num[], int sorted[])        //Definition
{

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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