Refactoring Legacy Code in Ruby

By Sava Virtosu

Any fool can write code that a computer can understand. Good programmers write code that humans can understand.

Martin Fowler

What is Refactoring?

A change to the system that leaves its behavior unchanged, but enhances some nonfunctional quality – simplicity, flexibility, understandability, performance

Kent Beck

Refactoring for me is:

When Should You Refactor?

The Rule of Three

The first time you do something, you just do it. The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway. The third time you do something similar, you refactor.

Three strikes and you refactor.

Don Roberts

HOw to refactor the code?

Code Smells

Duplicated Code

class BooksController < ApplicationController
  def created
    params[:book][:page] ||= 1
    params[:book][:per_page] ||= 10
    params.require(:book).permit(:title, :description, :author_id, :page)

    @book = Book.new(params)
    respond_to do |format|
      ...
    end
  end  def update
    params[:book][:page] ||= 1
    params[:book][:per_page] ||= 10
    params.require(:book).permit(:title, :description, :author_id, :page)

    respond_to do |format|
      ...
    end
  end
end

Duplicated Code


class BooksController < ApplicationController

  def created
    @book = Book.new(book_params)

    respond_to do |format|
      ...
    end
  end


  private
    def book_params
      params[:book][:page] ||= 1
      params[:book][:per_page] ||= 10
      params.require(:book).permit(:title, :description, :author_id, :page)
    end
end

Large Class || Conditionals

class User < ActiveRecord::Base

  scope :teacher, :conditions => { :role => 'teacher' }
  scope :student, :conditions => { :role => 'student' }

  ...
end

class Student < User
  after_initialize :set_default_role

  private
    def set_default_role
      self.role ||= 'student'
    end
end

Inheritance vs Mixins ?

Long Method

FUNCTIONS SHOULD DO ONE THING.

THEY SHOULD DO IT WELL.

THEY SHOULD DO IT ONLY.

Long Parameter List || Named Parameters


  ...
  def set_params author_id, title = '', article_id = nil, search_size = 10,  include_author = false
    ...
  end

searchCriteria.set_params(5, "Hope", 89)

def set_params(options = {})
  puts options["author_id"]
end

searchCriteria.set_params :author_id => 5, :title => "Hope", :article_id => 89

def set_params(*args)
  puts args.inspect
  options = args.extract_options!
end

searchCriteria.set_params :author_id => 5, :title => "Hope", { :article_id => 89 }
# => [5, "Hope", { "article_id" => "89" }]

Feature Envy

class Item  
  ...
end 
class Cart
  def item_price
    @item.price + @item.tax
  end
end

class Item  
  def price
    @price + @tax
  end
end 
class Cart
  def item_price
    @item.price
  end
end

Substitute Algorithm

def found_friends(people)
  friends = []
  people.each do |person|
    if(person == "Don")
      friends << "Don"
    end
    if(person == "John")
      friends << "John"
    end
    if(person == "Kent")
      friends << "Kent"
    end 
  end
  return friends
end

def found_friends(people)
  people.select do |person|
    %w(Don John Kent).include? person
  end
end

Dynamic Method Definition

class User
  ...
  
  def is_accepted?
    status == 'accepted'
  end
  def is_pending?
    status == 'pending'
  end
  def is_rejected?
    status == 'rejected'
  end
  def is_suspended?
    status == 'suspended'
  end
  ...
end

Dynamic Method Definition

class User
  ...
  
  ['accepted','pending','rejected','suspended'].each do |status|
    class_eval %{
      def #{status}?
        self[:status] == '#{status}'
      end

      alias_method :is_#{status}?, :#{status}?
    }
  end
  ...
end

Metaprogramming Madness?

I refactored all my classes and methods, now nothing works, that should i do next ?

TDD is The Only way

Cover your code before starting refactoring.

Null Object

class Article
  ...

  def self.find_and_rename id, new_name
    article = Article.find_by_id id
    article.rename new_name unless article.nil?
  end

  def rename new_name
    name = new_name + " - " + created_at.t_s
  end
  ...
end

describe Article do
  it "is renamed" do
    assert(Article.find_and_rename(1)).to be true
  end

  it "does nothing if article is not found" do
    Article.find_and_rename(0)
  end
end

Null Object

