Having trouble sorting an Array that has a zero in it.

Hey guys, I was just wondering if someone could take a look at my code real quick. I'm sorting an array, and when it comes to a zero it obviously prints zero's from that point on. I'm using a simple selection sort, and it works fine as long as there are no zero's in the array. Could suggest a fix to this problem???

insert

Code:

void sortArray (float nums[], int last)

{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
        {
            smallest = current;

            for (int walk = current + 1; walk <= current; walk++)
                
                if (nums[walk] < nums[smallest] && nums[smallest] != 0)
                    smallest = walk;
                
                else if (nums[smallest] == 0)
                    smallest = walk++; //Here's where I think my problem is...

                    temp = nums[current];
                    nums[current] = nums[smallest];
                    nums[smallest] = temp;
        }
return;
}

I want it to just print the 0 and resume sorting, but I've been looking at it for hours and can't seem to think of something to make that happen.

Thanks so much for any help...

I prefer to use braces even when they are optional, so this is how I would style your function:

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;

        for (int walk = current + 1; walk <= current; walk++)
        {
            if (nums[walk] < nums[smallest] && nums[smallest] != 0)
            {
                smallest = walk;
            }
            else if (nums[smallest] == 0)
            {
                smallest = walk++;
            }
        }

        temp = nums[current];
        nums[current] = nums[smallest];
        nums[smallest] = temp;
    }
}

Now, observe this line:

Code:

for (int walk = current + 1; walk <= current; walk++)

walk is set to current + 1, hence walk > current, thus walk <= current is always false, so the inner loop's body is never entered. Therefore, we can simplify your code to:

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;
        temp = nums[current];
        nums[current] = nums[smallest];
        nums[smallest] = temp;
    }
}

Since smallest == current, the swap is a self-swap, which has no net effect. The loop does no other work, and the function has no other code. Therefore, we can simplify your function to:

Code:

void sortArray(float nums[], int last) {}

Is this really the code that you compiled and tested?

I don't understand the special treatment for zero. The simple nums[ walk ] < nums[ smallest ] should be the only comparison. Also, it's only after you find the smallest that you swap. After that, one element has been sorted, so you continue from the second leftmost element, and so on, until all the elements have been selected.

Originally Posted by laserlight

I prefer to use braces even when they are optional, so this is how I would style your function:

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;

        for (int walk = current + 1; walk <= current; walk++)
        {
            if (nums[walk] < nums[smallest] && nums[smallest] != 0)
            {
                smallest = walk;
            }
            else if (nums[smallest] == 0)
            {
                smallest = walk++;
            }
        }

        temp = nums[current];
        nums[current] = nums[smallest];
        nums[smallest] = temp;
    }
}

Now, observe this line:

Code:

for (int walk = current + 1; walk <= current; walk++)

walk is set to current + 1, hence walk > current, thus walk <= current is always false, so the inner loop's body is never entered. Therefore, we can simplify your code to:

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;
        temp = nums[current];
        nums[current] = nums[smallest];
        nums[smallest] = temp;
    }
}

Since smallest == current, the swap is a self-swap, which has no net effect. The loop does no other work, and the function has no other code. Therefore, we can simplify your function to:

Code:

void sortArray(float nums[], int last) {}

Is this really the code that you compiled and tested?

Yes...it now prints the array, but it doesn't sort from lowest to highest. Ran it with another file with no zero's and it sorted it just fine. I've been at this for hours, lol. I just can't get it to sort the negative numbers, then the zero, and then follow with the positive numbers...driving me crazy!!![/QUOTE]

Originally Posted by Cgmgator

Yes...it now prints the array, but it doesn't sort from lowest to highest. Ran it with another file with no zero's and it sorted it just fine. I've been at this for hours, lol. I just can't get it to sort the negative numbers, then the zero, and then follow with the positive numbers...driving me crazy!!!

[/QUOTE]

You're right, I actually had

[CODE:]

for (int walk = current + 1; walk <= last; walk++)

[/CODE]

Before I tried to do a bubble sort...my eyes are starting to hurt, but it actually wasn't working before that.

Laser gave you a great starting point for how to evaluate your code and then whiteflags basically spelled out the selection sort algorithm for you.

The selection sort algorithm is:

1. Find the minimum element in the list

2. Swap it with the element in the first position of the list

3. Repeat the steps above for all remainder elements of the list starting at the second position.

Now compare that with your implementation:

Code:

for (int current = 0; current < last; current++)
        {
            smallest = current;

            for (int walk = current + 1; walk <= current; walk++)
                
                if (nums[walk] < nums[smallest] && nums[smallest] != 0)
                    smallest = walk;
                
                else if (nums[smallest] == 0)
                    smallest = walk++; //Here's where I think my problem is...

                    temp = nums[current];
                    nums[current] = nums[smallest];
                    nums[smallest] = temp;
        }

