Binary search should return index of target value #15

wants to merge 2 commits into

3 participants


In response to open issue #13.


@mhriess you should change the comments above the method to reflect your implementation. You can always cite wikipedia for evidence :).


Can you add some tests for that? Also, I'd be reluctant to merge an API change that isn't backwards compatible. What about creating a new method?


Happy to write some tests ASAP. Regarding the compatibility issue- technically a binary search method should return the position of a specified value (the input "key") if the value is present in the container. Since the method requires a target value as a parameter, I think it's safe to assume that the user already knows the value of their target, and thus it doesn't make sense to me to return the value/target as the product of the search. Thoughts?



class Tuple
  include Comparable

  attr_accessor :key, :value

  def <=>(rhs)
    key <=> rhs.key

  def initialize(key, value = nil); …; end


You could use this Tuple class or a similar structure to essentially implement a binary lookup table.

That said, I totally agree that retrieving the index is much more useful. I just would rather not break any extant callers of the method.


Again, I totally understand what you're saying about the backwards compatibility. The last point I'll make for my case is that the method, although already established in previous versions, is returning a value that users aren't expecting. It seems to me that from a project management perspective, it'd be better to change the method so it returns what people expect it to return by definition so future users of the code aren't caught off guard when the method returns a result that it shouldn't, and current users can adjust accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment