written by Eric Davis on March 09, 2010
0 Comments
I did this refactoring after yesterday's but I wanted to do a separate post to clearly show how I got to the end result.
The Refactoring
The big duplication in #show_detail is in the case statement. Yesterday's refactoring got it cleaned up enough so that seven cases where identical. So now refactoring the duplication is easy using a variant of inline method.
Before
# app/helpers/issues_helper.rb
module IssuesHelper
def show_detail(detail, no_html=false)
case detail.property
when 'attr'
field = detail.prop_key.to_s.gsub(/\_id$/, "")
label = l(("field_" + field).to_sym)
case detail.prop_key
when 'due_date', 'start_date'
value = format_date(detail.value.to_date) if detail.value
old_value = format_date(detail.old_value.to_date) if detail.old_value
when 'project_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'status_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'tracker_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'assigned_to_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'priority_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'category_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'fixed_version_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'estimated_hours'
value = "%0.02f" % detail.value.to_f unless detail.value.blank?
old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank?
end
when 'cf'
custom_field = CustomField.find_by_id(detail.prop_key)
if custom_field
label = custom_field.name
value = format_value(detail.value, custom_field.field_format) if detail.value
old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value
end
when 'attachment'
label = l(:label_attachment)
end
# ... more implementation
end
end
After
module IssuesHelper
def show_detail(detail, no_html=false)
case detail.property
when 'attr'
field = detail.prop_key.to_s.gsub(/\_id$/, "")
label = l(("field_" + field).to_sym)
case
when ['due_date', 'start_date'].include?(detail.prop_key)
value = format_date(detail.value.to_date) if detail.value
old_value = format_date(detail.old_value.to_date) if detail.old_value
when ['project_id', 'status_id', 'tracker_id', 'assigned_to_id', 'priority_id', 'category_id', 'fixed_version_id'].include?(detail.prop_key)
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when detail.prop_key == 'estimated_hours'
value = "%0.02f" % detail.value.to_f unless detail.value.blank?
old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank?
end
when 'cf'
custom_field = CustomField.find_by_id(detail.prop_key)
if custom_field
label = custom_field.name
value = format_value(detail.value, custom_field.field_format) if detail.value
old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value
end
when 'attachment'
label = l(:label_attachment)
end
# ... more implementation
end
end
Review
Once the case statement stopped checking detail.prop_key specifically, I was able to group the similar cases together. Now there are only three cases to watch for:
- Dates - due date or start date
- Associations
- Estimated hours
This is still a really smelly method with three case statements and four if statements but it's getting better. I'm not sure if I want to keep refactoring it or move onto another section. I've found that sometimes it's easy to over factor a section of code so you end up with this really nice piece of code, surrounded by a swampland of cruft.
Reference commit
Tagged: ruby redmine refactoring inline-method
written by Eric Davis on March 08, 2010
0 Comments
After over three months of development, I'm relieved to finally announce that SeeProjectRun has entered it's private beta. I'm using this beta period to get feedback before I open it up to the general public in April. Since I'll be actively connecting with every beta participant, I need to slowly give out invitations to keep from overloading myself. If you have already signed up for the See Project Run list, then you should be getting an invite this month.
Eric Davis
Tagged: business seeprojectrun
written by Eric Davis on March 08, 2010
0 Comments
The Refactoring
The flay report on Caliper shows that IssuesHelper#show_detail has the most duplicated code in Redmine, specifically inside of a long case statement. The first step I took was to extract a new method from the body of the case statement.
Before
# app/helpers/issues_helper.rb
module IssuesHelper
def show_detail(detail, no_html=false)
case detail.property
when 'attr'
label = l(("field_" + detail.prop_key.to_s.gsub(/\_id$/, "")).to_sym)
case detail.prop_key
when 'due_date', 'start_date'
value = format_date(detail.value.to_date) if detail.value
old_value = format_date(detail.old_value.to_date) if detail.old_value
when 'project_id'
p = Project.find_by_id(detail.value) and value = p.name if detail.value
p = Project.find_by_id(detail.old_value) and old_value = p.name if detail.old_value
when 'status_id'
s = IssueStatus.find_by_id(detail.value) and value = s.name if detail.value
s = IssueStatus.find_by_id(detail.old_value) and old_value = s.name if detail.old_value
when 'tracker_id'
t = Tracker.find_by_id(detail.value) and value = t.name if detail.value
t = Tracker.find_by_id(detail.old_value) and old_value = t.name if detail.old_value
when 'assigned_to_id'
u = User.find_by_id(detail.value) and value = u.name if detail.value
u = User.find_by_id(detail.old_value) and old_value = u.name if detail.old_value
when 'priority_id'
e = IssuePriority.find_by_id(detail.value) and value = e.name if detail.value
e = IssuePriority.find_by_id(detail.old_value) and old_value = e.name if detail.old_value
when 'category_id'
c = IssueCategory.find_by_id(detail.value) and value = c.name if detail.value
c = IssueCategory.find_by_id(detail.old_value) and old_value = c.name if detail.old_value
when 'fixed_version_id'
v = Version.find_by_id(detail.value) and value = v.name if detail.value
v = Version.find_by_id(detail.old_value) and old_value = v.name if detail.old_value
when 'estimated_hours'
value = "%0.02f" % detail.value.to_f unless detail.value.blank?
old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank?
end
when 'cf'
custom_field = CustomField.find_by_id(detail.prop_key)
if custom_field
label = custom_field.name
value = format_value(detail.value, custom_field.field_format) if detail.value
old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value
end
when 'attachment'
label = l(:label_attachment)
end
# ... more implementation
end
end
After
module IssuesHelper
def show_detail(detail, no_html=false)
case detail.property
when 'attr'
field = detail.prop_key.to_s.gsub(/\_id$/, "")
label = l(("field_" + field).to_sym)
case detail.prop_key
when 'due_date', 'start_date'
value = format_date(detail.value.to_date) if detail.value
old_value = format_date(detail.old_value.to_date) if detail.old_value
when 'project_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'status_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'tracker_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'assigned_to_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'priority_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'category_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'fixed_version_id'
value = find_name_by_reflection(field, detail.value)
old_value = find_name_by_reflection(field, detail.old_value)
when 'estimated_hours'
value = "%0.02f" % detail.value.to_f unless detail.value.blank?
old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank?
end
when 'cf'
custom_field = CustomField.find_by_id(detail.prop_key)
if custom_field
label = custom_field.name
value = format_value(detail.value, custom_field.field_format) if detail.value
old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value
end
when 'attachment'
label = l(:label_attachment)
end
# ... more implementation
end
# Find the name of an associated record stored in the field attribute
def find_name_by_reflection(field, id)
association = Issue.reflect_on_association(field.to_sym)
if association
record = association.class_name.constantize.find_by_id(id)
return record.name if record
end
end
end
Review
This refactoring changed the case statement so that most of the body methods are identical. This will make tomorrow's refactoring even easier, since the duplication is now really visible.
I think #find_name_by_reflection has an interesting implementation. It uses ActiveRecord's #reflect_on_association which is a very easy way to dynamically walk the association tree for classes. Here I used it to dynamically get the class of a model in order to fetch it's record. Another useful place for #reflect_on_association is in unit tests. I've worked on a project a long time ago that used this with RSpec to provide custom matchers for associations (e.g. Class.should belong_to(:other_class)).
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on March 05, 2010
0 Comments
The Refactoring
Today is the day that I finally get to finish the set of refactorings I started back on the 24th. My goal was to start converting the IssuesController into a skinny controller by moving code into the models. Today, I finally was able to cleanly move #issue_update from the controller into the Issue model.
Before
# app/controllers/issues_controller.rb
def edit
update_issue_from_params
@journal = @issue.current_journal
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
update_issue_from_params
if issue_update
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
else
@journal = @issue.current_journal
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments[:files].each(&:destroy) if attachments[:files]
end
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @issue.current_journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal})
if @issue.save
if !@issue.current_journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal})
return true
end
end
# failure, returns false
end
end
# app/models/issue.rb
class Issue < ActiveRecord::Base
# ...
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
update_issue_from_params
@journal = @issue.current_journal
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
update_issue_from_params
if @issue.save_issue_with_child_records(params, @time_entry)
render_attachment_warning_if_needed(@issue)
if !@issue.current_journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
else
render_attachment_warning_if_needed(@issue)
if !@issue.current_journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
@journal = @issue.current_journal
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments[:files].each(&:destroy) if attachments[:files]
end
end
# app/models/issue.rb
class Issue < ActiveRecord::Base
# Saves an issue, time_entry, attachments, and a journal from the parameters
def save_issue_with_child_records(params, existing_time_entry=nil)
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, project)
@time_entry = existing_time_entry || TimeEntry.new
@time_entry.project = project
@time_entry.issue = self
@time_entry.user = User.current
@time_entry.spent_on = Date.today
@time_entry.attributes = params[:time_entry]
self.time_entries << @time_entry
end
if valid?
attachments = Attachment.attach_files(self, params[:attachments])
attachments[:files].each {|a| @current_journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
# TODO: Rename hook
Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
if save
# TODO: Rename hook
Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
return true
end
end
# failure, returns false
end
end
Review
In order to complete this move I had move some of the code for setting the flash messages into the action, but that's fine by me. The resulting code is a bit bigger than when it was in the controller, but I feel that it's in a place where it can be better used now.
The other pending problem is that with this move, the plugin hooks are now misnamed (Redmine::Hook.call_hook). I know there are plugins using them so I don't want to just rename them and break the code. I'm thinking about adding a new Hook method for deprecated hooks that will let them continue to function but warn the plugin developers that they will be removed later. Has anyone else working with deprecating an API? How did you do it?
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on March 04, 2010
0 Comments
The Refactoring
I'm still hammering on #issue_update today. It's almost to the point where I can move the entire method down into the Issue model but it's creating too many instance variables that view needs. This refactoring moves one of those variables up into the higher level #edit and #update actions.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
update_issue_from_params
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
update_issue_from_params
if issue_update
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
else
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments[:files].each(&:destroy) if attachments[:files]
end
def update_issue_from_params
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
@journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
end
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
update_issue_from_params
@journal = @issue.current_journal
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
update_issue_from_params
if issue_update
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
else
@journal = @issue.current_journal
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments[:files].each(&:destroy) if attachments[:files]
end
def update_issue_from_params
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
@issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
end
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @issue.current_journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal})
if @issue.save
if !@issue.current_journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal})
return true
end
end
# failure, returns false
end
end
Review
Using replace temp with query I was able to replace the temp (@journal) with a query for the object (@issue.current_journal). I still have to set the temp in the actions but now it's used as a way to access the object instead of doing actual work on it. I think I'll be able to finish this refactoring set soon, I only see a couple more refactorings left on #issue_update.
Reference commit
Tagged: ruby redmine refactoring replace-temp-with-query
written by Eric Davis on March 03, 2010
0 Comments
Ever since I started my Daily Refactorings I've been noticing more and more little things to improve code. The nice thing is that I've built up the confidence to tackle the easy ones the moment I see them. Like this one I found while working on a new Redmine plugin for a client. It's nothing big, but it fixed some duplication in Redmine that would have caused me to duplicate even more code in my plugin.
If I can keep this up for a few more months, I think I'll be able to handle even the largest refactorings and code bases with ease.
Tagged: ruby refactoring
written by Eric Davis on March 03, 2010
0 Comments
The Refactoring
After thinking about yesterday's refactor, I noticed that setting the flash message is going to cause problems for future refactorings since it's trying to break the MVC constraints. Today I found a way to decouple tracking the failed saves while keeping the flash messages up in the controllers.
Before
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
end
end
end
end
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
def self.attach_files(obj, attachments)
attached = []
unsaved = []
flash = nil
if attachments && attachments.is_a?(Hash)
attachments.each_value do |attachment|
file = attachment['file']
next unless file && file.size > 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached << a)
end
if unsaved.any?
flash = l(:warning_attachments_not_saved, unsaved.size)
end
end
{:files => attached, :flash => flash, :unsaved => unsaved}
end
end
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
attr_accessor :unsaved_attachments
after_initialize :initialize_unsaved_attachments
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
def initialize_unsaved_attachments
@unsaved_attachments ||= []
end
end
end
end
end
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
def self.attach_files(obj, attachments)
attached = []
unsaved = []
if attachments && attachments.is_a?(Hash)
attachments.each_value do |attachment|
file = attachment['file']
next unless file && file.size > 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (obj.unsaved_attachments << a) : (attached << a)
end
end
{:files => attached, :unsaved => obj.unsaved_attachments}
end
end
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# Renders a warning flash if obj has unsaved attachments
def render_attachment_warning_if_needed(obj)
flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size) if obj.unsaved_attachments.present?
end
end
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
There's a lot going on here, so I'm going to try to break it down into smaller chunks.
acts_as_attachable.rb had a new accessor added, an array to track unsaved_attachments.
Attachment#attach_files had all of it's flash tracking removed and in it's place it will store attachments that didn't save to the object's unsaved_attachments.
- In the controllers (
IssuesController): instead of checking the response hash from Attachment#attach_files for a flash message, it's checking if there is anything in object#unsaved_attachments and displaying the flash.
- ApplicationController had a utility method
#render_attachment_warning_if_needed to abstract the "display a flash" logic. This was needed because there are 5 controllers using Attachment#attach_files and I didn't want to duplicate the code in each one.
Now I think I'll be able to finish refactoring IssuesController#issue_update by pushing the logic down to the model.
Reference commit
Tagged: ruby redmine refactoring
written by Eric Davis on March 02, 2010
0 Comments
The Refactoring
Today I used move method to refactor part of Redmine that had a TODO comment since 2007.
Before
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# TODO: move to model
def attach_files(obj, attachments)
attached = []
unsaved = []
if attachments && attachments.is_a?(Hash)
attachments.each_value do |attachment|
file = attachment['file']
next unless file && file.size > 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached << a)
end
if unsaved.any?
flash[:warning] = l(:warning_attachments_not_saved, unsaved.size)
end
end
attached
end
end
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
# ...
end
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
def self.attach_files(obj, attachments)
attached = []
unsaved = []
flash = nil
if attachments && attachments.is_a?(Hash)
attachments.each_value do |attachment|
file = attachment['file']
next unless file && file.size > 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached << a)
end
if unsaved.any?
flash = l(:warning_attachments_not_saved, unsaved.size)
end
end
{:files => attached, :flash => flash, :unsaved => unsaved}
end
end
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
I'm not completely happy with the results of this refactoring. The previous ApplicationController#attach_files accessed the flash directly so I had to do some tricks to keep the flash messages in the model. This is why the callers have to check attachments[:flash] themselves. It would be ideal if the model could handle this directly so no one forgets to check the flash value, but that will have to wait until later.
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on March 01, 2010
0 Comments
The Refactoring
Now that I have a single method to start refactoring, it's time to start digging into #issue_update. The first smell I see is that the method is working on an Issue, TimeEntry, and Attachments all at once, jumping between the three procedurally. So first I needed to separate these three objects.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def issue_update
if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
Now it's easier to see the section that deals with a TimeEntry. Using ActiveRecord I was able to store the unsaved TimeEntry onto the @issue so that when @issue.save is called they are both saved. The TimeEntry section can now be refactored to a separate method to further simplify the logic (maybe even refactored to a method on the Model).
Reference commit
Tagged: ruby redmine refactoring
written by Eric Davis on February 26, 2010
0 Comments
The Refactoring
This refactoring is what I call the magic trick refactor; it looks like a lot is going on but in reality it's just smoke and mirrors!
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def update
update_issue_from_params
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments.each(&:destroy)
end
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def update
update_issue_from_params
if issue_update
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
else
respond_to do |format|
format.html { render :action => 'edit' }
format.xml { render :xml => @issue.errors, :status => :unprocessable_entity }
end
end
rescue ActiveRecord::StaleObjectError
# Optimistic locking exception
flash.now[:error] = l(:notice_locking_conflict)
# Remove the previously added attachments if issue was not updated
attachments.each(&:destroy)
end
# ...
# TODO: Temporary utility method for #update. Should be split off
# and moved to the Issue model (accepts_nested_attributes_for maybe?)
# TODO: move attach_files to the model so this can be moved to the
# model also
def issue_update
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
Truthfully, this refactor isn't just smoke and mirrors. The behavior stayed the same but now #update is easier to read and is working at a higher level of abstraction (e.g. only handling the response). The #issue_update method is still pretty smelly but it's just about ripe for some refactoring.
Reference commit
Tagged: ruby redmine refactoring extract-method