Naheed MediaRanker Edges #48
Conversation
Media RankerWhat We're Looking For
Good work overall. I was right on the border between yellow and green on this one. Some of the wave 3 functionality is incomplete, but in general this site functions well, and it is clear to me that the learning goals for this assignment were met, especially around working with complex business logic. I don't have much insight into this, but probably the place where there's the most room for growth is in writing code quickly and efficiently. Part of this is just practice, but if you'd like to discuss tools for working on this I'd be happy to. There is also some room for improvement around testing, but you're on the right track here. As you work on bEtsy, it might be worthwhile to seek out an instructor or TA to get feedback on test coverage before the deadline. There are a few places where things could be cleaned up, which I've tried to outline below, but in general I am pretty happy with this submission. Keep up the hard work! |
| else | ||
| @user = User.new(username: name) | ||
| @user.save | ||
| redirect_to root_path |
There was a problem hiding this comment.
When you create a new user you're not saving their ID in the session. That means that a new user has to enter their name twice to log in: once to create the record, and once to set their user ID in the session.
| if save_success | ||
| redirect_to works_path | ||
| else | ||
| render :new |
There was a problem hiding this comment.
It would be good to put a message in the flash in both these cases, either "successfully created work" or "could not create work".
| def self.voted_works(user_id) | ||
| works = User.where(id: user_id) | ||
| return works | ||
| end |
There was a problem hiding this comment.
This method returns a list of (one) user, not a list of works. Why not use a has_many :through relationship here?
| belongs_to :user | ||
| belongs_to :work | ||
| validates :work, uniqueness: { scope: :user } | ||
| validates :user, uniqueness: { scope: :work } |
There was a problem hiding this comment.
Good work getting this tricky uniqueness scope figured out! However, I think you only need one or the other of these lines, not both.
| def self.list_top_ten(category) | ||
| works = Work.sort_works(category) | ||
| return works[0..9] | ||
| end |
There was a problem hiding this comment.
I love the way these model methods build on each other, each adding a small bit of functionality. This is a great example of functional decomposition.
| <h2 class="top-ten__header"> | ||
| Top Movies | ||
| </h2> | ||
| <ul class="list-group top-ten__list"> |
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?
| post '/sessions/logout', to: 'sessions#logout', as: 'logout' | ||
|
|
||
| resources :users | ||
|
|
There was a problem hiding this comment.
I don't think you need all 7 RESTful routes for users. This site should only use index and show.
| describe Vote do | ||
| let(:vote) { Vote.new } | ||
|
|
||
| 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 "sort_works" do | ||
| it "sorts works based on votes" do | ||
| sorted = Work.sort_works("album") | ||
| first = sorted.first.votes.count |
There was a problem hiding this comment.
This is a good start to testing for your custom model methods, but there's some interesting edge cases you haven't covered.
For find_top_work, 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?
Similarly for your sort_works and top_ten methods, 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?