1

I'm writing a piece of code meant to reverse strings using recursion. I believe my method is correct, but I keep getting a segmentation fault and I'm not sure where it's coming from. All my research indicates that it means I'm doing "something strange with memory". I'm new enough at this that these kinds of errors are still baffling, so any help here would be much appreciated. Here's my code:

#include <iostream>
#include <string>
using namespace std;
class Palindrome
{
    int front;
    int back;
public:
    Palindrome();
    string reverse(string word)
    {
        int len = word.length()-1;
        if (back == 0) {
            back = len;
        }
        if (front >= back)
            return word;
        else{
            char first = word[front];
            char last = word[back];
            word[front] = last;
            word[back] = first;
            front += 1;
            back -= 1;
            reverse(word);
        }
    }
};

Palindrome::Palindrome(){
front = 0;
back = 0;
}
quipish
  • 47
  • 2
  • 7
  • 2
    Have you run this through `gdb`? Where is it segfaulting? In any case, it is likely to be some invalid memory access. – RageD Apr 25 '12 at 16:36
  • 1
    You need to show the calling code too. – Paul R Apr 25 '12 at 16:37
  • 2
    Have you considered `std::reverse` or is this an exercise? Also, how are you calling your function(s)? – Mark B Apr 25 '12 at 16:42
  • 2
    if you call this once with one word (eg. "ReverseThisString") and then again with a word less than half the length of the first word (eg. word, `front` and `back` will both try to access data out of range. Come to think of it, this won't even work if you run it twice. – Jacob Apr 25 '12 at 16:45

3 Answers3

1

What I think Jacob Abrahams was trying to say, front is iterated, but never re-set to zero, so the second time you call it, it will either segfault or produce incorrect results depending on whether the second word is longer or shorter.

Furthermore, what Mark B already hinted at is that you can include algorithm and replace the whole Palindrome::reverse function with 

std::reverse(word.begin(), word.end());

Most of all it would help if you learned how to use a debugger or, in the future, at least give the specific error message for these kinds of questions.

EDIT: Forgot to add that recursion (e.g. a function calling itself) is usually a bad idea, because the execution stack is quite small and in this case, even after fixing the aforementioned issue, you will get a stack overflow for a particularly long string. It actually makes this particular code less clear.

smocking
  • 3,689
  • 18
  • 22
  • 1
    I'm going to go out on a limb and guess this is either an interview question or a exercise. Recursion for the same of recursion is generally not a good idea... – Michael Dorgan Apr 25 '12 at 17:06
  • Hmm didn't think about that, but it makes sense. Certainly hope this guy doesn't end up getting a job he's not qualified for, mostly for his own sake. – smocking Apr 25 '12 at 17:16
  • Don't worry, _she_ didn't get any job with this, and this is a very intro-to-recursion type situation. It's an exercise where we re-write a set of functions iteratively and then recursively, to show they can both be used to solve some of the same problems. I agree that recursion for the sake of recursion is no good, except when one is learning recursion for the first time, as is the case here, then it can be a useful exercise. I can see how it's something that could often go in the, more fancy than useful category though. – quipish Apr 26 '12 at 04:54
  • Also, thanks for the comment about re-setting iterators, I didn't think about it at the time, because I'd set them in the initializer. – quipish Apr 26 '12 at 04:57
1

I tried your code and got an "access violation" too, even with only one call. Beside the initialization issue described in other answers and comments, what is causing your seg fault is the missing "return" before your recursive call to "reverse". You need to write return reverse(word);

In Visual Studio, your original code gives this: warning C4715: 'Palindrome::reverse' : not all control paths return a value.

See this question for more details.

Here's a version of reverse() with both fixes:

    string reverse(string word)
    {
        int len = word.length()-1;
        if (back == 0) 
        {
            back = len;
        }
        if (front >= back)
        {
            front = 0;
            back = 0;
            return word;
        }
        else
        {
            char first = word.at(front);
            char last = word.at(back);
            word.at(front) = last;
            word.at(back) = first;
            front += 1;
            back -= 1;
            return reverse(word);
        }
    }
Community
  • 1
  • 1
Jem
  • 2,255
  • 18
  • 25
  • Thank you! Of course you have to return the function itself in order to recurse, learning curve thing. Much appreciated. – quipish Apr 26 '12 at 05:04
1

Personally, I consider mixing recursion and objects somewhat odd. One of the fundamental concepts of objects is that the object holds state that you want to keep track of. One of the fundamental concepts of recursion is that the execution stack holds the state you want to keep track of.

In this case, the state you want to keep track of is how much of the string has been processed/how much of the string remains to be processed. You can keep track of that without an object.

This smells a lot like a homework question. But I can't think of a hint to give you without just handing you the answer. The best I can do is make my answer (1) reverse any container, including but not limited to strings; (2) use an STL-like interface (i.e., iterators); and (3) reverse the string in place instead of reversing a copy of the string:

#include <algorithm> // std::swap

// the other headers are only for my example on how to use the code
#include <iostream>
#include <iterator>
#include <string>
#include <list>

template<typename Itor> void reverse_with_recursion(Itor begin, Itor end)
{
    using std::swap; // same trick used by the STL to get user-defined swap's,
                     // but fall back to std::swap if nothing else exists:
                     // http://en.wikipedia.org/wiki/Argument-dependent_name_lookup#Interfaces

    // if begin and end are pointing at the same element,
    // then we have an empty string and we're done
    if (begin == end) {
        return;
    }

    // the STL follows the pattern that end is one element after
    // the last element;  right now we want the last element
    --end;

    // if begin and end are pointing at the same element *now*,
    // then we have a single character string and we're done
    if (begin == end) {
        return;
    }

    swap(*begin, *end);
    return reverse_with_recursion(++begin, end);
}

int main()
{
    std::string foo("hello world");
    reverse_with_recursion(foo.begin(), foo.end());

    std::cout << foo << '\n';

    std::list<int> bar;
    for (int i = 0; i < 10; ++i) {
       bar.push_back(i);
    }

    reverse_with_recursion(bar.begin(), bar.end());

    std::copy(bar.begin(),
              bar.end(),
              std::ostream_iterator<int>(std::cout, " "));
    std::cout << '\n';
Max Lybbert
  • 19,717
  • 4
  • 46
  • 69