I need criticisms and refactoring advice for my controller code:

class ItemsController < ApplicationController

    def new


    def create
        params = items_params
        if not titles_and_urls_are_same_length(params)
            render :new

        collection = false
        if params[:titles].length > 1
            collection = true

        element = nil
        if collection
            collection = RecommendedCollection.new
            params[:titles].each_with_index do |title, index|
                resource = RecommendedResource.new(title: title, url: params[:urls][index])
                collection.resources << resource
            if not collection.save
                return render :new
            resource = RecommendedResource.new(title: params[:titles][0], url: params[:urls][0])
            if not resource.save
                return render :new

        if not RecommendedItem.create(element: element)
            return render :new
        render :new


        def titles_and_urls_are_same_length(params)
            params[:titles].length == params[:urls].length

        def items_params
            params.permit(:description, titles: [], urls: [])

Parameter example:

Parameters: {"utf8"=>"✓", "authenticity_token"=>"......", "titles"=>["Learning to program in ruby", "Refactoring Code blog"], "urls"=>["http://www.hi.com", "http://www.ruby-master.com"], "description"=>"Improve your ruby skills", "commit"=>"Recommend"}

The idea behind the controller is that the user submits a link along with a URL and description to a site they like. If you submit a single link, then it is just a RecommendedResource, if you submit multiple links then it is to be turned into a "collection" or RecommendedCollection. That is why I check for the length of the params (which is probably not a good way to write the code).

They all go into a RecommendedItem to show in the feed, so I loop through one model only.

Here is an image that explains the visual results:

enter image description here

  • \$\begingroup\$ Your original title was actually more ideal. We prefer such titles, not ones that specify a review request. \$\endgroup\$
    – Jamal
    Jul 8, 2015 at 2:16

1 Answer 1


Your controller is very creative but its got some pretty big problems. In Rails controllers are notoriously hard to test and can quickly become very buggy if they contain too much logic. Skinny controllers are a vital part of good MVC code.

Any time your controller action has more than two possible code branches than you should stop and rethink it.

The Rails way of approaching this problem is nested routes and nested parameters.

Lets start out with the nested routes:

resources :collections, shallow: true do
  resources :recommendations 

This would give us routes like:

                      Prefix Verb   URI Pattern                                               Controller#Action
   collection_recommendations GET    /collections/:collection_id/recommendations(.:format)     recommendations#index
                              POST   /collections/:collection_id/recommendations(.:format)     recommendations#create
new_collection_recommendation GET    /collections/:collection_id/recommendations/new(.:format) recommendations#new
          edit_recommendation GET    /recommendations/:id/edit(.:format)                       recommendations#edit
               recommendation GET    /recommendations/:id(.:format)                            recommendations#show
                              PATCH  /recommendations/:id(.:format)                            recommendations#update
                              PUT    /recommendations/:id(.:format)                            recommendations#update
                              DELETE /recommendations/:id(.:format)                            recommendations#destroy
                  collections GET    /collections(.:format)                                    collections#index
                              POST   /collections(.:format)                                    collections#create
               new_collection GET    /collections/new(.:format)                                collections#new
              edit_collection GET    /collections/:id/edit(.:format)                           collections#edit
                   collection GET    /collections/:id(.:format)                                collections#show
                              PATCH  /collections/:id(.:format)                                collections#update
                              PUT    /collections/:id(.:format)                                collections#update
                              DELETE /collections/:id(.:format)                                collections#destroy

So the user would create a brand new collection from /collections/new - and POST to /collections.

but we want the user to be able to create a recommendation on the fly as well. So lets make so that we can pass recommendations into the collection.

So what we do is add accepts_nested_attributes_for to RecommendedCollection:

class RecommendedCollection < ActiveRecord::Base
  has_many :recommendations, class_name: 'RecommendedResource'
  accepts_nested_attributes_for :recommendations, reject_if: :all_blank?

So from our Collection controller we can create both the collection and the recommendation

class CollectionController < ApplicationController

  def new
    @collection = RecommendedCollection.new
    @collection.recommendations.build # create an empty recommendation for the form.

  def create
    @collection = RecommendedCollection.create(collection_parameters)
    if @collection.persisted?
      redirect_to @collection
      render :new, notice: 'Collection could not be created.'

  def collection_parameters
    params.require(:collection).allow(recommendation_attributes: [:title, :url])

So now if we do POST /collection collection: { recommendation_attributes: ['Test', 'http://www.example.com'] } we create a collection and a recommendation.

The form would look something like this:

<%= form_for(@ollection) do |f| %>
   <%= f.fields_for(:recommendations) do |nested_fields| %>
     <%= nested_fields.text_field :url %> 
     <%= nested_fields.text_field :title %> 
   <% end %>
<%= end %>

Later when other users or the same user wants to add more recommendations to a collection they would post to /collections/1/recommendations.

  • \$\begingroup\$ This is exactly what I was looking for thank you! But one question, is Collection a new class or is it a RecommendedCollection? \$\endgroup\$
    – Wenqin Ye
    Jul 8, 2015 at 2:16
  • \$\begingroup\$ Its a RecommendedCollection, just a misstake on my part. \$\endgroup\$
    – papirtiger
    Jul 8, 2015 at 2:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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