patternrubyrailsMinor
Saving a recommended web article from form in Rails controller
Viewed 0 times
recommendedcontrollerrailswebformfromsavingarticle
Problem
I need criticisms and refactoring advice for my controller code:
Parameter example:
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
They all go into a
Here is an image that explains the visual results:
class ItemsController 1
collection = true
end
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
end
if not collection.save
return render :new
end
else
resource = RecommendedResource.new(title: params[:titles][0], url: params[:urls][0])
if not resource.save
return render :new
end
end
if not RecommendedItem.create(element: element)
return render :new
end
render :new
end
private
def titles_and_urls_are_same_length(params)
params[:titles].length == params[:urls].length
end
def items_params
params.permit(:description, titles: [], urls: [])
end
endParameter 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:
Solution
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:
This would give us routes like:
So the user would create a brand new collection from
and POST to
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
So from our Collection controller we can create both the collection and the recommendation
So now if we do
The form would look something like this:
Later when other users or the same user wants to add more recommendations to a collection they would post to
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
endThis 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#destroySo 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?
endSo 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.
end
def create
@collection = RecommendedCollection.create(collection_parameters)
if @collection.persisted?
redirect_to @collection
else
render :new, notice: 'Collection could not be created.'
end
end
def collection_parameters
params.require(:collection).allow(recommendation_attributes: [:title, :url])
end
endSo 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:
Later when other users or the same user wants to add more recommendations to a collection they would post to
/collections/1/recommendations.Code Snippets
resources :collections, shallow: true do
resources :recommendations
endPrefix 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#destroyclass RecommendedCollection < ActiveRecord::Base
has_many :recommendations, class_name: 'RecommendedResource'
accepts_nested_attributes_for :recommendations, reject_if: :all_blank?
endclass CollectionController < ApplicationController
def new
@collection = RecommendedCollection.new
@collection.recommendations.build # create an empty recommendation for the form.
end
def create
@collection = RecommendedCollection.create(collection_parameters)
if @collection.persisted?
redirect_to @collection
else
render :new, notice: 'Collection could not be created.'
end
end
def collection_parameters
params.require(:collection).allow(recommendation_attributes: [:title, :url])
end
end<%= form_for(@ollection) do |f| %>
<%= f.fields_for(:recommendations) do |nested_fields| %>
<%= nested_fields.text_field :url %>
<%= nested_fields.text_field :title %>
<% end %>
<%= end %>Context
StackExchange Code Review Q#96027, answer score: 3
Revisions (0)
No revisions yet.