The code has a few errors.
for (i = 1; i < (n - 1); i++)
The values array is zero based, whereas i starts at an index of 1, which is the second value in the array. If the largest number happened to be in the first position it will be skipped. i should start at zero.
{
min = i;
for (j = i + 1; j < n; j++)
{
These lines are correct. min stores the index of the minimum item. j iterates over all the "unsorted" items.
if (values[j] < values[min])
min = j;
These lines are also correct. This checks if a value in the unsorted portion of the list is less than the value at the end of the sorted portion of the list.
if (min != i)
{
hold = values[j];
values[j] = values[i];
values[i] = hold;
}
This is the second place where an error occurs, and there are two errors here. The first error is that it swaps the value in the unsorted portion at i, with the value at j. This should actually swap the value of i with the value of min. Think of i as the end of the sorted list. You want to take the smallest value you find in the unsorted list (indicated by j), and swap it with whatever is at the end of the sorted list.
That was the first problem with that block. The second problem is when the swap occurs. The swap occurs whenever the position of the min value does not match i, or whenever the position of the smallest value is not the last value in the list. This occurs on every iteration of the j loop, which is not necessary. The j loop only needs to locate the min value. The actual swap can happen after the j loop.
Finally, another potential problem exists, in that the code assumes the list is at least two elements in size. As far as I recall, check50 never tested with less than three values, so you may never actually encounter this error. However, for correctness, you may want to add a condition which only performs the sort if n is two or more.
Putting this all together, the code might look like this:
// behaviour is undefined if n < 2, will segfault, or return nonsense result
if (n < 2)
{
return;
}
int min, hold, i, j;
// loop through all but the last element in the array
// start at the first value, which is index 0
for (i = 0; i < (n - 1); i++)
{
min = i;
for (j = i + 1; j < n; j++)
{
// find and store the position of the smallest value
// if there is no value smaller than the last value in the unsorted list,
// then min == i at the end of the loop
if (values[j] < values[min])
{
min = j;
}
}
// swap the min value with the value at the end of the sorted list
if (min != i)
{
hold = values[i];
values[i] = values[min];
values[min] = hold;
}
}
References:
