Where is the mistake in my code to perform Binary Search?

Question

I was writing up code for a binary search algorithm.

Code:

#include "cs50.h"

int main(void) {
    int n = GetInt();
    int value = GetInt();
    int values[n];

    for (int i = 0; i < n; i++) {
        printf("Put in number %i ", i + 1);
        values[i] = GetInt();
    }

    int mid = (n - 1) / 2;
    int en = 0;
    int ex = n - 1;

    for (int i = 0, xt = i + 1; i < xt; i++) {
        if (value > values[mid]) {
            en = mid;
            mid = (en + ex) / 2;
        }
        else if (value < values[mid]) {
            ex = mid;
            mid = (en + ex) / 2;
        }
        else if (value == values[mid]) {
            printf("found");
            break;
        } else {
            printf("not found");
            break;
        }
    }
}

But it only works when the value to be found is somewhere in the middle.

It fails when :

value to be found is first or last.
value to be found is not in the values inputted.

I really cannot figure out the mistake.

This is an ideal situation where using a debugger will help you immensely. Or how about even some debug print statements? These are just standard debugging 101 steps you should perform before seeking help. — kaylum, Jul 08 '16 at 03:32
This is a good starting point for debugging: https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ . In this case, I'd recommend removing the `GetInt` and starting `values` with a minimal case that you know goes wrong, and then figuring out what happens. — Paul Hankin, Jul 08 '16 at 03:34
This might start with not commenting code (you use what _looks_ a "counting loop", a binary search has no use for such), choosing lazy variable names (`i` is perfectly OK for a "counter variable", `mid` and `n` are easy enough, but `en` and `ex` are asking for trouble (`lo`&`hi` would be lazy enough, but with IDE ubiquity, everyone seems better off with `low`&`high`)), and putting funny branches in _if-else if-else chains_: >, <, ==, _and else_? (`everything seems alright` - according to?) — greybeard, Jul 08 '16 at 03:55
Under what circumstances do you think the 'not found' message will be printed? That `else` clause seems redundant to me. The test for equality is also redundant; if the value is neither greater than nor less than the value, it must be equal to it. — Jonathan Leffler, Jul 08 '16 at 04:59

score 6 · Answer 1 · edited Jun 25 '17 at 14:40

There are a bunch of little things you have to get right in a binary search: handle the length=0 case, make sure the position you test is always valid, make sure you don't overflow (i.e., `(low+high)/2' is not the best way to write that), make sure the new test position is always different from the previous one, etc.

After having done it like a million times, every binary search I write is now done just like this:

bool search(int array[], int length, int valueToFind)
{
    int pos = 0;
    int limit = length;
    while(pos < limit)
    {
        int testpos = pos + ((limit - pos) >> 1);

        if (array[testpos] < valueToFind)
            pos = testpos + 1;
        else
            limit = testpos;
    }
    return (pos < length && array[pos] == valueToFind);
}

Notice that we only need to do one comparison per iteration, which is faster than the searches in the other answers. Instead of doing the equality test inside the loop, we reliably find the position where the element to find belongs, using only one comparison per iteration, and then at the end test to see if the element we want is there.

The way we calculate testpos ensures that pos <= testpos < limit, AND it works even if length is the largest possible integer value.

This form also makes it very easy to read off the invariants you want to see, without having to think about strange boundary conditions like high<low. When you come out of the loop, pos==limit so you don't have to worry about using the wrong one, etc.

The condition in this loop is also easily adaptable to different-purpose binary searches like "find where to insert x, ensuring that it goes after all the xs that are already in the array", "find the first x in the array", "find the last x in the array", etc.

Very good algorithm, `pos` is the smallest index even in case of duplicates. This answer deserves more upvotes and a validation mark. — chqrlie, Jun 25 '17 at 14:34

score 2 · Answer 2 · answered Jul 08 '16 at 03:35

2

It seems to me that the for loop will only run 1 time

for(int i = 0, xt = i + 1; i < xt; i++) {}

Also for your binary search algorithm to work, your list of integers would need to be sorted. I'm not sure what GetInt() does exactly, but if it returns a random value, that would also cause the search to fail.

answered Jul 08 '16 at 03:35

Velvacaine

103
7

`GetInt()` is a function from the [CS50](https://manual.cs50.net/library/) library. It reads an integer from standard input. – Jonathan Leffler Jul 08 '16 at 05:03

Cherubim · Answer 3 · 2017-06-26T04:16:35.833

As others have mentioned, you have got your conditions wrong int the for loop.

And one more issue is that

    if (value > values[mid])
    {
        en = mid;
        mid = (en+ex)/2;
    }

here when (value > values[mid]) you are assigning mid to en but instead mid+1 must be assigned as you have to search for the element after mid
similarly, when if (value < values[mid]), ex must be assigned with mid-1 as you have to search for element till the index 1 less than mid

A much better implementation of binary search would be the following:

Note: I have used low and high instead of en and ex respectively

int mid; //no need to initialize as mid is initialized at the start of each iteration
int low = 0; //instead of en
int high = n-1; //instead of ex

while( low <= high )
{
    mid = low + ((high - low) / 2);; //updating mid value

    if (value > values[mid])
    {
        low = mid+1; //updating low
    }

    else if (value < values[mid])
    {
        high = mid-1; //updating high
    }

    else // if (value == values[mid])
    {
        printf("found"); //if found print 'found'
        break;
    }
}

if(low>high)
    printf("not found\n");

making the above changes, your code would be:

#include "cs50.h"
#include <stdio.h>

int main(void)
{
    int n = GetInt();
    int value = GetInt();

    if (n <= 0) 
    {
        //handle the error or exit
    }

    int values[n];

    for (int i=0;i<n;i++)
    {
        printf("Put in number %i \n",i+1);
        values[i]=GetInt();
    }

    int mid;
    int low = 0;
    int high = n-1;

    while( low <= high )
    {
        mid = low + ((high - low) / 2);

        if (value > values[mid])
        {
            low = mid+1;
        }
        else if (value < values[mid])
        {
            high = mid-1;
        }
        else 
        {
            printf("found");
            break;
        }
    }
    if(low>high)
        printf("not found\n");

}

sample input:

5 //n
5 //value
1 2 3 4 5 //array elements

sample output:

Put in number 1 
Put in number 2 
Put in number 3 
Put in number 4 
Put in number 5 
found

for further reading, read this : click

`int values[n];` has undefined behavior if `n < 0`, `mid = (low + high)/2;` has a potential undefined behavior in case of arithmetic overflow. The last test `if (value == values[mid])` is redundant. — chqrlie, Jun 25 '17 at 15:29
thanks for pointing out @chqrlie, I tried making the changes in my post. Let me know if I got anything wrong. — Cherubim, Jun 25 '17 at 19:06

Where is the mistake in my code to perform Binary Search?

3 Answers3

Linked