All the time we have been doing programming we have always thougtht of building a system which will increase the performance of the system. Most of us think that by doing caching and adding more ram will increase the performance of the system. However in reality writing good code will also increase the performance of the system... In this blog I shall pen down our experience of how we cleaned much of our code to ensure our code is readable and increases the best performance.
Scenario:
We are currently working on a Large traffic website which already has a traffic of 15 M User Usage and about 35K-40K concurrent users. Right from the get go we need to handle large traffic.
Tool Used:
We started with Rails Best Practices (www.rails-bestpractices) tool to identify how good/bad our code is. It can be installed as a gem. The following are the methods
Add the following code into your gem file
gem 'rails_best_practices'
Run the following code in your console
> bundle install # installs your gem
> rails_best_practices g #creates a configuration file which you can use to evaluate your scenarios. By default there are 28 scenarios and you can switch on/off ones you dont need or you want to ignore
> rails_best_practices . # Rails best practices will run a tests on all the files within the root folder and outputs the analysis on your console. Alternatively you are provided with many more options to execute rails_best_practices.
Analysis of Rails Best Practices
Our analysis looked very good, we had written about few KLOCs with about 19 models, 15 controllers and the number of suggestions we found were about 355 with 4 developers working on the code. Since we know there are 28 best practices, the suggestions fell into 21 of these best practices. which means our code was good for the rest 7 best practices. We have grouped the best practices into the following
Controllers based best practices
- Move Finder to Named Scope
- Use Scope Access
- Replace Complex Creation With Factory method
- Move Model Logic to Model
- Law of Demeter
- Use Model Association
- Add Model Virtual Attribute
- Use Before Filter
- Simplify Render In Controllers
Views based best practices
- Replace Instance Variable With Local Vairable
- Move Code into Controller
- Move Code into Model
- Move Code into Helper
- Use Query Attribute
- Simplify Render in Views
Other best practices
- Remove Empty Helpers
- Use Multipart/ Alternative As Content Type of Email
- Remove Tab
- Remove Trailing Whitespace
- Use Say with Time in Migrations
- Always Add DB Index check
Looking at the errors and evaluating each criteria in the code, we realized we can ignore some of them because of understanding the code better or if they didnot pose any problem for improving the performance of the system and were used only for code cleaning purposes. the following are the detailed explaination of what was done and how you might want to look at it as well.
It becomes more clean to move the finders to a named scope. It makes sense to push most of the code from controllers to models as loading simple and small controllers are always good for the performance of the system. We had few areas where we created few named scopes to models. some of the changes we did were
1.
comment = Comment.find(:conditions=>["article_id = ?",article_id])
can be changed to
comment = Comment.where(:article_id=>article_id)
This is necessary that you start using AREL objects because, post Rails versions 3.2, all finders in rails will move to a plugin and not in the framework, which will degrade your performance
2.
comment = Comment.where(:published=>true)
can be changed to
comment = Comment.published
& in models
named_scope :published, :published=>true
Scope access is best used when you know the object is available and you can get the information from the object rather than using Finders. We modified our code to use Scope access for most of the objects created for the current user. For example.
comment = Comment.new(params[:comment])
comment.user = current_user
comment.save
can be changed to
comment = current_user.comments.create(params[:comment])
So by using Scope access, our number of calls get reduced and all are execute
3. Replace Complex Creation with Factory Method
This suggestion actually turned much of our coding perspective. Our earlier mechanisms were different in evaluating a code creation and helped us to move to a new way of thinking to create objects. However with respect to the evaluation, we were not very much comfortable in moving all the code with just 2 assignments counts to the model. Hence we first modified the Rails Best Practices YML file to set the attribute_assignments_count to 4 and ran the tests
So where all our code had more than 4 assigments, we moved into the model to a separate method. For example
#controller
def create
comment = Comment.new(params[:comment])
comment.user = user
comment.special = user.special?
comment.published = user.banned?
comment.save
end
can be changed to
# in model
def self.create_comment(params,user)
comment = self.new(params)
comment.user = user
comment.special = user.special?
comment.published = user.banned?
end
#in controllers
def create
comment = Comment.create_comment(params,current_user)
comment.save
end
It is always a good practice to have your controllers clean and simple. Most of your code should be in your models rather than controllers, why? Because controller gets loaded at all times and a model only when required. So you don’t use up more memory.
So it is very important that you keep most of your code in the model. Some of our examples were
# in controller
def publish_article
article = article.find(params[:id])
article.published = true
article.approver = current_user
article.save
end
can be changed to
# in model
def publish_article(user)
self.published = true
self.approver= user
self.save
end
# in controller
def publish_article
article = Article.find(params[:id])
article.publish_article(curren_user)
end
So it makes sense to have most of the logic code on the models rather than controllers
This is a simple and clean way of writing code but doesnot give any boost for the performance of the system. Law of demeter says that a model should talk only to it’s association and not to other associations… for e.g.
If we have an association where you have comment belongs to article then most of the times we try to use a code like
Article title = <% comment.article.title %>
Which actually under the law of demeter is a bad code. This can be eliminated by use of delegates where you add the following code to your model
#in controller model
delegate :title :to => :article, :prefix => true, :allow_nil =>true
and you can use
Article title = <%= comment.article_title %>
Which is much cleaner. It will also help you to evaluate if the article is nil or not and doesnot raise any error.
Law of Demeter holds good in both Controllers and Models
This is one of the standard mistakes that most of the developers do. Use the associated objects as part of settings. I strongly believe we should eliminate use of setting objects. For example
# in controller
def create
comment = Comment.new(params[:comment])
comment.article = @article
comment.save
end
Can be changed to
#in controller
def create
comment = @article.comments.create(params[:comment])
end
Saves you 3 lines of code.
7. Add Model Virtual Attribute
Most of the time when in controllers we try to overuse the controllers to do all activities from setting the variables to complex calculations. The best practices suggests to keep all settings to the model. In this section we identify that using Model attributes we can eliminate unnecessary calls on the server. Some of the examples are
#in controller
def create
@article = Article.new(params[:article])
@article.domain = params[:article][:url].split(‘/’).first
@article.relative_path = param[:article][:url].split(‘/’).last
@article.save
end
Can be changed to
#in controller
def create
@article = Article.create(params[:article])
end
#in model
def url=(value)
domain = value.split(“/”).first
relative_path = value.split(“/”).last
end
With the scenario the controllers is made more cleaner and most activities being moved to the models.
Before Filters are good to use to eliminate multiple calls of similar code. It is always good practice to group multiple similar codes into single routine and call it from the code. In our case rails best practices suggested us to use before filter for about 5 controllers. However looking at the scenarios we igored 4 suggestions as they were suggestions only for single code refactoring, But in case of the 5th controller, we had about 4 lines of code being called in 3 actions and hence moved the complete code into before filter.
Rails Best Practices provides an option to configure how many lines of repetitive code should be suggested. We configured the customize_count: 4 which we feel is a good number to be refactored rather than refactoring less than the numbers.
9. Simplify Renders in Controllers
Renders have been earlier written with more complex and connections to tell the users how they have to be used. Now with Rails 3.x, rails have become more cleaner and easier to use. Some of the examples are
1.
render :action=>”test_action”
becomes
render “test_action”
2.
render :file=>”/path/to/some/file”
becomes
render ‘/path/to/some/file’
In our code we found lots of places where we kept writing render :action at most of the places. However since they were at large, we went ahead to ignore them at this point of time, however we would come back and clean this off. Reason is that it doesnot affect the performance and is concentrated mainly on performance of the system.
10. Replace Instance Variable with Local Variable
Most of the places just because we have access to the @variable in our views, as a practice some tend to directly use these elements right into all parts of the code irrespective of if the code is in the action view file or in a partial file. The best practices is to eliminate use of these variable and convert them into local variable.
For this renders provide options to send local variable as part of :locals has which can be used to transmit variables from view to the partial.
In our code base we had 2 places where we had missed this scenario and rectified the same.
It is always believed that views should have least possible ruby code and more of HTML and related front end code. So its always best practice to move all the ruby code to helpers by creating necessary methods and calling the methods in the view
Moving code from views to helpers is good, however if the ruby code has anything to do with calculation or finders, then it is better to move that code to controllers and utilize the end variable as a instance variable within Controller. This is a good practice
This best practice in rails is to evaluate controller code / views code to keep minimal and move more into the model. However as suggested, where ever there is more code which is used by the same element can be grouped and moved to the model.
For example we had a multiple check scenario in the view
if current_user && (current_user == @article.user || @article.holders.include?(current_user))
which was termed as quite complex as the current_user was being used at many places. Hence this was refactored and made as
if @article.evaluate_user?(current_user)
and
#in article model
def evaluate_user?(user) user && (user == self.user || self.holders.include?(user)) end
Hence making the code in the view more readable and simple.
Rails 3 has a new concept of checking the model attributes for its availability and due to which for variables the .nil?, .blank?, .present? methods can be now be utilized only for objects. We could use “?” directly on any attribute. For example
To check a title of an article we can identify it as
@article.title.nil? or @article.title.blank? or @article.title.present?
Can be now replaced as
@article.title?
This makes code more clean and easy to ready. This is possible in any part of the application and not just restricted to views
This is similar to simplify renders in controllers, and it gives more details of what we could do with render partials and how to make user of locals.
For example
render :partial=>”somepartialfile”
Becomes
render “somepartialfile”
And
render :partial=>”somepartialfile”, :locals=>{:local1=>@local1,:local2=>@local2}
Can be re-written as
render “somepartialfile”,:local1=>@local1,:local2=>@local2
This is a better code readability but doesnot make much sense from the performance point of view, hence we have ignored this suggestion.
Whenever we create our controllers, by default our helpers get created. However thought they are just 4 lines basic code, they get loaded every time we run our application. So why waste time loading the empty helpers when you have not written any methods in it.
I would like to go with the Rails Best Practices to remove them. However one care should be taken that you do this at the end of the project and not at the beginning of the project, else you might not know when you really need it.
17. Use Multipart / Alternative As Content Type of Email
Most of the developers I have seen, just write the mailer’s code in an .html.erb or .html.haml code… however, they don’t take into consideration cases where the email clients donot support text/html content type
However some other developers to be on safer side, create text.erb file and send the email contents via text/plain content type. But this will deprive the rich content that the site would like to offer to its clients.
This can be achieved by use of a content type “Multipart/Content” type which can be used for both types for example
#set the content type
content_type "multipart/alternative" #in your mailer method
def welcome_email
. . . part :content_type => "text/html",
:body => render_message("send_welcome_email ", :email => email)
part "text/plain" do |p|
p.body = render_message("send_welcome_email", :email => email)
p.transfer_encoding = "base64"
endend
Calls following files
# app/views/notifier/send_welcome_email.html.erb # app/views/notifier/send_welcome_email.text.erb
We had to modify our Text/HTML component to include text/plain aswell.
Just because differnet developer are comfortable using different IDE, the tabs usage will lead to different number of spaces to be added. Which means code in one IDE looks differently organized on others IDE. It is always a best practice to remove your tabs and replace with spaces
We ignored this suggestion as this doesnot affect the performance of the system.
19. Remove Trailing White Space
Trailing white spaces are a pain when you are coding and they just occupy one space without any benefit. Also one disadvantage of the system would be that the trailing white spaces at times will create problems during “Diff”. Its advisable to remove them where ever necessary
We ignored this suggestion as this doesnot affect the performance of the system.
20. Use Say with Time in Migrations
If you have any migration files where it is used not to add / modify a field, but to change the contents or data in the table, it is advisable to run the migrations with say_with_time say_with_time, gives you a console IO entry (similar to puts) which will help you to identify what got modified and helps you to ensure what data have got executed.
For example if your migration file has
def self.up
Users.where(:author=>true).each do |user|
user.update_attribute(:article_count,user.articles.size)
end
end
In this case we wouldn’t know which all users got changed. So the better solution would be to use
def self.up
say_with_time “Updating all authors article count”
Users.where(:author=>true).each do |user|
say “User #{user.full_name} updated with #{user.articles.size}”
user.update_attribute(:article_count,user.articles.size)
end
end
end
At the end of migration, I would know which all users got migrated and what was the number. Makes sense isn’t it?
As developers most of the times we miss adding indexes when writing the migrations, we are more interested to complete the migration tables and move to the model parts, but what we miss at the end of the day is that Indexing is one of the most important entity when it comes to querying data faster.
It is a best practice to add indexing whenever you write your tabels, to add indexes to all your foreign keys.
We added about 9 indexes after running the Rails Best Practices.
With this we reduced 355 suggestions provided by the rails_best_practice plugin. There are many more suggestions that you might want to use, but for us these were the ones which we had to fix, we fixed about 234 suggestions and rest were ignored.
We would like to suggest to go through http://rails-bestpractices.com for more information on the differnet types of best practices.