(C++/WinAPI) Reversing LPSTR

Question

i have some problems to reverse LPSTR. Here is my function:

LPSTR Reverse(LPSTR a_lpText)
{
   int nTextLength = strlen((char*)a_lpText);
   LPSTR lpReversed = (LPSTR) GlobalAlloc(GPTR, nTextLength + 1);
   for (int i = 0; i < nTextLength; ++i)
      *(lpReversed + i) = (CHAR) *(a_lpText + nTextLength - i);
   return lpReversed;
}

Function, return not initialized LPSTR or some strange characters. Problem is probably in conversion? Thanks for answers!

Edit 1: strcat() don't work. I just want copy char by char.

Edit 2:

*(lpReversed + i) = (CHAR) *(a_lpText + nTextLength - i - 1);

Freeze the whole program.

Answer 1

This looks more like C code than C++, so I'll stick to that style. I can't understand why you would be using GlobalAlloc. You only need that for DDE which I cannot possibly imagine you are using here. Use malloc or new[] if this really is C++.

If you really are using a DDE API that requires GlobalAlloc, then keep the GlobalAlloc part separate from the string reversing code. Mixing the two concerns makes for unmaintainable code.

If this really is C++ then std::string is what you should be using wherever possible.

I also think there's great confusion with all the casting and all the non-standard Windows type macros. It makes the code pretty much unreadable.

There is also an indexing error as pointed out by Maximus. For what it is worth, I'd write the function something like this:

char* Reversed(const char* str)
{
   int len = strlen(str);
   char* reversed = (char*) malloc(len+1);
   reversed[len] = 0;//ensure return string has null-terminator
   for (int i = 0; i < len; ++i)
      reversed[len-1-i] = str[i];
   return reversed;
}

The only cast you need is the return value of malloc. If you were to use new[] then you would not even need to do that. In which case the code would be like this:

char* Reversed(const char* str)
{
   int len = strlen(str);
   char* reversed = new char[len+1];
   reversed[len] = 0;//ensure return string has null-terminator
   for (int i = 0; i < len; ++i)
      reversed[len-1-i] = str[i];
   return reversed;
}

One should always strive to write code without casts.

Don't do pointer arithmetic yourself when the index operator [] can be used. It's much easier to read this way.

Answer 2

Try this one )

*(lpReversed + i) = (CHAR) *(a_lpText + nTextLength - i - 1);

And it will be good to check a_lpText for NULL and zero-length!