What do you think is going on here?

I think when smallest gets to a value of 0, it stores the 0 in temp, then for some reason the 0 gets passed to the rest of the printed arrays...I added the if..else statement to try to find some way to deal with zero, but I am really missing something.

I think so to. Let's take a look at what whiteflags said:

I don't understand the special treatment for zero. The simple nums[ walk ] < nums[ smallest ] should be the only comparison. Also, it's only after you find the smallest that you swap. After that, one element has been sorted, so you continue from the second leftmost element, and so on, until all the elements have been selected.

When evaluating numbers the microprocessor is smart enough to understand -5 < -3 < 0 < 3...ect. You don't need any special cases there. Simply implement the algorithm listed above. When you first start your search simply set the smallest number to the first value in the array and begin the search.

I wonder if maybe my problem is the value that I passed for the last index in the array. I defined the SIZE = 90. I set a count to stop at 90. Would my value of last be the SIZE - count? Or maybe last = count - 1?

The problem is your implementation of your selection sort.

Lets say I made an array of 5 numbers: -3, -5, 0, 5, 2.

Now in order to sort my array using selection sort I would:

1. start with the first value of my array and make that my min. (so minus 3)
2. start looking at everyother value in the array excluding my starting point (so I would start with -5 or my starting point + 1)
3. Compare the two values, is my min > the next value?
3.a if Yes I would swap those values.
4. Continue comparing the rest of the list.
5. Start over at step 2 but make my new min be the next array value to look at.

Does this look right? I am so sorry to keep bothering you guys, but I won't be able to sleep until I get this right...

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;

        for (int walk = current + 1; walk <= last; walk++)
            if (nums[walk] < nums[smallest])
            smallest = walk;

        temp = nums[current];
        nums[current] = nums[smallest];
        nums[smallest] = temp;
    }

return;

You are starting to get there. Let's take a look at your code:

Code:

void sortArray(float nums[], int last)
{
    int smallest;
    int temp;

    for (int current = 0; current < last; current++)
    {
        smallest = current;

1. you actually only need to iterate the outer loop through last-1 since the inner loop will evaluate the last member
2. instead of storing the index store the actual value here since it will make the swap easier, aka smallest=nums[current]. Also note that your array is float here so I reccommend making smallest a float as well.

Code:

 for (int walk = current + 1; walk <= last; walk++){
            if (nums[walk] < nums[smallest]){
                 smallest = walk;
                 temp = nums[current];
                 nums[current] = nums[smallest];
                 nums[smallest] = temp;
            }
}

In red are where you need your brackets so the compiler knows where the dependent lines of code are for your loop.

In bold, walk < last; don't exceed the bounds of your array.

In blue let's take a look at your swapping:
1. make temp a float as well.
2. Save num[smallest] in your temp variable
3. Make num[walk] your new num[smallest]
4. place temp back in your array at num[walk]

Originally Posted by AndrewHunter

2. instead of storing the index store the actual value here since it will make the swap easier, aka smallest=nums[current]. Also note that your array is float here so I reccommend making smallest a float as well.

I think storing the index is fine. In fact, storing the actual value will make the swap harder because you need to know where the smallest element is so as to assign the first element's value to it.

Originally Posted by AndrewHunter

In blue let's take a look at your swapping:
1. make temp a float as well.
2. Save num[smallest] in your temp variable
3. Make num[walk] your new num[smallest]
4. place temp back in your array at num[walk]

Good catch with the type of temp. I am curious why you outlined a swap algorithm that mirrors Cgmgator's swap implementation. Even more interesting is why you added those braces where you did

Also, Cgmgator: what is last? The index of the last element?

Originally Posted by laserlight

I think storing the index is fine. In fact, storing the actual value will make the swap harder because you need to know where the smallest element is so as to assign the first element's value to it.

As always laser, thankyou for the insight. I guess I just never thought about it that way....

Originally Posted by laserlight

... I am curious why you outlined a swap algorithm that mirrors Cgmgator's swap implementation. Even more interesting is why you added those braces where you did
...

Huh, I guess I did. Silly me. As for the braces I didn't see the need to do the swap if none is required.

Judging by how you've named the second parameter, yes it is fine.
However you should note that with it like this you'll need to pass in size-1 as the second parameter. Standard library algorithms eliminate the need for the -1 and are minutely more efficient as a result.

Oh right yeah the type of 'temp' needs correcting. I noticed that earlier but forgot to say.