Daily Refactor #18: Extract method in AuthSourceLdap

written by Eric Davis on February 17, 2010 0 Comments

The Refactoring

AuthSourceLdap#authenticate still can use some more refactoring. I performed another extract method, this time to create a method to authenticate an DN (Distinguished Name).

Before

# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource 
  # ...
  def authenticate(login, password)
    return nil if login.blank? || password.blank?
    attrs = []
    # get user's DN
    ldap_con = initialize_ldap_con(self.account, self.account_password)
    login_filter = Net::LDAP::Filter.eq( self.attr_login, login ) 
    object_filter = Net::LDAP::Filter.eq( "objectClass", "*" ) 
    dn = String.new
    ldap_con.search( :base => self.base_dn, 
                     :filter => object_filter & login_filter, 
                     # only ask for the DN if on-the-fly registration is disabled
                     :attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
      dn = entry.dn
      attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?

    end
    return nil if dn.empty?
    logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
    # authenticate user
    ldap_con = initialize_ldap_con(dn, password)
    return nil unless ldap_con.bind
    # return user's attributes
    logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
    attrs    
  rescue  Net::LDAP::LdapError => text
    raise "LdapError: " + text
  end
  # ...
end

After

# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource 
  # ...
  def authenticate(login, password)
    return nil if login.blank? || password.blank?
    attrs = []
    # get user's DN
    ldap_con = initialize_ldap_con(self.account, self.account_password)
    login_filter = Net::LDAP::Filter.eq( self.attr_login, login ) 
    object_filter = Net::LDAP::Filter.eq( "objectClass", "*" ) 
    dn = String.new
    ldap_con.search( :base => self.base_dn, 
                     :filter => object_filter & login_filter, 
                     # only ask for the DN if on-the-fly registration is disabled
                     :attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
      dn = entry.dn
      attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?

    end
    return nil if dn.empty?
    logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?

    if authenticate_dn(dn, password)
      logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
      return attrs
    else
      return nil
    end
  rescue  Net::LDAP::LdapError => text
    raise "LdapError: " + text
  end

  # Check if a DN (user record) authenticates with the password
  def authenticate_dn(dn, password)
    ldap_con = initialize_ldap_con(dn, password)
    return ldap_con.bind
  end
  # ...
end

Review

Although it ended up adding more code than it took away, this refactoring made it more clear how a user (DN) is authenticated. Before, the method used guard clauses to control the logic which made it difficult to understand when they would be triggered. Now it's easier to see the two different code paths because they are controlled by the if authenticate_dn statement.

Also, like yesterday's refactoring, this adds a new private method that can be used by other methods in the class instead of duplicating the code.

Reference commit

Tagged: ruby redmine refactoring extract-method

Daily Refactor #17: Extract method in AuthSourceLdap

written by Eric Davis on February 16, 2010 0 Comments

Since I've been doing a lot of work on a Redmine plugin to add more LDAP features, I decided to use today to refactor some of Redmine's LDAP code.

The Refactoring

I just performed a simple extract method refactoring on the authenticate method in AuthSourceLdap to make it easier to understand what it does with the search results.

Before

# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource 
  # ...
  def authenticate(login, password)
    return nil if login.blank? || password.blank?
    attrs = []
    # get user's DN
    ldap_con = initialize_ldap_con(self.account, self.account_password)
    login_filter = Net::LDAP::Filter.eq( self.attr_login, login ) 
    object_filter = Net::LDAP::Filter.eq( "objectClass", "*" ) 
    dn = String.new
    ldap_con.search( :base => self.base_dn, 
                     :filter => object_filter & login_filter, 
                     # only ask for the DN if on-the-fly registration is disabled
                     :attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
      dn = entry.dn
      attrs = [:firstname => AuthSourceLdap.get_attr(entry, self.attr_firstname),
               :lastname => AuthSourceLdap.get_attr(entry, self.attr_lastname),
               :mail => AuthSourceLdap.get_attr(entry, self.attr_mail),
               :auth_source_id => self.id ] if onthefly_register?
    end
    return nil if dn.empty?
    logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
    # authenticate user
    ldap_con = initialize_ldap_con(dn, password)
    return nil unless ldap_con.bind
    # return user's attributes
    logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
    attrs    
  rescue  Net::LDAP::LdapError => text
    raise "LdapError: " + text
  end
  # ...
end

After

# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource 
  # ...
  def authenticate(login, password)
    return nil if login.blank? || password.blank?
    attrs = []
    # get user's DN
    ldap_con = initialize_ldap_con(self.account, self.account_password)
    login_filter = Net::LDAP::Filter.eq( self.attr_login, login ) 
    object_filter = Net::LDAP::Filter.eq( "objectClass", "*" ) 
    dn = String.new
    ldap_con.search( :base => self.base_dn, 
                     :filter => object_filter & login_filter, 
                     # only ask for the DN if on-the-fly registration is disabled
                     :attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
      dn = entry.dn
      attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?

    end
    return nil if dn.empty?
    logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
    # authenticate user
    ldap_con = initialize_ldap_con(dn, password)
    return nil unless ldap_con.bind
    # return user's attributes
    logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
    attrs    
  rescue  Net::LDAP::LdapError => text
    raise "LdapError: " + text
  end

  def get_user_attributes_from_ldap_entry(entry)
    [
     :firstname => AuthSourceLdap.get_attr(entry, self.attr_firstname),
     :lastname => AuthSourceLdap.get_attr(entry, self.attr_lastname),
     :mail => AuthSourceLdap.get_attr(entry, self.attr_mail),
     :auth_source_id => self.id
    ]
  end

  # ...
end

Review

This refactoring served two purposes:

  1. It shortened AuthSourceLdap#authenticate, which is difficult enough to understand
  2. The utility method made it easy to get the user attributes from an LDAP entry; which I can use in two parts of my redmine_extra_ldap plugin

Reference commit

Tagged: ruby redmine refactoring extract-method

Daily Refactor #16: Rename method in RolesController

written by Eric Davis on February 15, 2010 0 Comments

The Refactoring

I'm continuing last Friday's refactor today, but this time renaming the method in RolesController.

Before

# app/controllers/roles_controller.rb
class RolesController < ApplicationController
  layout 'admin'
  
  before_filter :require_admin

  verify :method => :post, :only => [ :destroy, :move ],
         :redirect_to => { :action => :list }

  def index
    list
    render :action => 'list' unless request.xhr?
  end

  def list
    @role_pages, @roles = paginate :roles, :per_page => 25, :order => 'builtin, position'
    render :action => "list", :layout => false if request.xhr?
  end

  # ...
end

After

# app/controllers/roles_controller.rb
class RolesController < ApplicationController
  layout 'admin'
  
  before_filter :require_admin

  verify :method => :post, :only => [ :destroy, :move ],
         :redirect_to => { :action => :index }

  def index
    @role_pages, @roles = paginate :roles, :per_page => 25, :order => 'builtin, position'
    render :action => "index", :layout => false if request.xhr?
  end

  # ...
end

Review

I also applied this refactoring to the other controllers that duplicated the code. The result is the same for each so I'm not going to bother post each one but you can see the changes for the AuthSourcesController and TrackersController on Github. This should remove an entire duplication set in flay and cleanup some older Redmine code.

Reference commit

Tagged: ruby redmine refactoring rename-method

Daily Refactor #15: Rename method in IssueStatusesController

written by Eric Davis on February 12, 2010 0 Comments

It's time to move on from the ReportsController to code that needs to be refactored more.

Using Caliper's Flay Report, it looks like IssueStatusesController, TrackersController, AuthSourcesController, and RolesController all have the exact same index action.

  def index
    list
    render :action => 'list' unless request.xhr?
  end

While I could just use pull up method to remove this duplication, there is a stronger smell here that I'd like to fix...

The Refactoring

In Rails the index action is generally used to list a collection of records, typically with some pagination. These four controllers are using the list action for that though and have the index action just run list. This is way too much redirection and it can be simplified.

Before

# app/controllers/issue_statuses_controller.rb
class IssueStatusesController < ApplicationController
  layout 'admin'
  
  before_filter :require_admin

  verify :method => :post, :only => [ :destroy, :create, :update, :move, :update_issue_done_ratio ],
         :redirect_to => { :action => :list }
         
  def index
    list
    render :action => 'list' unless request.xhr?
  end

  def list
    @issue_status_pages, @issue_statuses = paginate :issue_statuses, :per_page => 25, :order => "position"
    render :action => "list", :layout => false if request.xhr?
  end

  # ...
end

After

# app/controllers/issue_statuses_controller.rb
class IssueStatusesController < ApplicationController
  layout 'admin'
  
  before_filter :require_admin

  verify :method => :post, :only => [ :destroy, :create, :update, :move, :update_issue_done_ratio ],
         :redirect_to => { :action => :index }
         
  def index
    @issue_status_pages, @issue_statuses = paginate :issue_statuses, :per_page => 25, :order => "position"
    render :action => "index", :layout => false if request.xhr?
  end

  # ...
end

Review

Now IssueStatusesController only has the index action, which follows the Rails standards. This will also make it easier to add on a RESTful web service later on.

Reference commit (includes the changes to the tests and redirects that used list)

Tagged: ruby redmine refactoring rename-method

Daily Refactor #14: Extract method to before_filter in ReportsController

written by Eric Davis on February 11, 2010 0 Comments

The Refactoring

I started on a larger refactoring of the issue_report_details method today but found a potential security bug so I threw it away. It's better to be secure than refactored :)

So instead, I performed a common Rails refactoring for removing duplicate code: extracting some code to a before filter.

Before

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
    
    # ...
  end  

  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    # ...
  end

end

After

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize, :find_issue_statuses

  def issue_report
    # ...
  end  

  def issue_report_details
    # ...
  end

  private

  def find_issue_statuses
    @statuses = IssueStatus.find(:all, :order => 'position')
  end
end

Review

This is a good refactoring when your controller actions need some data but that data isn't their core responsibility. For example the find_project and authorize before filters in Redmine will find the current project, user, and check that the permissions allow access. These don't have anything to do with the actions, but they are still required.

Reference commit

Tagged: ruby redmine refactoring extract-method

Daily Refactor #13: Inline utility methods in ReportsController

written by Eric Davis on February 10, 2010 0 Comments

The Refactoring

Today I used the inline method refactoring to make the ReportsController even more concise by removing all of the utility methods.

Before

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
    
    @trackers = @project.trackers
    @versions = @project.shared_versions.sort
    @priorities = IssuePriority.all
    @categories = @project.issue_categories
    @assignees = @project.members.collect { |m| m.user }.sort
    @authors = @project.members.collect { |m| m.user }.sort
    @subprojects = @project.descendants.active
    issues_by_tracker
    issues_by_version
    issues_by_priority
    issues_by_category
    issues_by_assigned_to
    issues_by_author
    issues_by_subproject
      
    render :template => "reports/issue_report"
  end  

  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
    end

    respond_to do |format|
      if @field
        format.html {}
      else
        format.html { redirect_to :action => 'issue_report', :id => @project }
      end
    end
  end
private
  def issues_by_tracker
    @issues_by_tracker ||= Issue.by_tracker(@project)
  end

  def issues_by_version
    @issues_by_version ||= Issue.by_version(@project)
  end
    
  def issues_by_priority    
    @issues_by_priority ||= Issue.by_priority(@project)
  end
    
  def issues_by_category   
    @issues_by_category ||= Issue.by_category(@project)
  end
  
  def issues_by_assigned_to
    @issues_by_assigned_to ||= Issue.by_assigned_to(@project)
  end
  
  def issues_by_author
    @issues_by_author ||= Issue.by_author(@project)
  end
  
  def issues_by_subproject
    @issues_by_subproject ||= Issue.by_subproject(@project)
    @issues_by_subproject ||= []
  end
end

After

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
    
    @trackers = @project.trackers
    @versions = @project.shared_versions.sort
    @priorities = IssuePriority.all
    @categories = @project.issue_categories
    @assignees = @project.members.collect { |m| m.user }.sort
    @authors = @project.members.collect { |m| m.user }.sort
    @subprojects = @project.descendants.active

    @issues_by_tracker = Issue.by_tracker(@project)
    @issues_by_version = Issue.by_version(@project)
    @issues_by_priority = Issue.by_priority(@project)
    @issues_by_category = Issue.by_category(@project)
    @issues_by_assigned_to = Issue.by_assigned_to(@project)
    @issues_by_author = Issue.by_author(@project)
    @issues_by_subproject = Issue.by_subproject(@project) || []

    render :template => "reports/issue_report"
  end  

  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = Issue.by_tracker(@project)
      @report_title = l(:field_tracker)
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = Issue.by_version(@project)
      @report_title = l(:field_version)
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = Issue.by_priority(@project)
      @report_title = l(:field_priority)
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = Issue.by_category(@project)
      @report_title = l(:field_category)
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = Issue.by_assigned_to(@project)
      @report_title = l(:field_assigned_to)
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = Issue.by_author(@project)
      @report_title = l(:field_author)
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = Issue.by_subproject(@project) || []
      @report_title = l(:field_subproject)
    end

    respond_to do |format|
      if @field
        format.html {}
      else
        format.html { redirect_to :action => 'issue_report', :id => @project }
      end
    end
  end

end

Review

This refactoring shows that extracting methods isn't always the best approach. Since the utility methods' implementation did exactly what the name said, it's clearer to remove the utility method and inline the implementation directly in the caller. This was made possible because I moved most of implementation to the Model in a previous refactoring.

This also cleans up a subtle side effect in the issue_report_details action. Extra instance variables were being created when calling the utility methods even though the results was stored to @data. Though it's small, this would use resources and would expose those instance variables to the view.

Reference commit

Tagged: ruby redmine refactoring inline-method

Daily Refactor #12: Extract rendering in the case statement

written by Eric Davis on February 09, 2010 0 Comments

It seems like every time I try to apply a big refactoring, there are parts that are missed. I tried to refactor the entire case statement in issue_report_details into a common data structure and a lookup method. But it ended up being more complex than the case statement was so I threw it away and started over. Once again, the simple iterative process wins over the big rewrite.

The Refactoring

Before

# app/controllers/reports_controller.rb
  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      redirect_to :action => 'issue_report', :id => @project
    end
  end

After

# app/controllers/reports_controller.rb
  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
    end

    respond_to do |format|
      if @field
        format.html {}
      else
        format.html { redirect_to :action => 'issue_report', :id => @project }
      end
    end
  end

Review

Based on the raw lines of code changed, there wasn't a lot of duplicated code removed (7 added, 9 removed). But it does really clean up how the action reads. Before the action would:

  1. collect the data and do the rendering inside each case

Now the case statement

  1. collects the data
  2. does the rendering

This also means that the entire case statement can be moved out of the public action into a utility method.

Reference commit

Tagged: ruby redmine refactoring extract-rendering

Daily Refactor #11: Extract action in ReportsController

written by Eric Davis on February 08, 2010 0 Comments

I'm back up in the ReportsController now, getting ready to tackle some more of the duplication.

The Refactoring

Before

# config/routes.rb
  map.with_options :controller => 'reports', :action => 'issue_report', :conditions => {:method => :get} do |reports|
    reports.connect 'projects/:id/issues/report'
    reports.connect 'projects/:id/issues/report/:detail'
  end

# app/controllers/reports_controller.rb
  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
    
    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      @trackers = @project.trackers
      @versions = @project.shared_versions.sort
      @priorities = IssuePriority.all
      @categories = @project.issue_categories
      @assignees = @project.members.collect { |m| m.user }.sort
      @authors = @project.members.collect { |m| m.user }.sort
      @subprojects = @project.descendants.active
      issues_by_tracker
      issues_by_version
      issues_by_priority
      issues_by_category
      issues_by_assigned_to
      issues_by_author
      issues_by_subproject
      
      render :template => "reports/issue_report"
    end
  end  

private
  # ...
end

After

# config/routes.rb
  map.with_options :controller => 'reports', :conditions => {:method => :get} do |reports|
    reports.connect 'projects/:id/issues/report', :action => 'issue_report'
    reports.connect 'projects/:id/issues/report/:detail', :action => 'issue_report_details'
  end

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
    
    @trackers = @project.trackers
    @versions = @project.shared_versions.sort
    @priorities = IssuePriority.all
    @categories = @project.issue_categories
    @assignees = @project.members.collect { |m| m.user }.sort
    @authors = @project.members.collect { |m| m.user }.sort
    @subprojects = @project.descendants.active
    issues_by_tracker
    issues_by_version
    issues_by_priority
    issues_by_category
    issues_by_assigned_to
    issues_by_author
    issues_by_subproject
      
    render :template => "reports/issue_report"
  end  

  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')

    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      redirect_to :action => 'issue_report', :id => @project
    end

  end
private
  # ...
end

Review

This refactoring split the issue_report action into two separate actions: issue_report and issue_report_details. I did this for a few reasons:

  • I felt like issue_report was trying to do too much. It would generate a summary report and also one of 7 details reports.
  • Having one large action in a controller gets away from the standard RESTful design principles in Rails.
  • Having the implementation hidden inside of a large case statement made it more difficult to understand what was happening.

Now I can start to really remove the duplication from the internals of the actions. It's a good thing I've been building up the unit test suite as I've gone; the safety net of the tests will be valuable over the next few refactorings.

Reference commit

Tagged: ruby redmine refactoring extract-action

Daily Refactor #10: Pull up the #find_project method to the ApplicationController

written by Eric Davis on February 05, 2010 0 Comments

Back up in the ReportsController, there is still some duplication that needs to be resolved. There are nine methods:

  • 1 controller action (issue_report)
  • 1 private before_filter method (find_project)
  • 7 private utility methods

The controller action is ripe for a few refactorings but I wanted to start with the find_project for personal reasons. I used the pull up method to move find_project to the superclass (ApplicationController).

The Refactoring

Before

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  private
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])        
  rescue ActiveRecord::RecordNotFound
    render_404
  end

  # ... other private methods
end

After

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize

  private
  # ... other private methods
end

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

Review

Just looking at the ReportsController, this refactoring doesn't look that effective. But I didn't include the four other controllers in Redmine that had the exact same find_project method. Or the dozen plugins where I've had to implement an identical find_project.

So now ReportsController is starting to clean up quite nicely. Next week I should be able start refactoring the large controller action and clean up a lot of the internal duplication. I'm even thinking of splitting up the action a bit to remove the case statement.

Reference commit

Tagged: ruby redmine refactoring pull-up-method

Why I don't use HAML or SASS

written by Eric Davis on February 04, 2010 0 Comments

Seeing as the ERB vs HAML decision has now reached the armed conflict status, I figured that I would voice my opinions before it becomes yet another holy war.

HAML, SASS, and ERB are all tools. From what I've seen, they are all very good quality and they all have their shortcomings. You can write "brief elegant code" and "horrible spaghetti code that should never see the light of day" in them all.

But I choose ERB for all of my projects and anything I open source:

  1. The #1 reason I use ERB is because 99% of the development I do is with existing code base in ERB. Sure, ERB and HAML can be mixed but that's not a road I want to go down. I've already tried to mix Test::Unit and RSpec in a few projects and it's not maintainable at all (Test::Unit in the core code, RSpec in the plugins).
  2. Even in a brand new project, I will not use HAML if that project is going to be open source. Since ERB comes with Ruby and Rails, there is no additional requirement to getting it installed and setup. HAML and SASS both need additional gems installed in order to work. For Rails developers this might be easy but how "easy" would this be to explain to a non Rails developer who wants to use your software? Could you explain how to install HAML and SASS to your great Aunt Tillie?
  3. I've personally found that SASS doesn't play well with several deployment environments. Having to compile your CSS when deploying is yet another barrier to getting your software setup. I was also surprised at how many Rails applications are deployed to a environment with a read only public directory. This is not just on Heroku, it's also in many enterprise IT shops.

That said, HAML and SASS both have a lot of good ideas. I've only worked on trivial HAML applications but the views does look cleaner than ERB. I'd think that in more complex applications, HAML could get just as messy as ERB though. I also really like how SASS lets you add some dynamic bits to your CSS. I haven't found a really elegant solution to this yet using ERB, other than dumping inline CSS for the dynamic bits.

Like any opinions, I reserve the right to change this one at any time. But I will need something to change before that will happen. Maybe I just need someone to show me the ropes with HAML and SASS.

And if HAML, SASS, or ERB are working great for you, then keep using them and make them better.

Tagged: rails sass haml ruby