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