HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Reverse array in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arrayreversestackoverflow

Problem

I wrote a function to reverse an array. The first argument is an iterable object, the second argument is the size of the array element in bytes, and the third argument is the length of the array.

void reverse(void *object, int size, int length) {
    int i;
    int j; 
    int k;

    for(i=0, j=length-1; i < j; i++, j--) {
        for(k=0; k<size; k++){
            *((char*)object+(i * size)+k) = *((char*)object+(i * size)+k) ^ *((char*)object+(j * size)+k);
            *((char*)object+(j * size)+k) = *((char*)object+(i * size)+k) ^ *((char*)object+(j * size)+k);
            *((char*)object+(i * size)+k) = *((char*)object+(j * size)+k) ^ *((char*)object+(i * size)+k);
        }
    }
}


Could this implementation cause errors? How can I improve it?

Solution

Your code works – as far as I can see – correctly. But I would rewrite
it a bit to improve the readability (and with it the maintainability) of the
code.

The function name reverse() is quite general. That could refer
to reversing an array, string, or whatever. A better name might be
reverseArray().

The repeated cast (char*)object can be avoided by defining
a variable

char *ptr = objects;


once. Next, the address calculations can be simplified by separating
the swapping of two objects into a helper function:

void reverseArray(void *objects, int size, int length) {
    char *ptr = objects;
    for (int i = 0, j = length - 1; i < j; i++, j--) {
        swapObjects(ptr + size * i, ptr + size * j, size);
    }
}


Note also how i and j are declared locally to the for-loop.

Using your method, swapObjects() would be:

static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++) {
        o1[k] = o1[k] ^ o2[k];
        o2[k] = o1[k] ^ o2[k];
        o1[k] = o1[k] ^ o2[k];
    }
}


However, I can see no advantage in this "magically swap two bytes without
temporary storage" and would rewrite it as

static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++) {
        char tmp = o1[k];
        o1[k] = o2[k];
        o2[k] = tmp;
    }
}


which is much simpler (no XOR operations and less memory reads and
writes). It is possible to make the swap operation faster by using
int or long as temporary storage, but this requires a careful
check of the sizes and memory alignments.

Here you can get rid of the subcripts by increasing the
passed pointers

static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++, o1++, o2++) {
        char tmp = *o1;
        *o1 = *o2;
        *o2 = tmp;
    }
}


but that is a matter of taste. I don't think that it makes a difference
if you compile the code with optimizations switched on.

In addition, I would use size_t as type for the size and length
parameters. size_t is what sizeof returns, and is the correct type to describe the size
of an object in memory, see for example the declarations
of malloc(), calloc() or strlen().

Code Snippets

char *ptr = objects;
void reverseArray(void *objects, int size, int length) {
    char *ptr = objects;
    for (int i = 0, j = length - 1; i < j; i++, j--) {
        swapObjects(ptr + size * i, ptr + size * j, size);
    }
}
static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++) {
        o1[k] = o1[k] ^ o2[k];
        o2[k] = o1[k] ^ o2[k];
        o1[k] = o1[k] ^ o2[k];
    }
}
static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++) {
        char tmp = o1[k];
        o1[k] = o2[k];
        o2[k] = tmp;
    }
}
static inline void swapObjects(char *o1, char *o2, int size) {
    for (int k = 0; k < size; k++, o1++, o2++) {
        char tmp = *o1;
        *o1 = *o2;
        *o2 = tmp;
    }
}

Context

StackExchange Code Review Q#115625, answer score: 5

Revisions (0)

No revisions yet.