3

New to C programming and in the attempts to make a void function that swaps the values of two variables. When I wish to swap the values of two integer variables the following functions works just fine for lets say a = 11, b = 33 with function call swap(&a, &b):

void swap(int *a, int *b) {
  *a += *b;
  *b = *a - *b;
  *a -= *b; 
}

But when I try to do this with two elements of an array it does not work correctly for swap(&a[0], &a[2]) for example. It does however work with the following function:

void swap(int i, int j, int a[]) {
  int h = a[i];
  a[i] = a[j];
  a[j] = h;
}

Does anyone know why the first works of single variables but not with array elements? Sure there is a very good explanation that I am missing here. All help is welcome, thanks in advance!!

Here is the complete program:

#include <stdio.h>
#include <stdlib.h>


void swap(int *a, int *b) {
  *a += *b;
  *b = *a - *b;
  *a -= *b; 
}

void selectionSort(int a[], int len) {
  int i, j, min;
  for (i = 0; i < len; i++) {
    min = i;
    for (j = i+1; j < len; j++) {
      if (a[j] < a[min]) {
        min = j;
      }
    }
    swap(&a[i], &a[min]);
  }  
}

int main(int argc, char * argv[]) {
  int a[5] = {5, 4, 3, 2, 1};
  int len = 5, i;

  selectionSort(a, len);


  for (i = 0; i < len; i++) {
    printf("%d ", a[i]);
  }
  printf("\n");

    return 0;
}

Output for the array values is 1 2 0 0.

Improve this question
4
  • 4
    "It does not work correctly" is a terrible description of the problem. Commented Oct 22, 2014 at 18:55
  • It could be a situational issue. If, for some reason, your calling code executes a swap of an element against itself ( swap(&a[1], &a[1]); ), this would go unchecked... My rudimentary understanding of the method says it would set the value to 0 unintentionally. Perhaps this is occurring? Commented Oct 22, 2014 at 18:57
  • 2
    Please note there's a better (fewer operations, no chance of undefined behavior due to overflow) method of swapping two variables without extra space: *a ^= *b; *b ^= *a; *a ^= *b; Still doesn't work when a == b. Commented Oct 22, 2014 at 19:01
  • I recommend the answer of @DavidSchwartz, below. To solve these kinds of problems on your own, adding printf's and/or using a debugger is recommended :-) Commented Oct 22, 2014 at 19:08

4 Answers 4

Reset to default
6

Let me guess -- all the values turn to zeroes?

Your swap function is broken if a == b. Try:

void swap(int *a, int *b) {
  if (a != b)
  {
      *a += *b;
      *b = *a - *b;
      *a -= *b; 
  }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Yes this one works! That is nice thank you very much. Why is it that if two elements are equal, then swapped and put to 0?
@jake1992: When *a == *b (two elements are equal) it works just fine. Your problem is when a == b, that is, both point to the SAME element.
Great thanks for the explanation, learned something new about pointers!
Fixing the swap function isn't enough. The fact that he's swapping an element with itself points to a bug in the sort routine.
1

It is always better to write the classic swap function

void swap( int *a, int *b )
{
   int tmp = *a;
   *a = *b;
   *b = tmp;;
}

Or change function selectionSort the following way

   void selectionSort(int a[], int len) {
      int i, j, min;
      for (i = 0; i < len; i++) {
        min = i;
        for (j = i+1; j < len; j++) {
          if (a[j] < a[min]) {
            min = j;
          }
        }
        if ( min != i ) swap(&a[i], &a[min]);
      }  
    }
Improve this answer

Comments

0

Your swap function fails when both arguments point to the same object. I found out that you were doing that by adding a printf call before the call to swap, something you could have tried yourself:

printf("swap(%d, %d)\n", i, min);
swap(&a[i], &a[min]);

The algorithm you used in your swap function:

*a += *b;
*b = *a - *b;
*a -= *b;

appears to be intended to avoid creating a temporary variable, but at the cost of (a) failing when both objects are the same, and (b) risking undefined behavior if there's an arithmetic overflow. Arithmetic on 2's-complement integers will usually behave with wraparound semantics, which (I think) will give you the correct results, but it's a needless complication. It has even more problems if you're dealing with floating-point, where arithmetic can overflow or lose precision. There's a similar bitwise xor hack that has most of the same problems (and that doesn't work at all with floating-point). Declaring a temporary will solve both problems:

const int old_a = *a;
*a = *b;
*b = old_a;

(It's common to name the temporary something like tmp, but I like to give it a name that tells you more explicitly what it's used for; adding const tells the reader that it's not going to be changed after you initialize it.)

But that's not the only problem. The fact that you're trying to swap array elements with themselves indicates that your selectionSort function is also buggy; a correct sorting algorithm will never swap an element with itself, because it's useless to do so.

If you fix selectionSort, you probably don't need to fix swap, but I recommend doing so anyway.

Improve this answer

Comments

-4

In case of array elements just use a[0] and a[2] and don't use the "&" operator as arrays are pointers on their own so you don't need to give any reference.

Improve this answer

1 Comment

The array decays to a pointer, but [] includes a deference.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.