patterncModerate
Can this recursive binary search in C be made more concise?
Viewed 0 times
thiscanmadesearchmoreconciserecursivebinary
Problem
Can someone please let me know if they see glaring issues with this code? I tested a handful of cases and it seems to work but in other threads. I often see much more concisely written versions of this. I just want to know if anything I have done is superfluous or just stylistically poor form.
#include
int value = 5;
int values[] = {2,4,5,12,23,34};
int n = 6;
int re_search(int value, int values[], int n);
int main(void)
{
re_search(value, values, n);
return 0;
}
int re_search(int value, int values[], int n)
{
int first = 0;
int last = n-1;
int middle = (first+last)/2;
while (first values[middle])
{
first = middle + 1;
middle = (first + last) / 2;
return re_search(value, &values[middle], last-first);
}
else
{
last = middle - 1;
return re_search(value, &values[middle], last-first);
}
middle = (first + last) / 2;
}
printf("Did not find it\n");
return 0;
}Solution
Things you did well
-
Declaring your parameters as
-
Keeping your dependencies to a minimum.
Things you could improve
Bugs:
-
You sorted your array for your program.
Your program fails to find your
Let the computer do the hard work by using
Efficiency:
-
Recursion is slowing you down in your
Variables/Initialization:
-
You shouldn't use global variables.
The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.
To understand how the application works, you pretty much have to take into account every function which modifies the global state. That can be done, but as the application grows it will get harder to the point of being virtually impossible (or at least a complete waste of time).
If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.
So instead of using global variables, initialize the variables in
-
Right now your
Since this is just the number of elements in your array, we can make this a bit more dynamic, so that you don't have to change it if you add a few more elements to your array.
-
Right now you are hard coding the number you are looking for.
I would either let the user enter it, or determine a random number to search for.
Syntax/Styling:
-
You print out items to the console within your
I would have it so your function only evaluates if the number exists in the array and returns whether it could find it or not. Then let
Final Code
I've rewritten your code so that it is bug free, and so that it is a bit more dynamic. I also rewrote some bits so you could see what was going on, and so the program was more transparent.
-
Declaring your parameters as
void when you don't take in any arguments.-
Keeping your dependencies to a minimum.
Things you could improve
Bugs:
-
You sorted your array for your program.
int values[] = {2,4,5,12,23,34};Your program fails to find your
value if your array is not sorted initially.int values[] = {9,4,3,12,69,34};Let the computer do the hard work by using
qsort().Efficiency:
-
Recursion is slowing you down in your
re_search() method. Use iteration instead. The conversion isn't too hard, there are a lot of similarities.int binarySearch(int a[], int low, int high, int target)
{
while (low a[middle]) low = middle + 1;
else return middle;
}
return -1;
}Variables/Initialization:
-
You shouldn't use global variables.
int value = 5;
int values[] = {2,4,5,12,23,34};
int n = 6;The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.
To understand how the application works, you pretty much have to take into account every function which modifies the global state. That can be done, but as the application grows it will get harder to the point of being virtually impossible (or at least a complete waste of time).
If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.
So instead of using global variables, initialize the variables in
main(), and pass them as arguments to functions if necessary.-
Right now your
n variable is hard coded.int n = 6;Since this is just the number of elements in your array, we can make this a bit more dynamic, so that you don't have to change it if you add a few more elements to your array.
int n = sizeof(values)/sizeof(values[0]);-
Right now you are hard coding the number you are looking for.
int value = 5;I would either let the user enter it, or determine a random number to search for.
srand((unsigned int) time(NULL)); // seeding
int val = values[rand() % n];Syntax/Styling:
-
You print out items to the console within your
re_search() method.printf("Found it!\n");
...
printf("Did not find it\n");I would have it so your function only evaluates if the number exists in the array and returns whether it could find it or not. Then let
main() do all the printing to the console based on that information.printf("%s\n", (re_search(value, values, n)) ? "Found it!" : "Didn't find it.");Final Code
I've rewritten your code so that it is bug free, and so that it is a bit more dynamic. I also rewrote some bits so you could see what was going on, and so the program was more transparent.
#include
#include
#include
int binarySearch(int a[], int low, int high, int target)
{
while (low a[middle]) low = middle + 1;
else return middle;
}
return -1;
}
int compare(const void *a, const void *b)
{
return (*(int*)a - *(int*)b);
}
void printArray(int arr[], int size)
{
printf("{ ");
for (int i = 0; i = 0) printf("Found %d at index %d\n", val, found);
else printf("Didn't find %d\n", val);
return 0;
}Code Snippets
int values[] = {2,4,5,12,23,34};int values[] = {9,4,3,12,69,34};int binarySearch(int a[], int low, int high, int target)
{
while (low <= high)
{
int middle = low + (high - low)/2;
if (target < a[middle]) high = middle - 1;
else if (target > a[middle]) low = middle + 1;
else return middle;
}
return -1;
}int value = 5;
int values[] = {2,4,5,12,23,34};
int n = 6;int n = sizeof(values)/sizeof(values[0]);Context
StackExchange Code Review Q#44199, answer score: 10
Revisions (0)
No revisions yet.