class Article
  ...

  def self.find_and_rename id, new_name
    article = Article.find_by_id(id) || NullArticle.new
    article.rename new_name 
  end

  def rename new_name
    name = new_name + " - " + created_at.t_s
  end
  ...
end

class NullArticle
  def rename
    #do nothing
  end
end

Separate Query from Modifier

class Post
  ...

  def publish
    @published = true
    return POSTS.count { |post| !post.published? }
  end
  ...
end

it "is publishable and retrieves all unpublished posts count" do
  post = Post.find(1)
  post.publish.must_equal 3
end

Separate Query from Modifier

it "is publishable" do
  post = Post.find(1)
  post.publish.must_equal true
end

it "retrieves all unpublished posts count" do
  Post.unpublished.must_equal 4
end

class Post
  ...
  def self.unpublished
    return POSTS.count { |post| !post.published? }
  end

  def publish
    @published = true
  end
  ...
end

Assertions

class SquareRootCalculator
  class << self
    def calculate number
      if number > 0
        Math.sqrt number
      end
    end
  end
end

it "doesn't calculate a negative number's square root" do
      expect(SquareRootCalculator.calculate(-9)).to be_nil
end

expect { SquareRootCalculator.calculate(-9) }.to raise_error ArgumentError

Assertions

module Assertions
  def assert_positiv &block
    raise ArgumentError unless block.call
  end
  def assert_not_big &block
    raise TooBigError unless block.call
  end
end

class SquareRootCalculator
  extend Assertions

  def self.calculate number
    assert_positiv { number > 0 }
    assert_not_big { number < 5000 }
    Math.sqrt number
  end
end

Speculative Generality?

REFACTORING UNIT TESTS

Assertion Roulette

  it 'returns 201 if vote is down, deletes downvote and creates upvote' do
      @comment.comment_votes.map(&:destroy)
      downvote = FactoryGirl.create(:comment_vote)

      expect(@comment.upvotes_count).to eql(0)
      expect(@comment.downvotes_count).to eql(1)

      api_put "/posts/#{@post.safe_id}/comments/#{@comment.safe_id}/upvote"

      expect(response.status).to eql(201)

      comment = @comment.reload
                    
      with_json_response do |json|
        expect(json[:message]).to eql(['Comment upvoted!'])
        expect(json[:comment]).to eql(to_api_comment(comment))
      end

      expect(comment.upvotes_count).to eql(1)
      expect(comment.downvotes_count).to eql(0)

      expect(comment.comment_votes(true).find_by_id(downvote.id)).to be_nil
    end

Resource Optimism

it 'queries some external resource' do
  uri = URI('http://example.com/')

  response = Net::HTTP.get(uri)
  body = JSON.parse(response.body)

  expect(body).to include('example')
end

RSpec.configure do |config|
  config.before(:each) do
    stub_request(:get, /example.com/).
      with(headers: {'Accept'=>'*/*', 'User-Agent'=>'Ruby'}).
      to_return(status: 200, body: "example", headers: {})
  end
end

webmock

Indirect Testing

it 'sets book price' do
  Store.receive(:book, "New Book", 100)  
  expect(Store.count(:book, "New Book")).to be eq(100)
end

Incidental State

it 'publishes the article' do
  article.publish

  expect(Article.count).to eq(2)
end

it 'publishes the article' do
  expect { article.publish }.to change(Article, :count).by(1)
end

Tools

rails_best_practices

metric_fu

reek

simple_cov

....

codeclimate

RubyMine

Books and References

Books:

"Refactoring, Ruby Edition" by Martin Fowler, Jay Fields, Shane Harvie
"Working Effectively with Legacy Code" by Michael Feathers
"Clean Code" by Robert C. Martin

Links:

http://ghendry.net/refactor.html
http://blog.codeclimate.com/
http://sourcemaking.com/refactoring
http://youtu.be/q_qdWuCAkd8

Thanks

Refactoring Legacy Code in Ruby

By Sava Virtosu

Refactoring Legacy Code in Ruby

Practical examples about how bad code smells, and how to change it in a better way. How to add new functionalities to live running legacy system without breaking existing code. Discussion about how to write easy maintainable code.

11 years ago
1,847

Refactoring Legacy Code in Ruby

What is Refactoring?

When Should You Refactor?

HOw to refactor the code?

Code Smells

TDD is The Only way

REFACTORING UNIT TESTS

Tools

Books and References

Refactoring Legacy Code in Ruby

More from Sava Virtosu