String Reversal

Question

Wrote a practice code for reversing a string say. Input = "This is funny" Expected output = "ynnuf si siht"

This code works for me. Want to know your comments and a better optimised way to perform this.

void reverse_string(std::string input)
{ 
 int N = input.length();
 char buffer[100]; 
 char* ptr1;
 char* ptr2;
 ptr1= ptr2 = const_cast<char*>(input.c_str());
 while(*ptr2){ptr2++;}
 ptr2--;
 int i =0;
 while(ptr2 >= ptr1)
 {
 buffer[i] = *ptr2;
 ptr2--;
 i++;
 }
 buffer[i+1]='0円';
 for(int j=0;j<N;j++)
 {
 printf("%c",buffer[j]);
 }
 std::cout << endl;
}

• CiaPan: This seems more like a C code working on C++ data structure. Three hints: 1) you don't need a formatted print to output a single character; 2) once you have an array of chars to be output, you may print is at once as a string, no need to iterate each char separately; 3) don't mix printf and stdout << ....
• 200_success: @CiaPan Please write all critiques in answers, not comments.
• CiaPan: @200_success I'm pretty sure that's far too small to be an answer, esp. as a code review answer. Probably I would post it as an answer if we were at StackOverflow, but here, at CodeReview I'll leave it in a comment space.
• 200_success: @CiaPan Length is irrelevant. Comments are for helping to improve the question, and are therefore not an appropriate place to give advice for how to improve the code.
• funwithlinx: @CiaPan yes i modified it to use only std::cout and got it printed as expected. But printf is printing a random character after reversed.

Answer 1

You don't actually have to physically reverse the string in order to print it reversed.

#include <iostream>
void print_reversed_string(std::string s) {
 const char *begin = s.c_str();
 for (const char *current = begin + s.length() - 1; begin <= current; --current) {
 std::cout << *current;
 }
 std::cout << std::endl;
}
int main() {
 print_reversed_string("This is funny.");
}

This reverses the string without swapping any values in memory.

I wouldn't be surprised if std::reverse (as used in this answer) did something like this behind the scenes. If so, that is likely to be better optimized and more generally applicable than this solution.

Separating reversing and printing would often be good, although it simply won't work with this solution. Another reason why a compiler-optimized version of std::reverse would be better.

Note that I renamed the function to match what your original function did rather than what it said.

• Juho: You should also take the argument as a const-ref since you are not modifying it.
• CiaPan: Even shorter loop: const char *begin = s.c_str(), *current = begin + s.length() - 1; while(begin <= --current) { std::cout << *current; }

Answer 2

Avoid reinventing the wheel by using standard algorithms, such as std::reverse. Concretely, you could just do:

#include <iostream>
#include <string>
#include <iterator>
int main()
{
 std::string s = "this is funny";
 // Print it reversed, but don't modify s
 std::copy(s.rbegin(), s.rend(), std::ostream_iterator<char>(std::cout));
 std::cout << "\n";
 // Reverse s, and then print it
 std::reverse(s.begin(), s.end());
 std::cout << s << "\n";
}

• Bohdan Mart: I guess he is learning programming, so he need to write such tasks
• Juho: @BogdanMart Be as it may, this is the idiomatic way of doing it. I'd argue it's even more important to get things right from the beginning.

Answer 3

Repeated your code then added some comments

void reverse_string(std::string input)

Arguably if you don't intend to alter input, you should make it const

{ 
 int N = input.length();
 char buffer[100]; 

What if someone passes a string of 100 or more characters?

 char* ptr1;
 char* ptr2;
 ptr1= ptr2 = const_cast<char*>(input.c_str());

• You should not be doing a const_cast here. That indicates something dangerous is going on and it is quite wrong in this situation. The char *s should be char const *.

• In c++ it's generally better practice to declare variables only when you need them, and initialise at the point of declaration. N and buffer are not used till quite some way down.

   while(*ptr2){ptr2++;}
  

Add spaces for readability. while (*ptr2) { ptr2++; }

 ptr2--;

What is wrong with char const *ptr2 = ptr1 + strlen(ptr1) - 1; or are you forbidden to use the standard library? Also (pedantically) a std::string may contain multiple 0 bytes, in which case this would only reverse the first part of it.

 int i =0;
 while(ptr2 >= ptr1)

I'd have put a space after the while here, as it isn't a function call. But that's picky.

 {
 buffer[i] = *ptr2;
 ptr2--;
 i++;

It's more usual in c++ to use pre-increment and pre-decrement.

 }
 buffer[i+1]='0円';

N should be declared here, not waaaay up there (though given the performance of string::length it's probably not worth having a separate variable for it).

 for(int j=0;j<N;j++)
 {
 printf("%c",buffer[j]);
 }

Given buffer is a null terminated string you can just use puts to output it. But see below

 std::cout << endl;

• For consistency with the rest of the code, that should be std::endl;
• Using endl is arguable. It flushes the output stream for you and this can be a serious performance hit. In any case "\n" is a lot less typing than std::endl;
• Why mix C and C++ output methods? It'd be clearer to write one of
  • std::cout << buffer << "\n"; or
  • printf("%s\n", buffer).

}

• funwithlinx: oh it shows me the arena that needs development in my programming skills.

Answer 4

Looking only at the code to reverse the string, I think I'd use a somewhat different approach (than either the question or any of the answers I see right now use).

std::string reverse(std::string const &s) { 
 return std::string(s.rbegin(), s.rend());
}

As to the rest of the code: I'd much rather not see something named reverse_string not only reversing the string that it's passed, but also printing out the result (and not providing the result in any other way either). If that's what you really want, you should at least rename the function to something like print_reversed.

As a rule I'd rather keep the two separate though--it's much easier to re-use functionality when each piece only does one thing.

I'd also generally avoid using printf in C++. If you want to use a C output function, you'd probably be better off with putchar. Using the function as I've written it above, the obvious step would be something like:

std::cout << reverse("an input string");

I'd also advise avoiding using std::endl. If you just want to write a new-line, do exactly that: std::cout << "\n";. If you really want t flush the buffer (which endl does in addition to writing a new-line) do that explicitly:

std::cout << "\n" << std::flush;

• Jerry Coffin: Downvoter: care to state why you think this isn't helpful and/or what needs improvement?