Selection sort review. Does it look good?

Question

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.

Answer 1

• 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

Answer 2

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