1
int recurbinarysearch(int * oringalary, int low, int high, int target) {

    if (low > high) {
        return -1;
    } else {
        int mid = (low + high) / 2;
        if (target == * (oringalary + mid)) return mid;
        if (target > * (oringalary + mid)) return recurbinarysearch(oringalary, mid + 1, high, target);
        if (target < * (oringalary + mid)) return recurbinarysearch(oringalary, low, mid - 1, target);
    }
}

Does anyone see an error in my recursive binary search algorithm? Occasionally it returns an index that is incorrect (most often it is off by one), but once in awhile its way off. Usually however, it is right. I do not see the issue, would appreciate the help.

Karthik T
  • 31,456
  • 5
  • 68
  • 87
SystemFun
  • 1,062
  • 4
  • 11
  • 21
  • You sure the array is sorted? – nhahtdh Feb 19 '13 at 04:17
  • Yup almost 100% the array is printed out and it appears to be sorted correctly. – SystemFun Feb 19 '13 at 04:19
  • Is it possible that there's some kind of cumulative rounding error that results from dividing (low + high)/2? –  Feb 19 '13 at 04:22
  • Also, returns 0, not -1 if the number is not found.. – SystemFun Feb 19 '13 at 04:23
  • I don't belive so, they are both int's. so no loss of precision. if it's an odd number it will still eventually zero on the middle. In theory.. – SystemFun Feb 19 '13 at 04:24
  • this is initially fed `0, n-1` for an n-length array, right ? – WhozCraig Feb 19 '13 at 04:25
  • @WhozCraig: Nope, I spent twenty minutes not even looking at the calling code... that wasn't very smart... I was giving it n..... to many printf's allocate array's and sort's... used to passing the size. Thanks for the help. – SystemFun Feb 19 '13 at 04:30
  • If you wanna post an answer I'll give you an accept. – SystemFun Feb 19 '13 at 04:33
  • Not a problem, but you should also throw out the unneeded comparison in your algorithm. You want me to ex. that as well ? – WhozCraig Feb 19 '13 at 04:35
  • I am going to close this as too localized, since there is absolutely no clue from the question, and we have to guess from what we don't have. – nhahtdh Feb 19 '13 at 04:36
  • 1
    I'll just point out that `* (oringalary + mid)` is a rather strange and ugly way to write `oringalary[mid]`. – Gene Feb 19 '13 at 04:40

2 Answers2

1

EDIT This is accepted so I guess I should really try to turn it into a correct answer.

I originally assumed (see "NOTE" onwards below) that the question was using half-open bounds. It's actually (correctly) using inclusive bounds. In the initial call, low=0 and high=n-1.

Using inclusive bounds is often considered a bad thing - see Dijkstra's classic here (PDF). In C-family languages, half-open bounds are a common conventions, even the preference for for (i = 0; i < n; i++) over for (i = 0; i <= n-1; i++). However, given that inclusive bounds are used, the code in the question seems correct.

