Melissa : Edges :media Ranker #39
Conversation
Media RankerWhat We're Looking For
This is a good start. You were able to get the site working well, including logging in, voting and sorting media by votes, which is is impressive! However, there is room for growth around working with models, both in terms of writing methods to handle business logic, and testing that logic to make sure it's correct. Those are both important learning goals for this project, and it's important to me that you feel comfortable with those techniques. Please read my inline comments around business logic and model tests, take the opportunity to practice them as you continue working on bEtsy, and let me know if you have any questions or want further guidance. I do want to affirm that the weekend this project was assigned was very busy, and that the application you've submitted matches the functionality of our demo site pretty well. Even if there's improvements to be made, there's also a lot of good work here, and you should be proud of what you've accomplished. |
| belongs_to :user | ||
| belongs_to :work | ||
|
|
||
| validates :user_id, uniqueness: { scope: :work_id, message: "only one vote"} |
There was a problem hiding this comment.
Nice work getting this tricky validation figured out!
| def homepage | ||
| works_albums = Work.left_joins(:votes).group(:id).order("category asc, count(votes.work_id) desc").where({ category: "album" }).limit(10) | ||
|
|
||
| works_books = Work.left_joins(:votes).group(:id).order("category asc, count(votes.work_id) desc").where({ category: "book" }).limit(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 would allow you to break the work across multiple lines to make it easier to follow. 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_by_votes = Work.left_joins(:votes).group(:id).order("category asc, count(votes.work_id) desc")
works_for_category = works_by_votes.where(category: category)
return works_for_category.first(10)
end
end|
|
||
| @top_work = work.max_by { |w| w.votes.count } | ||
|
|
||
| @works = work.group_by(&:category) |
There was a problem hiding this comment.
Finding the top work would be another excellent model method.
| def edit | ||
| if @work.nil? | ||
| head :not_found | ||
| end |
There was a problem hiding this comment.
You make this check many times in this controller. Why not add it to the find_work filter?
| </head> | ||
| <title>Media Ranker</title> | ||
|
|
||
| <section> |
There was a problem hiding this comment.
This file would benefit from some general cleanup. For example, this <title> should be inside the <head>, and this <section> should be inside the <body>. You have several other dangling or unclosed tags, and it looks like you started to apply high-level sectioning elements (<header>, <main>, etc) but didn't follow through.
In the past your view organization and use of semantic HTML has been strong - what happened?
| <% @works.each do |c, works| %> <!-- iterate over the works hash that was created in in index by group_by --> | ||
| <strong> Top </strong> | ||
| <strong><%= c.titleize.pluralize%></strong> | ||
|
|
There was a problem hiding this comment.
I like that you're looping over the categories here to avoid copy-pasting the same table code 3 times. Good work keeping things DRY.
| @@ -0,0 +1,9 @@ | |||
| class VotesController < ApplicationController | |||
|
|
|||
| # do we need a votes controller? - asks Dan | |||
There was a problem hiding this comment.
Nope. You should remove this file.
|
|
||
| resources :users | ||
| resources :votes | ||
| resources :works |
There was a problem hiding this comment.
There's a lot of extra routes defined in this file:
- I don't think you're using the full set of RESTful routes for users, only
:indexand:show - Since you don't have anything in your
VotesController, you don't needresources :votesat all
Make sure you're only generating the routes you need for your application.
| describe Vote do | ||
| let(:vote) { votes(:vote_one) } | ||
|
|
||
| it "must be valid" do |
There was a problem hiding this comment.
You should be testing the uniqueness constraint here! Specific test cases I'd want to see:
- A user can have votes for two different works
- A work can have votes from two different users
- A user cannot vote for the same work twice
|
|
||
| describe Work do | ||
|
|
||
|
|
There was a problem hiding this comment.
You didn't put your business logic in the model, so it makes sense that you wouldn't have tests for it. However, if you were to follow the comments above, then you might end up with test cases like these.
For finding the media spotlight, I would wonder:
- 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?
For finding the top ten works in a category, I would ask:
- 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?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?