Ports - Heather #27
Ports - Heather #27
Conversation
…. View pages for works.
… in application view
Media RankerWhat We're Looking For
Well done on this, Heather! This project was huge and your submission is great. Your controllers and controller tests are great! I have no complaints on those! I'm really happy with the model code, the model tests, and all of the moving parts, too. Great job on hitting the learning goals. All of my comments are really about the following things:
I actually figured out what was going on!
Also, overall, your indentation always got a little weird in the view code (see the That being said, great submission. Well done! |
| @@ -0,0 +1,7 @@ | |||
| class HomepagesController < ApplicationController | |||
| def index | |||
| def index | |||
There was a problem hiding this comment.
Hm?! Why is an index method defined inside of the index method?! Actually, this is what produced the really interesting bug!!
| has_many :votes, dependent: :destroy | ||
|
|
||
| def self.top_ten(type) | ||
| array_by_type = self.where(category: type) |
There was a problem hiding this comment.
I think these variables names could be better. If it were me, I would rename array_by_type to simply works or categorized_works, and array_by_votes to sorted_works
| return array_by_votes[0..9] | ||
| end | ||
| else | ||
| return array_by_type |
There was a problem hiding this comment.
Hm... array_by_type (since it comes from a where call) will always be an array... even an empty array. If that's the case, do we need the if ... else here? Consider the following refactor of the method:
def self.top_ten(type)
array_by_type = self.where(category: type)
array_by_votes = array_by_type.sort_by { |work| -work.votes.count }
return array_by_votes[0..9]
endThe tests still pass! When you are ready, take some time to think about why this still works and is effectively the same thing as your current code
| spotlight = sorted_works.first | ||
|
|
||
| return spotlight | ||
| end |
There was a problem hiding this comment.
I like this code! It's concise and clean. How would you feel if you did the following refactor:
def self.media_spotlight
sorted_works = self.all.sort_by { |work| -work.votes.count }
return sorted_works.first
end
| <%= link_to "Media Ranker", homepages_path %> | ||
| <small>Ranking the Best of Everything</small> | ||
| </h1> | ||
| <% if flash.count > 0 %> |
There was a problem hiding this comment.
It probably would be way better to insert flash outside of the header tag to match the model website
| <%= render partial: "indextable", locals: { | ||
| header_category: "Albums:", | ||
| category: "album" | ||
| } %> |
| Rails.application.routes.draw do | ||
| root to: "homepages#index" | ||
|
|
||
| get "homepages/index" |
There was a problem hiding this comment.
Since you did resoures :homepages, only: [:index] below, you don't need this route. Also, it doesn't direct anyone anywhere! It's not quite the right syntax
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?