As was spotted by WhozCraig in the comments, though, the calling code wasn't respecting that convention, and was passing the wrong bounds - including an out-of-bounds garbage item in the search range. Because that extra item is garbage, the assumption that the items in the range are sorted is also possibly invalidated. Most searches won't find that garbage item (because you're unlikely to be searching for whatever junk value it has) but it will misdirect the search.


NOTE This probably isn't an answer, but it's too long for a comment.

Are your bounds inclusive, exclusive, or half-open?

I'm going to assume half-open - inclusive at low, exclusive at high. If so, this line looks wrong...

if (target < * (oringalary + mid))
    return recurbinarysearch(oringalary, low, mid - 1, target);

The reason is you've checked the item at mid, but you're using mid - 1 as your new exclusive upper bound. This means the item at mid - 1, which has not been checked, has been accidentally excluded from the search. The line should be...

if (target < * (oringalary + mid))
    return recurbinarysearch(oringalary, low, mid, target);

This keeps the item at mid - 1 in the range to search. The item at mid won't be searched again because the upper bound is exclusive.

Messing up the bounds in binary searches is a common problem, and it causes more errors than it looks like it should.

However, this in itself doesn't explain your symptoms - it should fail to find items sometimes (probably around 50% of searches) a lot but it shouldn't report the wrong position for searches that succeeded.

The usual symptoms for wrong bounds in binary searches are either infinite loops (the same item being repeatedly checked because it wasn't excluded from bounds) or searches failing to find items that are present (because items were excluded from the search range that weren't checked).

To be honest, I can't see how your symptoms can be occurring. Every possible way for the function to exit should give a correct successful result, or else the -1 failure result. The only possible exceptions I can think of would be outside of this code - misinterpreting the result, e.g. by failing to check for the -1 result.

BTW - this is the perfect opportunity for me to rep-whore my question and answer about the one-comparison-per-iteration binary search.

EDIT I think I spotted another problem with the bounds - still assuming half-open, this line is wrong...

if (low > high) {

and should be...

if (low >= high) {

The reason being that for half-open bounds, if the bounds are equal there's no items in between to check - even the low-bound item isn't valid, because the high bound is equal to it and exclusive. This allows you to still test

Community
  • 1
  • 1
  • +1 I actually thought the indexes looked half-baked when I first saw the problem. I'm used to doing this a single pointer and upper-limit, i.e. `bs(ar,len)` computes mid, and then either returns true on discovery, or returns `bs(ar, mid)` or `bs(ar+mid, len-mid)`, but that is more for determining existence rather than fetching the actual *value*. Thanks for catching this. – WhozCraig Feb 19 '13 at 05:09
  • I think that the bounds are inclusive in this particular implementation. In that case they work correctly *if initially fed inclusive bounds*. As OP noted, he was incorrectly passing in half-open bounds. – nneonneo Feb 19 '13 at 05:42
1

For a thorough discussion of binary search, study Jon Bentley's Programming Pearls.

Here's a test harness for your code, very much inspired by Programming Pearls, plus an instrumented version of your code. The only change I made is adding the (now commented out) debug printing in the binary search. The output from the test code is almost perfect (the harness says everything passes, but it is not quite correct):

N =  0: 
search for  0 in  0 entries - returned  0 found  0 PASS
N =  1: [0] = 1;
search for  0 in  1 entries - returned -1          PASS
search for  1 in  1 entries - returned  0 found  1 PASS
search for  2 in  1 entries - returned -1          PASS
N =  2: [0] = 1;[1] = 3;
search for  0 in  2 entries - returned -1          PASS
search for  1 in  2 entries - returned  0 found  1 PASS
search for  2 in  2 entries - returned -1          PASS
search for  3 in  2 entries - returned  1 found  3 PASS
search for  4 in  2 entries - returned -1          PASS
N =  3: [0] = 1;[1] = 3;[2] = 5;
search for  0 in  3 entries - returned -1          PASS
search for  1 in  3 entries - returned  0 found  1 PASS
search for  2 in  3 entries - returned -1          PASS
search for  3 in  3 entries - returned  1 found  3 PASS
search for  4 in  3 entries - returned -1          PASS
search for  5 in  3 entries - returned  2 found  5 PASS
search for  6 in  3 entries - returned -1          PASS
N =  4: [0] = 1;[1] = 3;[2] = 5;[3] = 7;
search for  0 in  4 entries - returned -1          PASS
search for  1 in  4 entries - returned  0 found  1 PASS
search for  2 in  4 entries - returned -1          PASS
search for  3 in  4 entries - returned  1 found  3 PASS
search for  4 in  4 entries - returned -1          PASS
search for  5 in  4 entries - returned  2 found  5 PASS
search for  6 in  4 entries - returned -1          PASS
search for  7 in  4 entries - returned  3 found  7 PASS
search for  8 in  4 entries - returned -1          PASS
N =  5: [0] = 1;[1] = 3;[2] = 5;[3] = 7;[4] = 9;
search for  0 in  5 entries - returned -1          PASS
search for  1 in  5 entries - returned  0 found  1 PASS
search for  2 in  5 entries - returned -1          PASS
search for  3 in  5 entries - returned  1 found  3 PASS
search for  4 in  5 entries - returned -1          PASS
search for  5 in  5 entries - returned  2 found  5 PASS
search for  6 in  5 entries - returned -1          PASS
search for  7 in  5 entries - returned  3 found  7 PASS
search for  8 in  5 entries - returned -1          PASS
search for  9 in  5 entries - returned  4 found  9 PASS
search for 10 in  5 entries - returned -1          PASS
N =  6: [0] = 1;[1] = 3;[2] = 5;[3] = 7;[4] = 9;[5] = 11;
search for  0 in  6 entries - returned -1          PASS
search for  1 in  6 entries - returned  0 found  1 PASS
search for  2 in  6 entries - returned -1          PASS
search for  3 in  6 entries - returned  1 found  3 PASS
search for  4 in  6 entries - returned -1          PASS
search for  5 in  6 entries - returned  2 found  5 PASS
search for  6 in  6 entries - returned -1          PASS
search for  7 in  6 entries - returned  3 found  7 PASS
search for  8 in  6 entries - returned -1          PASS
search for  9 in  6 entries - returned  4 found  9 PASS
search for 10 in  6 entries - returned -1          PASS
search for 11 in  6 entries - returned  5 found 11 PASS
search for 12 in  6 entries - returned -1          PASS
N =  7: [0] = 1;[1] = 3;[2] = 5;[3] = 7;[4] = 9;[5] = 11;[6] = 13;
search for  0 in  7 entries - returned -1          PASS
search for  1 in  7 entries - returned  0 found  1 PASS
search for  2 in  7 entries - returned -1          PASS
search for  3 in  7 entries - returned  1 found  3 PASS
search for  4 in  7 entries - returned -1          PASS
search for  5 in  7 entries - returned  2 found  5 PASS
search for  6 in  7 entries - returned -1          PASS
search for  7 in  7 entries - returned  3 found  7 PASS
search for  8 in  7 entries - returned -1          PASS
search for  9 in  7 entries - returned  4 found  9 PASS
search for 10 in  7 entries - returned -1          PASS
search for 11 in  7 entries - returned  5 found 11 PASS
search for 12 in  7 entries - returned -1          PASS
search for 13 in  7 entries - returned  6 found 13 PASS
search for 14 in  7 entries - returned -1          PASS
N =  8: [0] = 1;[1] = 3;[2] = 5;[3] = 7;[4] = 9;[5] = 11;[6] = 13;[7] = 15;
search for  0 in  8 entries - returned -1          PASS
search for  1 in  8 entries - returned  0 found  1 PASS
search for  2 in  8 entries - returned -1          PASS
search for  3 in  8 entries - returned  1 found  3 PASS
search for  4 in  8 entries - returned -1          PASS
search for  5 in  8 entries - returned  2 found  5 PASS
search for  6 in  8 entries - returned -1          PASS
search for  7 in  8 entries - returned  3 found  7 PASS
search for  8 in  8 entries - returned -1          PASS
search for  9 in  8 entries - returned  4 found  9 PASS
search for 10 in  8 entries - returned -1          PASS
search for 11 in  8 entries - returned  5 found 11 PASS
search for 12 in  8 entries - returned -1          PASS
search for 13 in  8 entries - returned  6 found 13 PASS
search for 14 in  8 entries - returned -1          PASS
search for 15 in  8 entries - returned  7 found 15 PASS
search for 16 in  8 entries - returned -1          PASS
N =  9: [0] = 1;[1] = 3;[2] = 5;[3] = 7;[4] = 9;[5] = 11;[6] = 13;[7] = 15;[8] = 17;
search for  0 in  9 entries - returned -1          PASS
search for  1 in  9 entries - returned  0 found  1 PASS
search for  2 in  9 entries - returned -1          PASS
search for  3 in  9 entries - returned  1 found  3 PASS
search for  4 in  9 entries - returned -1          PASS
search for  5 in  9 entries - returned  2 found  5 PASS
search for  6 in  9 entries - returned -1          PASS
search for  7 in  9 entries - returned  3 found  7 PASS
search for  8 in  9 entries - returned -1          PASS
search for  9 in  9 entries - returned  4 found  9 PASS
search for 10 in  9 entries - returned -1          PASS
search for 11 in  9 entries - returned  5 found 11 PASS
search for 12 in  9 entries - returned -1          PASS
search for 13 in  9 entries - returned  6 found 13 PASS
search for 14 in  9 entries - returned -1          PASS
search for 15 in  9 entries - returned  7 found 15 PASS
search for 16 in  9 entries - returned -1          PASS
search for 17 in  9 entries - returned  8 found 17 PASS
search for 18 in  9 entries - returned -1          PASS

Almost all of that is fine; the only problem child is the very first search, which should fail because no value should be found in an empty array.

The test code is:

#include <stdio.h>

int recurbinarysearch(int *oringalary, int low, int high, int target)
{
    //printf("-->> %d..%d: ", low, high);
    if (low > high)
    {
        //printf("<<-- %d ", -1);
        return -1;
    }
    else
    {
        int mid = (low + high) / 2;
        if (target == * (oringalary + mid))
        {
            //printf("<<-- %d ", mid);
            return mid;
        }
        if (target > * (oringalary + mid))
        {
            int r = recurbinarysearch(oringalary, mid + 1, high, target);
            //printf("<<-- %d ", r);
            return r;
        }
        if (target < * (oringalary + mid))
        {
            int r = recurbinarysearch(oringalary, low, mid - 1, target);
            //printf("<<-- %d ", r);
            return r;
        }
    }
}

int main(void)
{
    for (int i = 0; i < 10; i++)
    {
        int a[i+1]; // No zero-size arrays in C
        printf("N = %2d: ", i);
        for (int j = 0; j < i; j++)
        {
            a[j] = 2 * j + 1;
            printf("[%d] = %d;",j, a[j]);
        }
        putchar('\n');

        for (int j = 0; j < 2*i+1; j++)
        {
            int f = recurbinarysearch(a, 0, i, j);
            //putchar('\n');  // debug
            printf("search for %2d in %2d entries - returned %2d",
                    j, i, f);
            if (f >= 0 && f <= i)
            {
                printf(" found %2d", a[f]);
                printf(" %s", (a[f] == j) ? "PASS" : "FAIL");
            }
            else
                printf(" %8s %s", "", (j % 2 == 0) ? "PASS" : "FAIL");
            putchar('\n');
        }
    }
    return(0);
}

I'm going to leave you to work out how to deal with the empty array case.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278