current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

In Rails 3, I have two models, Nutrient and Recommendation. Nutrient has_many Recommendation, Recommendation belongs_to Nutrient. I am trying to implement the create method of the recommendation in the controller. Is this the proper way to do it:

def create
  nutrient_id = params[:recommendation][:nutrient_id]
  if nutrient_id.blank?
    #nutrient_id was blank in the submit, get other recommendation params and re-render 'new'
    params[:recommendation].delete(:nutrient_id)
    @recommendation=Recommendation.new(params[:recommendation])
    render 'new'
  else
    @nutrient = Nutrient.find(nutrient_id)
    if @nutrient
      #nutrient was found by id, create recommendation
      @recommendation = @nutrient.recommendations.build(params[:recommendation])
      if @recommendation.save
        redirect_to @recommendation
      else
        render 'new'
      end
    else
      #nutrient was not found by id, get other recommendation params and re-render 'new'
      params[:recommendation].delete(:nutrient_id)
      @recommendation = Recommendation.new(params[:recommendation])
      render 'new'
    end
  end
end
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Your approach is fundamentally flawed, I'm afraid. You're not using Rails' abilities to your advantage at all.

For a simple create operation, your controller method rarely needs to be more than 10 lines or so. There's way too much going on in yours.

Start by nesting your routes:

 resources :nutrients do
   resources :recommendations
 end

Run rake routes to see how the routes now look. Basically, a recommendation will have a URL like /nutrient/:nutrient_id/recommendation/:id.

This means you'll have to make some changes to links in your views and your redirections. Where you could previously write recommendations_path and so on you'll now have to write nutrient_recommendations_path(@nutrient) and so on. Again, rake routes will show you what helpers you now have.

In your recommendation form, you'll have to do something like:

 <%= form_for [@nutrient, @recommendation] %>

so the form's action URL will be correct. An get rid of the hidden input that I assume you've added to the form as recommendation[nutrient_id].

Inside the RecommendationsController, you can always get the "parent" nutrient by saying

 @nutrient = Nutrient.find(params[:nutrient_id])

as that param is given in the URL.

In the end, your create method will look like

def create
  @nutrient = Nutrient.find(params[:nutrient_id])
  @recommendation = @nutrient.recommendations.new(params[:recommendation])

  if @recommendation.save
    redirect_to @recommendation, :notice => "Recommendation created."
  else
    render 'new'
  end
end

See something like this post for a more thorough diff-like walkthrough of the changes to make.

Note, that if Recommendation has further nested resources, you should use "shallow" routes, but that's left as an exercise to the reader.

share|improve this answer
    
Thank you so much flambino, I knew I was on the wrong path I just didn't know how to ask the right question. Thanks for laying it all out, you've saved me a huge headache and probably a lot of future miss-steps. –  Mike Apr 3 '13 at 21:43
    
@Mike No problem. When your code starts to smell (methods are too long, things are getting complicated, etc), just remember that in 99 out of 100 cases, you're trying to well-known problem. So always check if Rails can do it all already, or look for a gem that might do it. I highly recommend looking at Railscasts, The Ruby Toolbox, and of course Rails itself –  Flambino Apr 3 '13 at 22:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.