Why doesn't this reverse function work correctly?

Question

return_code swap_int_buffer(int* buffer, int size, int index1, int index2) {        
    if(index1 <0 || index2<0){
        return INVALID_ARGUMENT_NEGATIVE;
    } else if(index1 > size || index2 >size){
        return INVALID_INDEX_OUT_OF_BOUNDS;
    }else{
        int *bucket;
        int i;
        int j;

        bucket = (int*)malloc(sizeof(int)*size);
        for(i = size - 1, j = 0; i>=0; i--, j++){
            *(bucket+j) = *(buffer+i);
    }       
    for(i=0; i<size; i++){
        *(buffer+i) = *(bucket+i);      
    }       
    return SUCCESS;     
}

return NOT_IMPLEMENTED;
}

I dont understand why when I test this code it doesnt work correctly, the tests are provided by my university, the function is meant to reverse an int array. Am I missing something or? Thanks for any help :)

Run in a debugger and step through the code line by line. And [don't cast the result of `malloc` in C](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Then remember that e.g. `*(buffer + i)` is equivalent to `buffer[i]` (easier to read for many, and one character less to write :). And is there a reason you're not using e.g. [`memcpy`](http://en.cppreference.com/w/c/string/byte/memcpy) to copy the memory? — Some programmer dude, Mar 03 '15 at 14:19
can you explain what you expect the function should do, and what it does right now? — wimh, Mar 03 '15 at 14:19
@JoachimPileborg im very new to C so I dont know many library functions, what does memcpy do? — , Mar 03 '15 at 14:23
@Callum Sangray It seems your function does not make any sense.:) — Vlad from Moscow, Mar 03 '15 at 14:31
yes vlad, and I've removed bucket as a pointer and changed it to an array, it still fails tests that involve actually reversing the array — , Mar 03 '15 at 14:32
@Callum Sangray The function has a memory leak. I think you have to reverse the array in place without allocating additional memory. Also there is no any sense to pass the size of the array if the function accepts starting and ending indices. — Vlad from Moscow, Mar 03 '15 at 14:34
@VladfromMoscow I have removed allocating memory and changed bucket to an array, but it doesnt reverse the array correctly as I'm failing the tests — , Mar 03 '15 at 14:39

Vlad from Moscow · Answer 1 · 2015-03-03T14:59:31.907

I do not see any greate sense in your function declaration (and definition).

I would write the function the following way

void reverse( int *a, size_t n )
{
   for ( size_t i = 0; i < n / 2; i++ )
   {
      int x = a[i];
      a[i] = a[n-i-1];
      a[n-i-1] = x;
   }
}

If you have an array named a and want to reverse it starting from index i1 and ending with index i2 then you can call the function the following way

reverse( a + i1, i2 - i1 + 1 );

Such functions should be written as simple as posiible. All checks of arguments should be done in the caller of the function. If you want you can write a wrapper to the function with the signature you showed. Also take into account that again there is no sense to declare indices as having type int instead of size_t.

If the function has to be declared as you showed then it can be defined like this (without testing)

return_code swap_int_buffer( int *buffer, int size, int index1, int index2 ) 
{        
    if ( index1 < 0 || index2 < 0 )
    {
        return INVALID_ARGUMENT_NEGATIVE;
    } 
    else if( index1 >= size || index2 >= size )
    {
        return INVALID_INDEX_OUT_OF_BOUNDS;
    }
    else if ( index2 < index1 )
    {
        return NOT_IMPLEMENTED;
    }
    else
    {
        int n = index2 - index1 + 1;
        buffer += index1;

        for ( index1 = 0; index1 < n / 2; index1++ )
        {
            int x = buffer[index1];
            buffer[index1] = buffer[n-index1-1];
            buffer[n-index1-1] = x;
        }

        return SUCCESS;
    }     
}

I cant change the parameters of the function, it must be coded the way I've tried — , Mar 03 '15 at 14:47
@Callum Sangray In what case should the function return NOT_IMPLEMENTED? — Vlad from Moscow, Mar 03 '15 at 14:50
its in there as a fail safe, it doesnt have to be there as it probably will do nothing ever, but its in the err_code enum the uni provided — , Mar 03 '15 at 14:52

Why doesn't this reverse function work correctly?

1 Answers1