Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary Search by Sergey #1

Closed
wants to merge 4 commits into from
Closed

Binary Search by Sergey #1

wants to merge 4 commits into from

Conversation

sergeyenin
Copy link
Contributor

@den-sergeyev @egorhm @tarasevich what do you think?

int mid = (right-left)/2;
if (left>=right) return -1;
if (element < array[mid]) {
return find(array, element, left, mid);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you include mid element in recursive call? I think it's obsolete here and in another branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sergeyenin , I misunderstood @den-sergeyev, so to make it clear what he meant is:

return find(array, element, left, mid-1);

otherwise you will get infinite recursion in some cases.

return find(array, element, left, mid);
}
else if (element > array[mid]) {
return find(array, element, mid, right);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same problem here

@sergeyenin
Copy link
Contributor Author

@den-sergeyev @tarasevich @egorhm more comments?

@@ -1,5 +1,43 @@
public class BinarySearch {
public static int find(int[] array, int element) {
int iterativeResult = findIterative(array, element);
int reccursiveResult = findReccursive(array, element, 0, array.length-1);
int resultValue = (iterativeResult == reccursiveResult) ? iterativeResult : -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest to return something else in this case, e.g. -2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose returning -1 means, that the Binary search have not got correct answer for the questions - what is the element`s positions.
In a case, when Iterative and Recursive functions are returning the different answers - it means that we have not correct answer.
I dont think that return special code is a good idea, since it demands additional contract besides standard one.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sergeyenin, I think you can't distinguish two cases here:

  1. IterativeResult returns -1 and it's correct answer.
  2. IterativeResult and recursiveResult differs, hence something is wrong but you still returns -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, 2-nd case means, that we have not found the correct answer, which more or less equals to 1-st case.
Anyway, it is a tradeoff between returning more rich information and extending the contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@den-sergeyev understands my point :)

@sergeyenin , you will not violate the "contract" if your code is correct ;) and if your code is incorrect it's already violation of the "contract". It's better to extend a contract than to hide mistakes from unit testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarasevich me either, but I don`t see a lot advantages, really)

@sergeyenin sergeyenin closed this Apr 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants