Rails Best Practices 3: Increase Controller’s readability
10:45 AMDevelopment, Metodologies, RubyClaudio
[POST UPDATE ON 19/07/2010 - 15:45]
Continuing our analysis of the Rails Best Practices today we’ll see two other tricks to make more readable method of the controller.
1. Methods within model
Suppose we have the classic user registry defined in such a way that an admin user can enable or disable other users.
The deactivate method inside user Controller may be defined as follows:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 | class UserController < ApplicationController def deactivate @user = User.find(params[ :id]) if @user.admin? flash[:error] = "You can not deactivate Admin user" else if @user.update_attributes(:status, "N") @user.modified_by = current_user @user.save else flash[:error] = "Error during user deactivation" end end redirect_to :action => 'index' end end |
What is clear is that this method has a lot of business logic inside. We see then how to define a method in the User Model to implement business logic so we can streamline this method.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 | class User < ActiveRecord:Base def deactivate(user) ret = false if self.update_attributes(:status, "N") self.modified_by = user self.save ret = true end ret end end class UserController < ApplicationController def deactivate @user = User.find(params[ :id]) if @user.admin? flash[:error] = "You can not deactivate Admin user" else if !@user.deactivate(current_user) flash[:error] = "Error during user deactivation" end redirect_to :action => 'index' end end end |
This rewrite makes the deactivate method within Controller more readable and more easily testable.
2. Factory Method
In a similar way to what has been done for the deactivate method, sometimes can be handy while writing a create method, to define a Factory Method in the model that contains the logic of creation / initialization of the object and then call it from the controller.
As usual, let’s see an example.
Imagine you are developing a new management system where a news must be associated with the user who created it and it must also be visible only for a week.
A solution, without the Factory Method is the following:
1 2 3 4 5 6 7 8 9 10 | class PostController < ApplicationController def create @post = Post.new(params[:post]) @post.user = current_user @post.expiring_date = Time.now + 7.day @post.save end end |
Let us now see how to define a proper method (the Factory Method) of creating the news in the model so, once again, we simplify the controller
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 | class Post < ActiveRecord:Base def publish_new_post(params, user) post = Post.new(params) post.user = user post.expiring_date = Time.now + 7.day post.save end end class PostController < ApplicationController def create @post = Post.publish_new_post(params[:post], current_user) end end |
Once again we managed to write a better code in the Controller by moving the logic in the Model.
This post concludes the first part of this series when I tried to illustrate some best practices that allow to simplify the controller and move the business logic in the Model.
From the next post I will start to illustrate some Best Practices applicable to the Models.
Tags: Best Practices, controller, factory method, ruby on rails
About DevInterface
We are an information and communication technology agency. Our mission is to provide web application development, design services and communication strategies. We specialize in building web applications with modern and efficient frameworks.
Related Post
14 Responses to “Rails Best Practices 3: Increase Controller’s readability”
Leave a Reply
Insert code beetween <code lang="ruby"> and </code>
-
our LINKS
In first example in corrected users controller end is missing.:)
Wouldn’t it be better if we go furhter and did in controller:
def deactivate
@user = User.find(params[ :id])
if @user.deactivate
redirect_to :action => ‘index’
else
flash[:error] = “Error during user deactivation”
end
end
And moved admin verification to model, and maybe add validation and display errror about admin on on form. Or just skip that admin info.
The first example looks still pretty weird to me even after refactoring.
* You set modified_by after you have saved the user model. This way this won’t get saved in the database.
* The current_user method (usually) is not available in the User model, just like the params method for accessing the request parameters.
* What is the params[:status] for anyway?
* Why not do the admin check in the model as well using validations?
class User ['N'], :if => :admin?, :message => “An admin account cannot get deactivated.”
end
You might also want to take a look at the association proxies that activerecord provides on associations and how you can use them.
The common practice is to do something like;
current_user.posts.build(params[:post])
Instead of:
post = Post.new(params[:post])
post.user = current_user
Factory method (publish_new_post) seems to already save the model to db, so why call save() on it again in the controller?
Your User#deactivate method is completely wrong. Have you even tested it?
Hello everyone, thank you for corrections. Accidentally I had released a “draft” of the article.
Now I have introduced some corrections, but if you find other bugs or inaccuracies, feel free to write a comment.
As I said in my introduction “write these posts will also be an opportunity for me to investigate my knowledge” , so I like to recive any contribute that help me (and all my readers) to write better code.
In Post#publish_new_post you reference Self, which does not exist. You also created the method as an instance method rather than a class method.
if !@user.deactivate(current_user)
ಠ_ಠ
Your post has been linked at the Drink Rails blog as one of the top Ruby on Rails related blogs of the day.
Thank you Sohan for the link
We use a rails plugin to accomplish CRUD operations on controllers called resource_full (http://github.com/vraravam/resource_full) – since most of the CRUD operations on the controller are similar.
2
3
4
5
6
7
8
9
10
11
12
13
14
15
before_create :set_expiration_date
private
def set_expiration_date
self.expiring_date = Time.now + 7.day
end
end
class PostController < ApplicationController
include ResourceFull
def create_model_object
model_class.create(model_params.merge(:user => current_user))
end
end
At first glance, the hooks that resource_full provides can be intimidating. But if you consider writing similar plumbing code for all controllers, we find this a much better alternative.
In my previous post, I am assuming that the ‘current_user’ is a utility method that is accessible at the controller level (similar to your example).
I think you wanted to do your factory method a class method:
class Post < ActiveRecord:Base
def self.publish_new_post(params, user)
Post.build(params).tap do |post|
post.user = user
post.expiring_date = Time.now + 7.day
end
end
end
end
That will return an unsaved Post, you'll probably want to handle the save on the controller to be able to do something if the save fails
[...] Maral has a couple of examples of cleaning up controller code by moving logic to the [...]