views:

417

answers:

5

Hi,

I've written a small program for my programming class, that uses pointers to reverse a char array. I was wondering if there is anything that I should do differently? Am I doing this correctly? Is there a more efficient way to accomplish this? Thanks!

My small program:

int main ( )
{
char buffer[80];

PrintHeader();

cout << "\nString reversal program";
cout << "\nType in a short string of words.";
cout << "\nI will reverse them.";
cout << "\n:";
cin.getline(buffer, 79);

cout << "\nYou typed " << buffer;
reverse (buffer);
cout << "\nReversed: " << buffer;

cout << endl;
system("PAUSE");
return 0;
}


void reverse(char* string)
{
char* pStart, *pEnd;
int length;
char temp;

length = strlen(string);

pStart = string;
pEnd = &string[length - 1];

while(pStart < pEnd)
{
    temp = *pStart;
    *pStart = *pEnd;
    *pEnd = temp;
    pStart++;
    pEnd--;
}
}
+1  A: 

You could use std::swap(*pStart, *pEnd) instead of open-coding swap.

Heck, you could just use std::reverse(buffer, buffer + strlen(buffer)). But I suppose that wouldn't really be using pointers yourself, and given that requirement, it looks fine.

Well, actually, a tiny nit: if length==0, then &string[length - 1] isn't pointing into the character array and is theoretically not a valid pointer.

ephemient
+6  A: 
void str_reverse( char *str ) {
    char *str_end = strchr( str, 0 );
    std::reverse( str, str_end );
}

if you're supposed to write a loop,

void str_reverse( char *str ) {
    for ( char *str_end = strchr( str, 0 ) - 1;
          str < str_end;
          ++ str, -- str_end ) {
        std::swap( *str, *str_end );
    }
}

or, of course, if you can use a C++ string,

void str_reverse( std::string &str ) {
    std::reverse( str.begin(), str.end() );
}
Potatoswatter
A: 

Nothing really wrong with yours I would stay away from using well know type names as variables like string for example as it makes it confusing for others to read. Just for one more way you can do it.

void RevBuff(char* Buffer)
{

int length = strlen(Buffer);
char * CpBuff = _strdup(Buffer);
for(int i = length -1, x = 0; i >=0 ; i--, x++)
{
    Buffer[x] = CpBuff[i];
}
free(CpBuff);
}

However like stated above you almost always want to use a library function over your own code if you can find one(you have no idea how many times I've seen professional programmers writing code that exists in a standard library when it could have easily been discovered with a Google search but I digress.

rerun
A: 

Assuming you can't use anything but C string functions, I would

  • avoid pre-declaring variables at the beginning of the function. This is a requirement of C (with the 1990 standard), but in C++ it is more idiomatic to declare and initialize variables where you use them.

  • avoid going out of bounds (decrementing beyond start of string) if the string is empty.

So something like:

void reverse(char* string)
{
    char* first = string;
    char* last = string + strlen(string);

    while(first < last)
    {
        --last; //avoids decrementing last beyond start of string if string is empty
        char temp = *first;
        *first = *last;
        *last = temp;
        ++first;
    }
}
visitor
A: 

Your code is very verbose and enumerates every operation, like picking the length of the string or assigning pointer to the end of the string or using a helper variable for exchanging values.

There are many ways to make it more efficient (as other answers prove), but arguably more correctly. Your solution puts code clarity ahead of performance and that is a praise worthy habit.

Of course the code could be written in half as many instructions, but optimizing compiler will do pretty much the same work off your code (one could shed a couple of cycles by some clever coding), but your is simply more readable.

Of course if you really want some extreme performance boost (but on much longer strings, megabytes of data maybe), this is a perfect job for the GPU. It would use 50 times as much time to set up the operation, then a tiny fraction of current CPU time to perform it.

SF.
I don't think the count of instructions and helper variables maps directly to execution time (an optimizing compiler can most probably remove them and rearrange things).
visitor
@visitor: yes. An optimizing compiler can get confused with overly verbose code at times and fail to guess what the author meant -> fail to optimize it. It's not a rule though, and overly terse code can confuse the optimizer just the same. Only with a lot of skill and knowledge you can write code more optimal than the optimizer could. So no sense optimizing this code by hand here because chances are you'll end worse off than by leaving it clear, obvious and simple.
SF.