-
Notifications
You must be signed in to change notification settings - Fork 2
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
Avoid calling count on Enumerable #3
Avoid calling count on Enumerable #3
Conversation
91a9ced
to
7f8faa8
Compare
Is it easy to still include zero or more than one in the error message? |
@mikegee Its somewhat easy. I've been going back and forth between two implementations, let me know what you think is better (or if they are both bad): Option 1: require "first_and_only/version"
module Enumerable
def first_and_only!
first_two_count = first(2).count
fail(FirstAndOnly::CountNotOne.new(first_two_count)) if first_two_count != 1
first
end
module FirstAndOnly
class CountNotOne < StandardError
COUNT_ZERO_ERROR_MESSAGE = 'Expected the count to be 1, was 0.'.freeze
COUNT_GREATER_THAN_ONE_ERROR_MESSAGE = 'Expected the count to be 1, was at least 2.'.freeze
def initialize(first_two_count)
if first_two_count == 0
message = COUNT_ZERO_ERROR_MESSAGE
else
message = COUNT_GREATER_THAN_ONE_ERROR_MESSAGE
end
super(message)
end
end
end
end Option 2: require "first_and_only/version"
module Enumerable
def first_and_only!
case first(2).count
when 0
fail FirstAndOnly::CountNotOne, FirstAndOnly::COUNT_ZERO_ERROR_MESSAGE
when 1
first
else
fail FirstAndOnly::CountNotOne, FirstAndOnly::COUNT_GREATER_THAN_ONE_ERROR_MESSAGE
end
end
module FirstAndOnly
CountNotOne = Class.new(StandardError)
COUNT_ZERO_ERROR_MESSAGE = 'Expected the count to be 1, was 0.'.freeze
COUNT_GREATER_THAN_ONE_ERROR_MESSAGE = 'Expected the count to be 1, was at least 2.'.freeze
end
end As i was writing these out I thought of a third implementation where there are two separate error classes, one for the zero count case and one for the greater than one case. What do you think? |
I have an allergic reaction to case statements, but I must admit that option seems better here. Splitting the two errors into their own classes is probably an improvement too. |
1b15fea
to
0cb32b5
Compare
@mikegee I agree, I usually dislike case statements, but I think in this case it worked well. I split out the error into two classes, but I made the two specific classes inherit from the What do you think? |
48fe2fe
to
dfd2e95
Compare
As much as I would love to continue to engineer the hell out of this miniscule detail of a gem basically nobody uses, let's ship it. Thanks Derek! |
@schneiderderek I've added you to the project! Please merge you change, and then merge a version bump and tag. |
@mikegee we talked about this awhile back so I thought I would put something together.
There could be
Enumerable
objects where it takes a very long time in order to get a count.first_and_only
should avoid calling count especially sincefirst_and_only
only needs to know whether there are 0, 1, or 2 objects to do its job.@dapplebeforedawn What do you think?