Since I'm all hopped-up on white tea from an early morning meeting, this refactoring is a two in one.
The Refactoring
I used move method and extract method on the InvoiceController to move the business logic down into the model where it belongs.
Before
class InvoiceController < ApplicationController
def autofill
# Get project
@p = Project.find_by_id(params[:autofill][:project_id])
# Get customer
@customer = Customer.find_by_id(@p.customer_id) # Customer plugin only has a 1-way relationship
# Build date range
@date_from = params[:autofill][:date_from]
@date_to = params[:autofill][:date_to]
# Build activities
if params[:autofill][:activities]
@activities = params[:autofill][:activities].collect {|p| p.to_i }
end
@activities ||= []
# Fetch issues
@issues = @p.issues.find(:all,
:conditions => ['time_entries.spent_on >= :from AND time_entries.spent_on <= :to AND time_entries.activity_id IN (:activities)',
{
:from => @date_from,
:to => @date_to,
:activities => @activities
}],
:include => [:time_entries])
@total_time = @issues.collect(&:time_entries).flatten.collect(&:hours).sum
# Time logged without an issue
@time_entries = @p.time_entries.find(:all,
:conditions => ['issue_id IS NULL AND spent_on >= :from AND spent_on <= :to AND activity_id IN (:activities)',
{
:from => @date_from,
:to => @date_to,
:activities => @activities
}])
@total_time += @time_entries.collect(&:hours).sum
@total = @total_time.to_f * params[:autofill][:rate].to_f
respond_to do |format|
format.js
end
end
end
After
class InvoiceController < ApplicationController
def autofill
@autofill = Autofill.new_from_params(params[:autofill])
# TODO: should just access @autofill directly
@p = @autofill.p
@customer = @autofill.customer
@date_from = @autofill.date_from
@date_to = @autofill.date_to
@activities = @autofill.activities
@issues = @autofill.issues
@total_time = @autofill.total_time
@time_entries = @autofill.time_entries
@total = @autofill.total
respond_to do |format|
format.js
end
end
end
class Autofill
def self.new_from_params(params)
autofill = Autofill.new
# Get project
autofill.p = Project.find_by_id(params[:project_id])
# Get customer
autofill.customer = Customer.find_by_id(autofill.p.customer_id) # Customer plugin only has a 1-way relationship
# Build date range
autofill.date_from = params[:date_from]
autofill.date_to = params[:date_to]
# Build activities
if params[:activities]
autofill.activities = params[:activities].collect {|p| p.to_i }
end
autofill.activities ||= []
# Fetch issues
autofill.issues = autofill.p.issues.find(:all,
:conditions => ['time_entries.spent_on >= :from AND time_entries.spent_on <= :to AND time_entries.activity_id IN (:activities)',
{
:from => autofill.date_from,
:to => autofill.date_to,
:activities => autofill.activities
}],
:include => [:time_entries])
autofill.total_time = autofill.issues.collect(&:time_entries).flatten.collect(&:hours).sum
# Time logged without an issue
autofill.time_entries = autofill.p.time_entries.find(:all,
:conditions => ['issue_id IS NULL AND spent_on >= :from AND spent_on <= :to AND activity_id IN (:activities)',
{
:from => autofill.date_from,
:to => autofill.date_to,
:activities => autofill.activities
}])
autofill.total_time += autofill.time_entries.collect(&:hours).sum
autofill.total = autofill.total_time.to_f * params[:rate].to_f
autofill
end
end
Review
With this code now in the model, the controller action is a lot cleaner and I can really start to work on refining the Autofill logic. This also opens up the potential for scripting the Model with a Rake tasks (e.g. automated invoices?).
One thing I wanted to point out:
This Autofill code is horrible, ugly, and only had user testing (no unit tests). And I wrote it as an experienced Rails developer who knew better. Point: everyone writes bad code, what makes you a better developer is keeping an open mind and trying to improve that code as much as you can. Just like what I'm trying to do with this Refactoring series.
Reference commit
Tagged: ruby redmine refactoring extract-method move-method
The Refactoring
Since I refactored Invoice#outstanding yesterday, I can now build on that to refactor Invoice#fully_paid?.
Before
class Invoice < ActiveRecord::Base
def fully_paid?
total_paid = self.payments.collect(&:amount).sum
return total_paid >= self.amount
end
end
After
class Invoice < ActiveRecord::Base
def fully_paid?
outstanding <= 0
end
end
Review
A nice side benefit of this refactoring is that since #fully_paid? uses #outstanding it will also get the performance boost from using the SQL sum.
Reference commit
Tagged: ruby redmine refactoring replace-temp-with-query
I decided to move to another section of Invoice before I tackle the finder used in Invoice#open. I'm thinking about using a simple state machine for the different invoice states and that will require some redesign. I found it's best to think about redesigns for time before starting on them.
The Refactoring
Today I refactored the inner logic of Invoice#outstanding to replace how a temp variable is being used.
Before
class Invoice < ActiveRecord::Base
def outstanding
total = self.amount - self.payments.collect(&:amount).sum
if total > 0.0
return total
else
return 0.0
end
end
end
After
class Invoice < ActiveRecord::Base
def outstanding
(total = amount - payments.sum('amount')) > 0 ? total : 0.0
end
end
Review
This was a simple method but it was doing two things wrong:
- There was too much code to do a simple arithmetic operation.
payments.collect(&:amount).sum was creating a bunch of ActiveRecord objects that would just get thrown away. The new code uses SQL to sum the amount field directly.
I'm thinking about changing this method so it will return negative amounts when there is an overpayment. If that happens, I can remove the ternary operation entirely.
Reference commit
Tagged: ruby redmine refactoring inline-temp
The Refactoring
While adding tests to my Redmine Invoice plugin, I found the following nasty bit of logic.
Before
class Invoice < ActiveRecord::Base
def self.open
invoices = self.find(:all)
return invoices.select { |invoice| !invoice.fully_paid? && !invoice.late? }
end
end
After
class Invoice < ActiveRecord::Base
def self.open
invoices = self.find(:all)
return invoices.select { |invoice| invoice.open? }
end
# Is this invoice current but not fully paid?
def open?
!fully_paid? && !late?
end
end
Review
By using extract method, I created the #open? predicate which made the code (and tests) easier to read. I need to work on the finder inside Invoice#open next, since it's selecting every Invoice record and filtering in Ruby.
Reference commit
Tagged: ruby redmine refactoring extract-method
I'm starting to refactor my Redmine Invoice plugin now. This plugin is over two years old now and it's code has seen better days.
The Refactoring
The Rails Best Practices gem showed me a quick refactoring to remove some duplication from InvoiceController using a before_filter.
Before
class InvoiceController < ApplicationController
def show
@invoice = Invoice.find(params[:invoice][:id])
@payments = @invoice.payments.find(:all, :order => 'applied_on DESC')
render :layout => 'print' if params[:print]
end
def edit
@invoice = Invoice.find(params[:invoice][:id])
@last_number = last_invoice_number
end
def update
@invoice = Invoice.find(params[:invoice][:id])
if @invoice.update_attributes(params[:invoice])
flash[:notice] = "Invoice saved"
redirect_to :action => "show", :id => params[:id], :invoice => { :id => @invoice }
else
render :action => 'edit', :id => params[:id], :invoice => { :id => @invoice }
end
end
def destroy
@invoice = Invoice.find(params[:invoice][:id])
if @invoice.destroy
flash[:notice] = "Invoice deleted"
redirect_to :action => "index", :id => params[:id]
else
flash[:notice] = "Error"
render :action => 'index', :id => params[:id]
end
end
end
After
class InvoiceController < ApplicationController
before_filter :find_invoice, :only => [:show, :edit, :update, :destroy]
def show
@payments = @invoice.payments.find(:all, :order => 'applied_on DESC')
render :layout => 'print' if params[:print]
end
def edit
@last_number = last_invoice_number
end
def update
if @invoice.update_attributes(params[:invoice])
flash[:notice] = "Invoice saved"
redirect_to :action => "show", :id => params[:id], :invoice => { :id => @invoice }
else
render :action => 'edit', :id => params[:id], :invoice => { :id => @invoice }
end
end
def destroy
if @invoice.destroy
flash[:notice] = "Invoice deleted"
redirect_to :action => "index", :id => params[:id]
else
flash[:notice] = "Error"
render :action => 'index', :id => params[:id]
end
end
private
def find_invoice
@invoice = Invoice.find(params[:invoice][:id])
end
end
Review
I can see the Redmine Invoice plugin is going to need a lot of work over the next few days. It has a lot of legacy code back from Redmine's early days, before I figured out how to test Redmine plugins. I'm hoping to get it cleaned up and released along with Redmine 1.0 this summer.
Reference commit
Tagged: ruby redmine refactoring move-method
The Refactoring
Today I used move method to clean up a hack job I made a couple of months ago.
Before
class KanbansController < ApplicationController
def sync
# Brute force update :)
Issue.all.each do |issue|
KanbanIssue.update_from_issue(issue)
end
respond_to do |format|
format.html {
flash[:notice] = l(:kanban_text_notice_sync)
redirect_to kanban_path
}
end
end
end
After
class KanbansController < ApplicationController
def sync
Issue.sync_with_kanban
respond_to do |format|
format.html {
flash[:notice] = l(:kanban_text_notice_sync)
redirect_to kanban_path
}
end
end
module RedmineKanban
module IssuePatch
module ClassMethods
# Brute force update :)
def sync_with_kanban
Issue.all.each do |issue|
KanbanIssue.update_from_issue(issue)
end
end
end
end
end
Review
This refactoring accomplishes two things:
- It helps someone understand what the
#sync action in the controller does.
- It gives a little handhold for anyone wanting to run the sync from someone else in the code. (e.g. like scripting the sync from a Rake task)
Reference commit
Tagged: ruby redmine refactoring move-method
The code in the Redmine Merge plugin looks pretty good so I'm going back to my Redmine Kanban plugin.
Today's refactoring was guided by the Rails Best Practices gem. This gem uses static code analysis to search for common smells in Rails applications. Running it on Redmine Kanban showed several smells I can tackle:
./app/controllers/kanbans_controller.rb:8,13 - use before_filter for show,update
./app/views/settings/_kanban_settings.html.erb:11 - move code into controller
./app/views/settings/_kanban_settings.html.erb:30 - move code into controller
./app/views/settings/_kanban_settings.html.erb:69 - move code into controller
./app/views/settings/_kanban_settings.html.erb:85 - move code into controller
./app/models/kanban_pane/incoming_pane.rb:8 - keep finders on their own model
./app/views/settings/_kanban_settings.html.erb:6 - move code into model (@settings)
./app/views/settings/_kanban_settings.html.erb:7 - move code into model (@settings)
./app/views/settings/_kanban_settings.html.erb:63 - move code into model (@settings)
./app/views/settings/_kanban_settings.html.erb:64 - move code into model (@settings)
./app/views/settings/_kanban_settings.html.erb:79 - move code into model (@settings)
./app/views/settings/_kanban_settings.html.erb:80 - move code into model (@settings)
The Refactoring
I'm starting with an extract method in KanbansController.
Before
class KanbansController < ApplicationController
def show
@settings = Setting.plugin_redmine_kanban
@kanban = Kanban.new
end
def update
@settings = Setting.plugin_redmine_kanban
@from = params[:from]
@to = params[:to]
user_and_user_id
# ...
end
end
After
class KanbansController < ApplicationController
before_filter :setup_settings
def show
@kanban = Kanban.new
end
def update
@from = params[:from]
@to = params[:to]
user_and_user_id
# ...
end
def setup_settings
@settings = Setting.plugin_redmine_kanban
end
end
Review
These refactorings are one of my favorites. They are simple and help to highlight the differences between different methods. Just like waves on a coastline, they can really change the feel of code over time.
Reference commit
Tagged: ruby redmine refactoring extract-method
The Refactoring
Today's refactoring is just like yesterday's but I wanted to show a quick tip with #create! I didn't mention.
Before
class SourceVersion < ActiveRecord::Base
include SecondDatabase
set_table_name :versions
def self.migrate
all.each do |source_version|
v = Version.new
v.attributes = source_version.attributes
v.project = Project.find(RedmineMerge::Mapper.get_new_project_id(source_version.project_id))
v.save!
RedmineMerge::Mapper.add_version(source_version.id, v.id)
end
end
end
After
class SourceVersion < ActiveRecord::Base
include SecondDatabase
set_table_name :versions
def self.migrate
all.each do |source_version|
version = Version.create!(source_version.attributes) do |v|
v.project = Project.find(RedmineMerge::Mapper.get_new_project_id(source_version.project_id))
end
RedmineMerge::Mapper.add_version(source_version.id, version.id)
end
end
end
Review
The difference between this refactoring and yesterday's is that I need to keep track of the saved objects for RedmineMerge::Mapper. Since I removed the v temporary variable and used #create! I need to reintroduce a variable that represents the saved Version. Luckily, #create! returns the saved object when it's successful so the flow ends up:
Version.create! builds a new Version object, v.v gets it's attributes set.v is yielded to the block, picking up it's project attribute through the setter (v.project=).- The block returns and
Version.create! tries to save. If saving fails, an Exception is thrown and Ruby jumps up the stack until the script exits.
- If saving was successful,
Version.create! returns the saved Version object setting it to the local variable version.
RedmineMerge::Mapper can now use the local variable version.
Not bad for 4 lines of Ruby.
Reference commit
Tagged: ruby redmine refactoring inline-temp
I wanted to start refactoring my Redmine merge plugin now. I'm getting it ready for a release so doing a few refactorings now will help me make sure it's working properly.
The Refactoring
Using inline temp I was able to clean up how the SourceNews#migrate converts data from one database to the other database.
Before
class SourceNews < ActiveRecord::Base
include SecondDatabase
set_table_name :news
belongs_to :author, :class_name => 'SourceUser', :foreign_key => 'author_id'
def self.migrate
all.each do |source_news|
n = News.new
n.attributes = source_news.attributes
n.project = Project.find(RedmineMerge::Mapper.get_new_project_id(source_news.project_id))
n.author = User.find_by_login(source_news.author.login)
n.save!
end
end
end
After
class SourceNews < ActiveRecord::Base
include SecondDatabase
set_table_name :news
belongs_to :author, :class_name => 'SourceUser', :foreign_key => 'author_id'
def self.migrate
all.each do |source_news|
News.create!(source_news.attributes) do |n|
n.project = Project.find(RedmineMerge::Mapper.get_new_project_id(source_news.project_id))
n.author = User.find_by_login(source_news.author.login)
end
end
end
end
Review
Since SourceNews#migrate was a pretty simple method, this refactoring is easy to understand. By using the block method of ActiveRecord#create! I don't have to worry about calling #save! at the end. It's also a nice and simple way to keep all of the News construction in places (inside the block).
Reference commit
Tagged: ruby redmine refactoring inline-temp
The Refactoring
Today I used extract class to move the #calendar method from IssuesController to it's own controller. This is part of a large refactoring plan I have to get IssuesController slimmed down to a more manageable size.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
before_filter :authorize, :except => [:index, :changes, :calendar, :preview, :context_menu]
before_filter :find_optional_project, :only => [:index, :changes, :calendar]
def calendar
if params[:year] and params[:year].to_i > 1900
@year = params[:year].to_i
if params[:month] and params[:month].to_i > 0 and params[:month].to_i < 13
@month = params[:month].to_i
end
end
@year ||= Date.today.year
@month ||= Date.today.month
@calendar = Redmine::Helpers::Calendar.new(Date.civil(@year, @month, 1), current_language, :month)
retrieve_query
@query.group_by = nil
if @query.valid?
events = []
events += @query.issues(:include => [:tracker, :assigned_to, :priority],
:conditions => ["((start_date BETWEEN ? AND ?) OR (due_date BETWEEN ? AND ?))", @calendar.startdt, @calendar.enddt, @calendar.startdt, @calendar.enddt]
)
events += @query.versions(:conditions => ["effective_date BETWEEN ? AND ?", @calendar.startdt, @calendar.enddt])
@calendar.events = events
end
render :layout => false if request.xhr?
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
before_filter :authorize, :except => [:index, :changes, :preview, :context_menu]
before_filter :find_optional_project, :only => [:index, :changes]
# No more calendar action
end
# app/controllers/calendars_controller.rb
class CalendarsController < ApplicationController
before_filter :find_optional_project
rescue_from Query::StatementInvalid, :with => :query_statement_invalid
helper :issues
helper :projects
helper :queries
include QueriesHelper
def show
if params[:year] and params[:year].to_i > 1900
@year = params[:year].to_i
if params[:month] and params[:month].to_i > 0 and params[:month].to_i < 13
@month = params[:month].to_i
end
end
@year ||= Date.today.year
@month ||= Date.today.month
@calendar = Redmine::Helpers::Calendar.new(Date.civil(@year, @month, 1), current_language, :month)
retrieve_query
@query.group_by = nil
if @query.valid?
events = []
events += @query.issues(:include => [:tracker, :assigned_to, :priority],
:conditions => ["((start_date BETWEEN ? AND ?) OR (due_date BETWEEN ? AND ?))", @calendar.startdt, @calendar.enddt, @calendar.startdt, @calendar.enddt]
)
events += @query.versions(:conditions => ["effective_date BETWEEN ? AND ?", @calendar.startdt, @calendar.enddt])
@calendar.events = events
end
render :layout => false if request.xhr?
end
end
Review
Since I just finished up refactoring the GanttsController this refactoring was pretty easy. IssuesController is a still a large controller (482 lines) but it's getting better each week. It looks like all of the remaining actions in IssuesController belong there so I'm going to start to tackle some duplication and code smells inside each method.
Reference commit
Tagged: ruby redmine refactoring extract-class
