Skip to content
This repository has been archived by the owner on Apr 18, 2020. It is now read-only.

Make README more verbose #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RichardBarrell
Copy link

  • Add a section called "Return values" to README.md that explains, for each
    case (needle found, needle too small, needle between two elements, needle too
    big) what value bs() will return.
  • Add a "Parameters" section to README.md that explains each parameter one by
    one. Move the existing explanations of the parameters into it.

I had to read the implementation to understand what values bs() would return when needle is not found, so I've added more text to explicitly spell out what happens in each case.

- Add a "Parameters" section to README.md that explains each parameter one by
  one. Move the existing explanations of the parameters into it.
- Add a section called "Return values" to README.md that explains, for each
  case (needle found, needle too small, needle between two elements, needle too
  big) what value `bs()` will return.
- If you didn't specify `minimumIndex` and `maximumIndex`, the return value will be `-1` in this case.
- If the needle is larger than any element of the haystack, `bs()` returns `-1 * (largestIndex + 2)`, where `largestIndex` is the index of the last element in the haystack.
- If you didn't specify `minimumIndex` and `maximumIndex`, the return value will be `-1 * (haystack.length + 1)`.
- If the needle is in between two elements of the haystack, `bs()` returns `-1 * (largerIndex + 1)`, where `largerIndex` is the index of the larger or the two elements which the `needle` sits between.
Copy link

Choose a reason for hiding this comment

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

There is an easier way to state the return values:

  • If the needle is equal to an element in the haystack, bs() will return that element's index.
  • If the needle is not equal to an element in the haystack, bs() will return -(x+1), where x is where the element would have been if it was in the haystack. (Therefore if the element is less than all elements in the array, this would be -1; if the element is greater than all elements in the array, this would be -(haystack.length+1).)
    • Note that this means a negative return value indicates that the needle was not in the haystack, while a zero or positive return value indicates that the needle was in the haystack.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, that's terser. I think I want to break the latter half of that paragraph into bullet points.

This terser explanation omits explaining what happens if minimumIndex and maximumIndex are supplied… which makes it clearer.

I think this would be better if I changed it so that the return value explains everything in the following order:

  • first the easy case (needle found in haystack)
  • second, stating the general rule for when needle isn't in haystack, as you have here
  • then a bullet point for each case (needle before, needle between, needle after)
  • lastly, a completely separate paragraph (maybe even a completely separate section) explaining what happens if one specifies non-default values of minimumIndex and maximumIndex.

I'll come back and have another go at writing later. Thank you for the feedback! :)

Choose a reason for hiding this comment

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

If the needle is not equal to an element in the haystack, bs() will return -(x+1) where x is where the element would have been if it was in the haystack

This is correct, though I found it a bit confusing. Since x is negative, it's easier for me to understand it this way:

If the needle is not equal to an element in the haystack, bs() will return -x, where x-1 is the position where the element would have been if it were in the haystack.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants