Skip to content
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

Hayden - Edges / C10 - Bro Ranker #28

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

haydenwalls
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. The .spotlight self-method returns the highest voted medium.
Describe how you approached testing that model method. What edge cases did you come up with? I realize now that I did not test any edge cases for that method. If none of my model instances have any votes however, it doesn't really matter which instance gets displayed as the spotlight. Likewise, if all medium instances were deleted it would crash the site but I avoided that by hiding the media spotlight code within a conditional.
What are session and flash? What is the difference between them? Rails-provided hashlike objects. Session persists across a user's entire browsing session, flash only persists for one request-response cycle.
Describe a controller filter you wrote. I pulled the medium find_by method calls out into a before_action
What was one thing that you gained more clarity on through this assignment? I feel somewhat more comfortable just generally configuring rails backend stuff, which is good because I had felt uncomfortable with it previously.
What is the Heroku URL of your deployed application? https://bro-ranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? Not really sorry

… enable media deletion, add routes for PagesController
@tildeee
Copy link

tildeee commented Oct 26, 2018

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Rails fundamentals (RESTful routing, use of named paths) x
Views are well-organized (DRY, use of semantic HTML, use of partials) x
Errors are reported to the user x
Business logic lives in the models x
Models are thoroughly tested, including relations, validations and any custom logic yes! thorough and well-done
Wave 1 - Media
Splash page shows the three media categories x
Basic CRUD operations on media are present and functional x
Wave 2 - Users and Votes
Users can log in and log out x
The ID of the current user is stored in the session x
A user cannot vote for the same media more than once x
All media lists are ordered by vote count x
Splash page contains a media spotlight x
Wave 3 - Users and Votes
Media pages contain lists of voting users x
Individual user pages and the user list are present x
Optional - Styling
Bootstrap is used appropriately x
Look and feel is similar to the original x
Overall

Nicely done, Hayden! The code looks great, efficient, well-organized, and the site looks good too.

My comments are mostly small suggestions and questions.

I'm really impressed by how thorough and readable your tests are! They're great.

Overall, good work!

return sorted.reverse!
end

def parse_bro_category(category)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of view helpers!

self.all.max_by do |medium|
medium.votes.length
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! This is a great implementation of spotlight.

<section class="media-table">
<h4>Visual Bros</h4>
<%= render partial: "media_table", locals: { collection: order_by_votes(@movies) } %>
</section>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This view is already very clean code, as you made a good choice to pull out the media_table, but you could definitely find a way to push this so it's even shorter and cleaner ;)

<ul class="list-group top-ten__list">
<%= render partial: 'medium', collection: first_ten_by_votes(@albums) %>
</ul>
</section>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar comment for another bit of view code, but there's definitely another way to reduce this code... Just a suggestion though.

<article>
<h4>Bro, lemme be honest w you...</h4>
<p>
We literally have no idea how the internet works. We scraped most of the code for this app from StackOverflow posts, and I have actually used the same password for every single website I've logged into since 1998. Besides, the government doesn't even need to brute force your hashes, they INVENTED the encryption algorithms. You really think they wouldn't put a backdoor in there? Tor was invented by the Navy, lol.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 🔥

category: 'book',
creator: 'Jarth Dengees',
publication_year: 1999
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use an existing medium saved in the fixtures? (I definitely think that this is very readable though, so I won't fight for this very terribly, but the fixtures are definitely there already.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants