Binary search should return index of target value

[mhriess]

In response to open issue #13.

[tannerwelsh]

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

[loganb]

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?

[mhriess]

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?

[loganb]

Consider:

class Tuple
  include Comparable

  attr_accessor :key, :value

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

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

Search.binarySearch(array, Tuple.new('key'))

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.

[mhriess]

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.