# Selection sort review. Does it look good?

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.

• 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

Apart from the other answer , i would suggest

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


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
{

}