-
Notifications
You must be signed in to change notification settings - Fork 43
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
Refactor find_answer
#686
Refactor find_answer
#686
Conversation
/// * `question`: question to compare with. | ||
pub fn responds(&self, question: &GenericQuestion) -> bool { | ||
if let Some(class) = &self.class { | ||
if !question.class.eq(class) { |
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.
Alternatively, we can write:
if !question.class.eq(class) { | |
if question.class != *class { |
It looks more natural to me, wdyt?
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.
in that case, also do it for question.text
below
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.
Yes, an even for e_val.eq(value)
below.
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.
Makes sense, thanks!
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.
LGTM
Problem
While checking the answers module, I discovered we were using a labeled for. That's perfectly fine, but coming from Ruby I felt that we could use a higher-level approach.
I took it as a refactoring exercise given that the original implementation was OK.
Solution
for
). Moreover, I think the logic belongs toQuestion
orAnswer
.for
structs with calls tofind
andall
, as they express the intention better.GenericQuestion
to avoid spelling the whole module name (agama_lib::questions::GenericQuestion
vsGenericQuestion
). If tomorrow we decide to change theGenericQuestion
to a different place, we need to update all the references. Not to mention that the lines are rather long and harder to read.Testing