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

MediaRanker by Becca #35

Open
wants to merge 21 commits into
base: RmT/master
Choose a base branch
from
Open

Conversation

rmtolmach
Copy link

I didn't worry about the difference in white space between mine and the sample. I didn't have any partials or anything fancy. I would like to have time to learn that stuff in the next applicable project.

@catchingash catchingash self-assigned this Jan 6, 2016
end

def show
@album = Album.find(params[:id])

Choose a reason for hiding this comment

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

Since you repeat @album = Album.find(params[:id]) on several actions, you can use a before_action to set this and DRY up your code.

<div class="col-lg-12">
<h3>New Album</h3>

<%= form_for @album, url: {action: "create"}, html: {class: "form-group"} do |f| %>

Choose a reason for hiding this comment

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

As you mentioned, this would be a great place to use a partial :)

@catchingash
Copy link

Nice job, @rmtolmach! In general, your code was clean and concise. As you mentioned, partials would've really helped to DRY up your code. It was good that you included tests for all of your controllers. There's still room for you to expand your tests -- confirming that #update actually updates the record in the database, adding tests for your model validations, etc. I only had minor comments on your code. Let me know if anything is unclear, or if you have any questions; I'd be more than happy to clarify :)

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