July 15, 2014

Most Common Mistakes On Legacy Rails Apps

Lately I’ve been supporting several legacy projects.

As many of you may know, working on legacy projects sucks most of the time, usually because most of the code is ugly and difficult to understand or read.

I decided to make a list of common bad practices, or what I consider bad practices, and how to improve the code or avoid this bad practices.

Summary #

Using Query Methods Outside Models
Using Logic Inside The Views
Using Meaningless Names On Variables Or Methods
Using Unless Or Negative Expressions On Conditionals
Not Using Tell, Don’t Ask
Using Complex Conditionals
Using “self.” On Models Instace Methods When There’s No Need To
Using Conditionals And Returning The Condition
Using Inline CSS In Your Views
Using JavaScript Inside Your Views
Passing Method Call As An Argument When Calling A Method
Not Isolating Rake Tasks Using Classes
Not Following Sandi Metz’ Rules

Using Query Methods Outside Models #

Bad #

# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def index
    @users = User.where(active: true).order(:last_login_at)
  end

end

This code is not reusable or easy to test. If you want to look for all active users and order them again somewhere else on your code, you’ll end up duplicating code.

Good #

Instead of using query methods on your controllers, we just isolate them using scopes on your model as the example below. This makes the code reusable, easier to test and read.

# app/models/user.rb
class User < ActiveRecord::Base

  scope :active, -> { where(active: true) }
  scope :by_last_login_at, -> { order(:lasst_login_at) }

end

# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def index
    @users = User.active.by_last_login_at
  end

end

Everytime you’re about to use where, order, joins, includes, group, having or some other query method, remember to use them inside your models.

Using Logic Inside The Views #

Bad #

<!-- app/views/posts/show.html.erb -->
...

<h2>
  <%= "#{@comments.count} Comment#{@comments.count == 1 ? '' : 's'}" %>
</h2>

...

This small piece of code might seem not causing harm at first but this makes the HTML a bit complicated to read and it’s for sure that if you added one piece of logic in the view, you’ll end up adding more logic in the future. The other problem this code presents it’s that we can’t reuse it or test it in isolation.

Good #

Isolate the logic using Rails’ helpers.

# app/helpers/comments_helper.rb
module CommentsHelper
  def comments_count(comments)
    "#{comments.count} Comment#{comments.count == 1 ? '' : 's'}"
  end
end

<!-- app/views/posts/show.html.erb -->
<h2>
  <%= comments_count(@comments) %>
</h2>

The first benefit we get to see at first sight is that the HTML is easier to read. Also we can re use the code and test easily.

Another good aproach to avoid this problem is using decorators. Please refer to draper gem.

Note: You can use pluralize method to achieve the same result. I just used this code to make a point.

Using Meaningless Names On Variables Or Methods #

Bad #

# app/models/topic.rb
class Topic < ActiveRecord::Base

  def self.r_topics(questions)
    rt = []

    questions.each do |q|
      topics = q.topics

      topics.each do |t|
        if t.enabled?
          rt << t
        end
      end
    end

    Topic.where(id: rt)
  end

end

The main problem with this kind of legacy code is that you spend most of time trying to figure what does the code do. What does method like r_topics do, or variables like rt means. Other variables meaning like the one used on blocks can be deducted, but they do not reveal the intention at first sight.

Good #

Always use intention revealing names when naming methods or variables. This will help other developers to understand your code easily.

# app/models/topic.rb
class Topic < ActiveRecord::Base

  def self.related(questions)
    related_topics = []

    questions.each do |question|
      topics = question.topics

      topics.each do |topic|
        if topic.enabled?
          related_topics << topic
        end
      end
    end

    Topic.where(id: related_topics)
  end

end

The benefits of this approach are:

The first time you read the method name, you may have an idea of what the method will return. A collection of topics related to a collection of questions.
Now you know that related_topics is an array which stores the collection of the topics related to a collection of questions. In the previous code, it wasn’t clear what was the meaning of rt.
Using topic instead of t and question instead of q, makes your code easier to read the first time, because you don’t have to do Mental Compilation. Now you just read it and the code speaks for itself.

