July 15, 2014

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.

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 #

# 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.

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 #

<!-- 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.

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 #

# 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.

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 .

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 . 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 #

# 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.

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 #

# 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.

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 #

# 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?.

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 #

# 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.

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 #

# app/models/user.rb class User < ActiveRecord::Base def full_name if name name else "No name" end end end

or

# 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.

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 #

<!-- 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.

// 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 #

<!-- 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.

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 #

# 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 sluggify method which receives name as 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.

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 #

# 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.

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.

599 Kudos