-
Notifications
You must be signed in to change notification settings - Fork 25
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
Panda level finished #18
base: master
Are you sure you want to change the base?
Conversation
Sorry about missing this! I'm on it this morning. |
end | ||
|
||
def add(movie) | ||
if @movies.include?(movie) |
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.
Later we use this method (add) as:
puts "[#{movie.title}] is already on your list!" unless movie_list.add(movie)
This means this method is responsible for two things:
- keeping the movies unique (prevent dupes)
- telling you if the movie was a dupe
There are exceptions to this rule, but I think Command/Query separation is a good one to follow. More: http://martinfowler.com/bliki/CommandQuerySeparation.html & http://tony.pitluga.com/2011/08/04/command-query-separation-in-ruby.html
In this specific case, I might add a "contains?" method
if movie_list.contains? movie
puts "[#{movie.title}] is already on your list!"
else
movie_list.add movie
end
thoughts? what 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.
Thanks for the links about Command/Query separation!
The idea of MovieList#add
is actually derived from ActiveRecord::Base#save
. I want to use the return value (true/false) to let the user know if he adds the movie successfully or not.
After reading the links, I think that separating the two idea is a better and cleaner design. But inside the MovieList#add
, I still want to check if the argument is duplicated or not. Because I believe that MovieList#add
should have the responsibility to protect the internal data from messed up by the user.
When I try to implement the new method, how to handle bad argument comes into question. Should I just neglect it or throw an exception? Which one will you choose? Or, maybe you have a better idea? 😄
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.
sure, I can buy that -- if you want MovieList#add
to not:
- not add the item if it exists
- tell the program something bad happened if someone tried to add a dupe:
in MovieList#add
I would raise an exception if it is contained when you try to add it.
Most excellent job! especially with the tests! Go you! |
No description provided.