Using Unless Or Negative Expressions On Conditionals #

Bad #

# app/services/charge_user.rb
class ChargeUser

  def self.perform(user, amount)
    return false unless user.enabled?

    PaymentGateway.charge(user.account_id, amount)
  end

end

This code may not be difficult to understand at all, but using unless or negative expressions will add a little additional complexity to the code, because you have to do Mental Compilation.

Good #

The example above it’s easier to read if we use if or positive expressions.

# app/models/user.rb
class User < ActiveRecord::Base

  def disabled?
    !enabled?
  end

end

# app/services/charge_user.rb
class ChargeUser

  def self.perform(user, amount)
    return false if user.disabled?

    PaymentGateway.charge(user.account_id, amount)
  end

end

Don’t you think this way it’s easier to read? Prefer the use of if instead of unless and positive expressions over negative expressions. Remember to create opposite methods if you don’t have one, just like we did on the User model.

Not Using Tell, Don’t Ask #

Bad #

# app/models/user.rb
class User < ActiveRecord::Base

  def enable!
    update(enabled: true)
  end

end

# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def enable
    user = User.find(params[:id])

    if user.disabled?
      user.enable!
      message = "User enabled"
    else
      message = "User already disabled"
    end

    redirect_to user_path(user), notice: message
  end

end

The problem with this is that you’re adding control logic where it doesn’t belong. You are asking if a user is disabled, and if that’s the case, then it should enable that user.

Good #

A good approach would be to move that control logic inside our enable! method.

# app/models/user.rb
class User < ActiveRecord::Base

  def enable!
    if disabled?
      update(enabled: true)
    end
  end

end

# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def enable
    user = User.find(params[:id])

    if user.enable!
      message = "User enabled"
    else
      message = "User already disabled"
    end

    redirect_to user_path(user), notice: message
  end

end

Now our controller doesn’t need to know which conditions needs to meet a user to get enabled. The responsable of that is the User model and the enable! method.

Using Complex Conditionals #

Bad #

# app/controllers/posts_controller.rb
class PostsController < ApplicationController

  def destroy
    post = Post.find(params[:id])

    if post.enabled? && (user.own_post?(post) || user.admin?)
      post.destroy
      message = "Post destroyed."
    else
      message = "You're not allow to destroy this post."
    end

    redirect_to posts_path, notice: message
  end

end

This conditional is asking too many things, when it should really be asking for one thing. Can this user destroy this post?.

Good #

Now we realised that a user needs to own a post or be an admin to destroy that post, and also that post needs to be enabled. Extracting that conditional to a method which we can re use later would be the best way to go.

# app/models/user.rb
class User < ActiveRecord::Base

  def can_destroy_post?(post)
    post.enabled? && (own_post?(post) || admin?)
  end

end

# app/controllers/posts_controller.rb
class PostsController < ApplicationController

  def destroy
    post = Post.find(params[:id])

    if user.can_destroy_post?(post)
      post.destroy
      message = "Post destroyed."
    else
      message = "You're not allow to destroy this post."
    end

    redirect_to posts_path, notice: message
  end

end

Whenver you have a conditional that uses && or || extract that conditional into a method which you can re use later.

Using “self.” On Models Instace Methods When There’s No Need To #

Bad #

# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    "#{self.first_name} #{self.last_name}"
  end

end

This code it’s simple to read but there’s no need to use self., removing self. will make the code easier to read and the code will keep working.

Good #

On your models, self. is only needed on instance methods when you need to make assignments, otherwise, using self. will make your code a bit complicated to read.

# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    "#{first_name} #{last_name}"
  end

end

Using Conditionals And Returning The Condition #

Bad #

# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    if name
      name
    else
      "No name"
    end
  end

end

# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    name ? name : "No name"
  end

end

The problem with this codes is that they add control when there’s no need to.

Good #

There’s an easier way to get the same result.

# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    name || "No name"
  end

end

Basically this code returns the name if it’s different from false or nil, otherwise, it will return “No name”.

|| and && are very powerful operators which can help you to improve your code quality if you use it right.

Using Inline CSS In Your Views #

Bad #

<!-- app/views/projects/show.html.erb -->

...

<h3 style="font-size:20px;letter-spacing:normal;color:#95d60a;line-height:100%;margin:0;font-family:'Proxima Nova';">
  SECRET PROJECT
</h3>

...

Here we have only one tag which is receiving all that style. Now, imagine all your tags receiving inline styling. This will turn our HTML into an unreadable document and not only that, we will have to duplicate our styling code every time we have to introduce another h3 element with the same style.

Good #

// app/assets/stylesheets/application.css

.project-title {
    font-size: 20px;
    letter-spacing: normal;
    color: #95d60a; 
    line-height: 100%;
    margin: 0;
    font-family:'Proxima Nova';
}

<!-- app/views/projects/show.html.erb -->

...

<h3 class="project-title">
  SECRET PROJECT
</h3>

...

Now we can re use our style on any elements we want and our HTML is more readable and easy to understand.

Note: This example is pretty simple, you should consider splitting your CSS into several small files and use your application.css to load the css your need. Also you can and you should use inline CSS for email templates only.

Using JavaScript Inside Your Views #

Bad #

<!-- app/views/questions/show.html.erb -->

...

<textarea rows="4" cols="50" class='wysihtml5'>
  Insert your question details here.
</textarea>

...

<script>
  $(document).ready(function(){
  $('textarea.wysihtml5').wysihtml5({
    "font-styles": true, //Font styling, e.g. h1, h2, etc. Default true.
    "emphasis": true, //Italics, bold, etc. Default true.
    "lists": true, //(Un)ordered lists, e.g. Bullets, Numbers. Default true.
    "html": false, //Button which allows you to edit the generated HTML. Default false.
    "link": true, //Button to insert a link. Default true.
    "image": true, //Button to insert an image. Default true.
    "color": true //Button to change color of font. Default true.
  });
});
<script>

This is bad because this logic gets attached to this specific view. You can’t re use this code.

Good #

Rails has it own place to store and organize all your javascript, the app/assets/javascripts/ path.

// app/assets/javascripts/application.js

...

$(document).ready(function(){
  $('textarea.wysihtml5').wysihtml5({
    "font-styles": true, //Font styling, e.g. h1, h2, etc. Default true.
    "emphasis": true, //Italics, bold, etc. Default true.
    "lists": true, //(Un)ordered lists, e.g. Bullets, Numbers. Default true.
    "html": false, //Button which allows you to edit the generated HTML. Default false.
    "link": true, //Button to insert a link. Default true.
    "image": true, //Button to insert an image. Default true.
    "color": true //Button to change color of font. Default true.
  });
});

...

<!-- app/views/questions/show.html.erb -->

...

<textarea rows="4" cols="50" class='wysihtml5'>
  Insert your question details here.
</textarea>

...

Now we can re use our code in any view. We just need to add a textarea element with the wysihtml5 class and our javascript code will be executed.

Note: This example is pretty simple, you should consider splitting your JavaScript into several small files and use your application.js to load the JavaScript your need. Also, if you are using CoffeeScript instead of JavaScript, please, be consistent and stick with it, don’t write some code using CoffeeScript and other in JavaScript.

Passing Method Call As An Argument When Calling A Method #

Bad #

# app/services/find_or_create_topic.rb
class FindOrCreateTopic

  ...

  def self.perform(user, name)
    find(user, sluggify(name)) || create(user, name)
  end

  ...

end

The issue with this relies when calling find and passing as arguments user and sluggifymethod which receives nameas an argument. You might be thinking, what’s the problem with this? I can understand the code perfectly. Yes, the code might be easy to read, but you have to make a little Mental Compilation which I like to avoid every time I can.

Good #

A good way to avoid Mental Compilation when this happens is using variables with meaningful names.

# app/services/find_or_create_topic.rb
class FindOrCreateTopic

  ...

  def self.perform(user, name)
    slug = sluggify(name)
    find(user, slug) || create(user, name)
  end

  ...

end

This way you’ll avoid doing Mental Compilation. Our variable has an intention revealing name, and now when your call the find method, you know that find is receiving a user and a slug.

Not Isolating Rake Tasks Using Classes #

Bad #

# lib/tasks/recalculate_badges_for_users.rake
namespace :users do

  desc "Recalculates Badges for Users"
  task recalculate_badges: :environment do
    user_count = User.count

    User.find_each do |user|
      puts "#{index}/#{user_count} Recalculating Badges for: #{user.email}"

      if user.questions_with_negative_votes >= 1 || user.answers_with_negative_votes >= 1
        user.grant_badge('Critic')
      end

      user.answers.find_each do |answer|
        question   = answer.question
        next unless answer && question

        days       = answer.created_at - question.created_at
        days_count = (days / 1.day).round

        if (days_count >= 60) && (question.vote_count >= 5)
          grant_badge('Necromancer') && return
        end
      end
    end
  end

end

There are several problems with this rake task. The main problem is that this rake is very long and difficult to test. It’s not easy to read or understand at first sight. You have to trust that it recalculates the badges for your users because it say so.

Good #

The best way to proceed with this is to move all your logic to an specific class. So, lets create a class specific for this job.

# app/services/recalculate_badge.rb
class RecalculateBadge
  attr_reader :user

  def initialize(user)
    @user = user
  end

  def grant_citric
    if grant_citric?
      user.grant_badge('Critic')
    end
  end

  def grant_necromancer
    user.answers.find_each do |answer|
      question = answer.question
      next unless answer && question

      if grant_necromancer?(answer, question)
        grant_badge('Necromancer') && return
      end
    end
  end

  protected

    def grant_citric?
      (user.questions_with_negative_votes >= 1) ||
      (user.answers_with_negative_votes >= 1)
    end

    def days_count(answer, question)
      days = answer.created_at - question.created_at
      (days / 1.day).round
    end

    def grant_necromancer?(answer, question)
      (days_count(answer,question) >= 60) &&
      (question.vote_count >= 5)
    end

end

# lib/tasks/recalculate_badges_for_users.rake
namespace :users do

  desc "Recalculates Badges for Users"
  task recalculate_badges: :environment do
    user_count = User.count

    User.find_each do |user|
      puts "#{index}/#{user_count} Recalculating Badges for: #{user.email}"
      task = RecalculateBadge.new(user)
      task.grant_citric
      task.grant_necromancer
    end
  end

end

As you can see, now the rake task is easier to read and you can test every grant badge method on isolation. Also we could reuse this class if we need to. Of course this code could be written in a better way, but lets keep it simple.

Thanks @luislavena for the suggestion of refactoring this example.

Not Following Sandi Metz’ Rules #

Classes can be no longer than one hundred lines of code.
Methods can be no longer than five lines of code.
Pass no more than four parameters into a method. Hash options are parameters.
Controllers can instantiate only one object. Therefore, views can only know about one instance variable and views should only send messages to that object (@object.collaborator.value is not allowed).

Check out this article from thoughtbot, Sandi Metz’ Rules For Developers for more details about these rules.

603

Kudos

603

Kudos

Most Common Mistakes On Legacy Rails Apps

Summary #

Using Query Methods Outside Models #

Bad #

Good #

Using Logic Inside The Views #

Bad #

Good #

Using Meaningless Names On Variables Or Methods #

Bad #

Good #

Using Unless Or Negative Expressions On Conditionals #

Bad #

Good #

Not Using Tell, Don’t Ask #

Bad #

Good #

Using Complex Conditionals #

Bad #

Good #

Using “self.” On Models Instace Methods When There’s No Need To #

Bad #

Good #

Using Conditionals And Returning The Condition #

Bad #

Good #

Using Inline CSS In Your Views #

Bad #

Good #

Using JavaScript Inside Your Views #

Bad #

Good #

Passing Method Call As An Argument When Calling A Method #

Bad #

Good #

Not Isolating Rake Tasks Using Classes #

Bad #

Good #

Not Following Sandi Metz’ Rules #

Now read this

Strong Parameters The Right Way