Daily Refactor #32: Inline Method in IssuesHelper#show_detail

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

Launching See Project Run

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

Daily Refactor #31: Extract Method in IssuesHelper#show_detail

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

Daily Refactor #30: Move Method into Issue

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

Daily Refactor #29: Replace Temp with Query in IssuesController#issue_update

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

Noticing more refactorings

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

Daily Refactor #28: Decouple flash messages from Attachment#attach_files

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

Daily Refactor #27: Move Method to the Attachment Model

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

Daily Refactor #26: Convert Procedural Design to Objects in #issue_update

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

Daily Refactor #25: Extract method in IssuesController#update

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

Page 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

The Refactoring

Before

After

Review

Subscribe to Updates

Recent Articles

Connect with me on