MediaRanker - Semret Nicodimos - Edges #24
Conversation
…at were not necessary
…_helper.rb, wrote one test for model for work
Media RankerWhat We're Looking For
Great job overall! Your implementation matches the demo site very closely, and I would say the learning goals for this assignment were definitely met. The one place where I see room for improvement is around working with custom model methods - you've made a good start here, but you should keep practicing this as we go forward. As with any project of this size there are a few places where things could be cleaned up, which I've tried to outline below, but in general I am quite happy with this submission. Keep up the hard work! |
| def update | ||
| work = Work.find_by(id: params[:id]) | ||
|
|
||
| if @work.update(work_params) |
There was a problem hiding this comment.
Here you store the work in work, but call .update on @work. This is probably why updating fails.
| # for top 10 movies | ||
| @movies = Work.where(category: "movie") | ||
| # sort by votes and take only 10 movies | ||
| @movies = (@movies.sort_by { |movie| -movie.votes_count}).take(10) |
There was a problem hiding this comment.
These long lines of database work would be great as model methods. That would help DRY out this code, and having it in a separate method would make the functionality simpler to test as well. Something like this:
class Work
# ...
def self.top_ten(category)
works_for_category = Work.where(category: category)
sorted_works = works_for_category.sort_by { |movie| -movie.votes_count}
return sorted_works.first(10)
end
end| validates :user_id, presence: true | ||
| validates :work_id, presence: true | ||
|
|
||
| validates_uniqueness_of :work_id, scope: :user_id |
There was a problem hiding this comment.
Good work getting this tricky uniqueness scope figured out!
| validates :title, :category, presence: true | ||
|
|
||
| validates_uniqueness_of :title, scope: :category | ||
| scope :featured, -> {order('votes_count DESC')} |
There was a problem hiding this comment.
I like the idea of using a scope along with the counter cache here to sort the votes.
First, featured might not be the best name for this, since it returns the entire list of works sorted by vote count. Something like by_vote_count might be a better name.
Second, you should be using this scope in your controller to get the top ten:
@movies = Work.where(category: 'album').featured.first(10)This is the big benefit of defining functionality of a scope: you can invoke it not just on the Work class, but on any collection of Works you got from the database (like from .where).
| <tr> | ||
| <%@works.each do |work| %> | ||
| <%if work.category == "album"%> | ||
| <td scope="row"><%=work.votes.count%></td> |
There was a problem hiding this comment.
You have the same code to show a list of works repeated 3 times. Could you use a view partial or a loop to DRY this up?
| end | ||
|
|
||
| resources :users | ||
|
|
There was a problem hiding this comment.
I don't think you need all 7 restful routes for users - only index and show are needed for this project.
| end | ||
| describe 'Validations' do | ||
| it 'valid when user and work ids are present' do | ||
| is_valid = @vote.valid? |
There was a problem hiding this comment.
Since this validation is complex, there are a couple other positive test cases I would like to see:
- A user can have votes for two different works
- A work can have votes from two different users
|
|
||
| describe Work do | ||
| # describe 'relations' do | ||
| # it 'a vote can be set through the method work' do |
There was a problem hiding this comment.
It's difficult to write model tests if you didn't write model methods, but you should at least have tests for the featured scope here.
If you were to write a method to find the top ten works, I would look for the following cases:
- What if there are no works of that category?
- What if there are less than 10 works?
- What if there's a tie for last place, e.g. works 9, 10 and 11 all have 0 votes?
Similarly, if you had a model method for finding the spotlight, I might ask:
- What happens if there are no works?
- What happens if there are works but no votes?
- What happens if two works have the same number of votes?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?