-
Notifications
You must be signed in to change notification settings - Fork 5
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
improve Idol#find_by_name #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your pr! It's a good idea for extend method. I have few advices.
case idol_name | ||
when Symbol | ||
all_idols.find { |idol| idol.key == idol_name } || raise("unknown idol: #{idol_name}") | ||
when String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the code below? I think we can decrease code volume.
all_idols.find do |idol|
[idol.key, idol.name.given, idol.name.aka, idol.name.full].include?(idol_name) || raise("unknown idol: #{idol_name}")
end
or we can extend Idol::Name#==
method like below
def ==(other)
super || [aka, family, full].compact.include?(other)
end
and can write find_by_name
method like this.
all_idols.find { |idol| [idol.key, idol.name].include? idol_name } || raise("unknown idol: #{idol_name}")
How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second idea is better. I'll try it.
context 'can find idol' do | ||
where(:name, :expected) do | ||
[ | ||
[ "未来", :mirai], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove space: [ "
-> ["
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for pointing out. I'll fix it.
4b1d42c
to
404c22e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
example: