views:

221

answers:

4
+3  A: 

First of all, you have two is in your code. That shouldn't even compile.

Also, index_of((i++), (s++)); is effectively the same as:

index_of(i, s); 
++i;
++s;

In other words, you calling index_of over & over with the same parameters it was called with the first time. (it'll never return to get to the ++i).

Also, purely stylistic, but since the return type is an int, you should return 0, and save NULL for use with pointers.

James Curran
A: 

The main problem here is that you are using post-increment, and result of (i++) is i . You have to use ++i

a1ex07
+4  A: 

This is how I would do it, which is simplify everything by moving it into different functions. Untested but it should hopefully give you an idea.

/**
 * Checks if one string starts with another string. This returns an
 * incorrect result if it is called where prefix is an empty string.
 */
bool starts_with_impl(const char* haystack, const char* prefix) {
    if ( *prefix == 0 ){ 
        //reached the end of prefix without having found a difference in characters
        return true;
    }else if ( *haystack == 0 || *prefix != *haystack ){
        //either prefix is longer than haystack or there is a difference in characters.
        return false;
    }
    //move along the haystack and prefix by one character
    return starts_with_impl(++haystack, ++prefix);
}
/**
 * Wrapper around starts_with_impl that returns false if prefix is an empty string
 */
bool starts_with(const char* haystack, const char* prefix) {
    return *prefix ? starts_with_impl(haystack, prefix) : false;
}

int get_substr_impl(const char* haystack, const char* const needle, int index) {
    if ( *haystack == 0 ){
        //reached the end of haystack with no match, -1 is no string found
        return -1;
    }else if ( starts_with(haystack, needle) ){
        //we have found a substring match.
        return index;
    }
    //move along haystack by one character
    return get_substr_impl(++haystack, needle, ++index);
}
/**
 * Wrapper function for the above to hide the fact we need an additional argument.
 * I am avoiding using a default argument as it makes a messy api
 */
int get_substr(const char* haystack, const char* const needle) {
    return get_substr_impl(haystack, needle, 0);
}

From the comments

you've got 2 const's in the get_substr_impl method....

Intentional.

// means that the data is constant, in other words I can't change the value of needle
const char* needle;

//means that as well as the data being constant 
//I can't change the address that the pointer points to.
const char* const needle;

from the main method wouldn't i just call teh get_substr_impl with the same prameters given in get_substr?

I split it as get_substr_impl has an additional (required) argument int index that is required for the inner workings of the function and should always start at 0. While you could call get_substr_impl("abc", "a", 0);, it looks nicer and is more understandable to call get_substr("abc", "a"); and it avoids errors (like calling get_substr_impl("abc", "a", 1);)

Yacoby
+1 just for the parameter names!
James Curran
you've got 2 `const`'s in the `get_substr_impl` method.... and where did *other come from/goto? --- and from the `main` method wouldn't i just call teh get_substr_impl with the same prameters given in `get_substr`? --- would that be for error handling?
Wallter
:-) what if needle is empty?
Eamon Nerbonne
@Eamon if needle is a pointer to `\0` it will return -1, if needle is `0` then the behavior is undefined.
Yacoby
All strings start with the empty string.
Eamon Nerbonne
Great parameter names indeed, btw. Clear names are half the work :-) - I'll use those too!
Eamon Nerbonne
@Eamon you were right about the issues with an empty `needle`, all code has been run through a compiler, so should be far more correct.
Yacoby
The `needle` and `haystack` parameter names were one of the things I really liked about PHP (Which is where I stole them from ;))
Yacoby
+9  A: 
  1. Don't change any state variables. Your code should not include the operator ++ anywhere. You are not trying to loop over a datastructure and change your local variables in some fashion - you are trying to generate an entirely new but smaller problem each time. So, all those ++ operators - whether pre or post increment - are red flags.
  2. You have more than one sub-problem. (...so single function recursion isn't ideal).

    Let's look at this systematically.

    Assume you have a working index_of and you just want to call it with input that's shorter than yours, and that both haystack and needle aren't empty yet. Then one of two things may be:

    • The haystack starts with the same letter as the needle, and you just need to look a little deeper to verify this.
      - What happens if verification succeeds - what if it fails? is this an index_of subproblem?

    • ...or the haystack starts off wrong, and you need to look deeper.
      - Is looking deeper into the haystack an index_of subproblem?

    Notice that if the haystack starts OK it doesn't necessarily mean that it starts with the full search string - and if it starts OK but does not start with the full search string, you really don't need to continue looking. That means that the "starts-with" sub-problem is fundamentally different than the index-of sub-problem:

    • index_of: here, failure to find the search string at index 0 means you need to look further.
    • starts_with: here, failure to find the search string at index 0 means you need to stop.

    It is possible to say startswith(haystack, needle):= 0==index_of(haystack, needle), but obviously index_of is a more complicated problem with a more expensive solution - so you shouldn't do that; it's also more confusing at first.

  3. Identify your base cases - When either needle or haystack are empty, what does that mean?

  4. Good names help clarify things, and recursion is hard to read at the best of times - Yacoby's reply for instance has some meaningful choices here.

Summary

I'm not going to solve your own puzzle for you, but a few tips in recap...

  • Avoid changing your local variables: you're trying to define a subproblem and then just call the right function for those newer, shorter parameters. Don't use side-effects, they make things terribly complex.
  • Recursion doesn't necessarily mean just one function A calling A - it can be any cyclic call graph that eventually terminates; you may use more functions
  • If needle and haystack start with the same letter, that doesn't mean that the entire needle is at the start of the haystack - and if it is not, you still need to continue searching
  • This line is wrong: if (*s == '\0') { return 1; }
Eamon Nerbonne
Very good systematic explanation
VoidPointer
With out using the `++` operator how am i supposed to increment the pointer? GREAT EXPLANATION thank you!
Wallter
Don't "increment" the pointer - pass a *new* pointer representing a substring - i.e. something similar to this: *if-not-found-here:* `return index_of(haystack+1, needle);`
Eamon Nerbonne
brilliant! I never thought of doing it that way -> thanks!
Wallter
BTW, something "similar-to-this" would return the incorrect index; you still need logic to compute that to compensate for the different string position and the possibility no index is found...
Eamon Nerbonne