written by Eric Davis on February 04, 2010
0 Comments
Yesterday's refactoring to the ReportsController helped to move some of the ActiveRecord::Base#select_all methods out of the controller but I was still left with the duplication in the Issue model. Today, I'll dig into those model methods and remove most of the duplication.
The Refactoring
Before
# app/models/issue.rb
class Issue < ActiveRecord::Base
def self.by_tracker(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
t.id as tracker_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t
where
i.status_id=s.id
and i.tracker_id=t.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, t.id")
end
def self.by_version(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
v.id as fixed_version_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v
where
i.status_id=s.id
and i.fixed_version_id=v.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, v.id")
end
def self.by_priority(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
p.id as priority_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p
where
i.status_id=s.id
and i.priority_id=p.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, p.id")
end
def self.by_category(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
c.id as category_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c
where
i.status_id=s.id
and i.category_id=c.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, c.id")
end
def self.by_assigned_to(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as assigned_to_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.assigned_to_id=a.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, a.id")
end
def self.by_author(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as author_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.author_id=a.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, a.id")
end
After
# app/models/issue.rb
class Issue < ActiveRecord::Base
def self.by_tracker(project)
count_and_group_by(:project => project,
:field => 'tracker_id',
:joins => Tracker.table_name)
end
def self.by_version(project)
count_and_group_by(:project => project,
:field => 'fixed_version_id',
:joins => Version.table_name)
end
def self.by_priority(project)
count_and_group_by(:project => project,
:field => 'priority_id',
:joins => IssuePriority.table_name)
end
def self.by_category(project)
count_and_group_by(:project => project,
:field => 'category_id',
:joins => IssueCategory.table_name)
end
def self.by_assigned_to(project)
count_and_group_by(:project => project,
:field => 'assigned_to_id',
:joins => User.table_name)
end
def self.by_author(project)
count_and_group_by(:project => project,
:field => 'author_id',
:joins => User.table_name)
end
# ...
private
# Query generator for selecting groups of issue counts for a project
# based on specific criteria
#
# Options
# * project - Project to search in.
# * field - String. Issue field to key off of in the grouping.
# * joins - String. The table name to join against.
def self.count_and_group_by(options)
project = options.delete(:project)
select_field = options.delete(:field)
joins = options.delete(:joins)
where = "i.#{select_field}=j.id"
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
j.id as #{select_field},
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{joins} as j
where
i.status_id=s.id
and #{where}
and i.project_id=#{project.id}
group by s.id, s.is_closed, j.id")
end
end
Review
The big duplication was from the 6 select_all statements that only differed in what fields are used and what table to join. So I moved the general code to a separate method, provided parameters for the parts of the statement that would change, and built the select_all statement based on a simpler set of options. This allowed me to slim down the public methods considerably.
So now it would be trivial to add new public methods to count and group based on other fields. Also if the SQL needs to be rewritten at any point in the future, it only needs to be done in one place, not six.
This is as far as I want to take the model refactoring at this time. I'll be heading back up to the controllers to refactor some more duplication up there. Duplication in controllers will affect an application more than duplication in models because a user tends to touch the controllers directly.
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on February 03, 2010
0 Comments
The Refactoring
This is the first part to a larger refactoring of the ReportsController in Redmine. This controller is responsible for calculating the total number of issues based on different criteria (e.g. issue status, issue priority). There are two big problems with the current code:
- It has a lot of duplication, both in the main methods and the utility methods
- It has raw query methods in the controller (
ActiveRecord::Base.connection.select_all)
So the first step will be to move all of the raw query methods out of the controller and into the Issue model where they belong.
(I've included the entire ReportsController to make it easier to see how much of it will be changed. Don't let your eyes glaze over in awe over this code.)
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')
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
# Find project of id params[:id]
def find_project
@project = Project.find(params[:id])
rescue ActiveRecord::RecordNotFound
render_404
end
def issues_by_tracker
@issues_by_tracker ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
t.id as tracker_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t
where
i.status_id=s.id
and i.tracker_id=t.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, t.id")
end
def issues_by_version
@issues_by_version ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
v.id as fixed_version_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v
where
i.status_id=s.id
and i.fixed_version_id=v.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, v.id")
end
def issues_by_priority
@issues_by_priority ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
p.id as priority_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p
where
i.status_id=s.id
and i.priority_id=p.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, p.id")
end
def issues_by_category
@issues_by_category ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
c.id as category_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c
where
i.status_id=s.id
and i.category_id=c.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, c.id")
end
def issues_by_assigned_to
@issues_by_assigned_to ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as assigned_to_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.assigned_to_id=a.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, a.id")
end
def issues_by_author
@issues_by_author ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as author_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.author_id=a.id
and i.project_id=#{@project.id}
group by s.id, s.is_closed, a.id")
end
def issues_by_subproject
@issues_by_subproject ||=
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
i.project_id as project_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s
where
i.status_id=s.id
and i.project_id IN (#{@project.descendants.active.collect{|p| p.id}.join(',')})
group by s.id, s.is_closed, i.project_id") if @project.descendants.active.any?
@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')
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
# Find project of id params[:id]
def find_project
@project = Project.find(params[:id])
rescue ActiveRecord::RecordNotFound
render_404
end
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
# app/models/issue.rb
class Issue < ActiveRecord::Base
# Extracted from the ReportsController.
# TODO: refactor into a common factory or named scopes
def self.by_tracker(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
t.id as tracker_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t
where
i.status_id=s.id
and i.tracker_id=t.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, t.id")
end
def self.by_version(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
v.id as fixed_version_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v
where
i.status_id=s.id
and i.fixed_version_id=v.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, v.id")
end
def self.by_priority(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
p.id as priority_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p
where
i.status_id=s.id
and i.priority_id=p.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, p.id")
end
def self.by_category(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
c.id as category_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c
where
i.status_id=s.id
and i.category_id=c.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, c.id")
end
def self.by_assigned_to(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as assigned_to_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.assigned_to_id=a.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, a.id")
end
def self.by_author(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
a.id as author_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
where
i.status_id=s.id
and i.author_id=a.id
and i.project_id=#{project.id}
group by s.id, s.is_closed, a.id")
end
def self.by_subproject(project)
ActiveRecord::Base.connection.select_all("select s.id as status_id,
s.is_closed as closed,
i.project_id as project_id,
count(i.id) as total
from
#{Issue.table_name} i, #{IssueStatus.table_name} s
where
i.status_id=s.id
and i.project_id IN (#{project.descendants.active.collect{|p| p.id}.join(',')})
group by s.id, s.is_closed, i.project_id") if project.descendants.active.any?
end
# End ReportsController extraction
end
Review
This refactor alone moved over 30% of the code to the Model and makes it easier to see that is only one public method in this entire controller. There is still a bit that needs to be done in the Issue, since all I really did was to move some code duplication down to the model. But now that the model has all this code, it's easier to see some similarities in the methods in order to extract it to a utility method.
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on February 02, 2010
0 Comments
Working off of Caliper's flay report for Redmine, I was able to refactor another code duplication.
The Refactoring
Redmine supports connecting to seven different source control systems to import their changes (e.g. git, subversion, mercurial). When Redmine runs the import, it adds each Changeset (a commit) into the database and also information about each Change (e.g. file modification, file added, etc) using the fetch_changesets method.
The fetch_changesets method for the darcs, mercurial, and subversion repositories all used the same code to add a new Change to the database. Literally, a copy an paste of one another.
Before
# app/models/changeset.rb
class Changeset < ActiveRecord::Base
# ...
end
# app/models/repository/darcs.rb
class Repository::Darcs < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => changeset,
:action => change[:action],
:path => change[:path],
:from_path => change[:from_path],
:from_revision => change[:from_revision])
end
# ...
end
# app/models/repository/mercurial.rb
class Repository::Mercurial < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => changeset,
:action => change[:action],
:path => change[:path],
:from_path => change[:from_path],
:from_revision => change[:from_revision])
end
# ...
end
# app/models/repository/subversion.rb
class Repository::Subversion < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => changeset,
:action => change[:action],
:path => change[:path],
:from_path => change[:from_path],
:from_revision => change[:from_revision])
end unless changeset.new_record?
# ...
end
After
# app/models/changeset.rb
class Changeset < ActiveRecord::Base
# Creates a new Change from it's common parameters
def create_change(change)
Change.create(:changeset => self,
:action => change[:action],
:path => change[:path],
:from_path => change[:from_path],
:from_revision => change[:from_revision])
end
end
# app/models/repository/darcs.rb
class Repository::Darcs < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
changeset.create_change(change)
end
# ...
end
# app/models/repository/mercurial.rb
class Repository::Mercurial < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
changeset.create_change(change)
end
# ...
end
# app/models/repository/subversion.rb
class Repository::Subversion < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
changeset.create_change(change)
end unless changeset.new_record?
# ...
end
Review
Even though this duplication looks like it was caused by a copy and paste, notice that the Subversion version has an extra guard to make sure the Changeset was saved. That tells me that at some point in it's history, there was a bug that needed the guard in Subversion. Sure enough git blame shows that exact conclusion in commit 7bd4590cd.
So why wasn't it ported to the other versions? Might they have similar bugs?
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on February 01, 2010
0 Comments
This week I'm planning to do some refactoring on Redmine's code base. Today I fixed a smell that Caliper found with flay.
The Refactoring
Redmine has several model objects that generate Events, which are listed on it's Activity Page and cause email notifications to be sent. A few of these models had identical recipients methods defined, which caused a Flay score of 368 (bad). Using move method I as able to move 3 of the 4 into Redmine's acts_as_event plugin. The fourth will need some more refactoring before it could be moved.
Before
# vendor/plugins/acts_as_event/lib/acts_as_event.rb
# ... nothing related to recipients
# app/models/news.rb
class News < ActiveRecord::Base
# Returns the mail adresses of users that should be notified
def recipients
notified = project.notified_users
notified.reject! {|user| !visible?(user)}
notified.collect(&:mail)
end
end
# app/models/document.rb
class Document < ActiveRecord::Base
# Returns the mail adresses of users that should be notified
def recipients
notified = project.notified_users
notified.reject! {|user| !visible?(user)}
notified.collect(&:mail)
end
end
# app/models/message.rb
class Message < ActiveRecord::Base
# Returns the mail adresses of users that should be notified
def recipients
notified = project.notified_users
notified.reject! {|user| !visible?(user)}
notified.collect(&:mail)
end
end
After
# vendor/plugins/acts_as_event/lib/acts_as_event.rb
# Returns the mail adresses of users that should be notified
def recipients
notified = project.notified_users
notified.reject! {|user| !visible?(user)}
notified.collect(&:mail)
end
# app/models/news.rb
class News < ActiveRecord::Base
# ...
end
# app/models/document.rb
class Document < ActiveRecord::Base
# ...
end
# app/models/message.rb
class Message < ActiveRecord::Base
# ...
end
Review
While this refactor is pretty simple, duplication in a large system like Redmine can really affect development. That's why Test Driven Development has that final refactor step at the end of every cycle. Hopefully this refactor will make it easier to work on Redmine's event notification system.
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on January 30, 2010
0 Comments
I wanted to share an application I created that I've been using for a few months called Sinatra Todo.
Why I built it
I've been using GTD and other time management systems for several years now and have seen myself go through two stages:
- Using high tech, shiney, whiz-bang gadgets to track my todo list. (e.g. Tracks, Emacs Planner Mode)
- Using low tech, simple tools (e.g. index cards, sheet of paper, free form todo list)
The big problem with the high tech tools is that I would spend too much time tinkering with how my tasks were organized and not enough time doing the actual tasks. The low tech tools also suffer a problem, I would become overwhelmed by the quantity of tasks and would easily loose focus and become disorganized.
Last time when I decided to go back to a low tech tool, I decided to just try to keep everything in a semi-structured text file in Emacs. I found todo-list-mode by Billy Lamberta and liked that it's just a simple Emacs mode that highlights each line of text according to the first character. So a line starting with '1' would be red (todo today), '2' would be a paler red, etc. It also highlighted hashtags at the end of lines which worked perfect for GTD contexts and simple categories. Nice and low tech.
The one downside was that it only worked in Emacs. When I'm at my desktop, I have Emacs always at hand but sometimes I'm in another part of the house or away from home and need to see my todo list. This is where Sinatra Todo comes in...
Benefits
Sinatra Todo is a stupidly simple Sinatra app that understands the format of the todo-list file and shows the contents as a set of HTML pages. It will automatically pick up all of the different priorities and tags in the file and auto link them. So if you want to see everything for the "WorldDomination" project, you can click that tag and Sinatra Todo will only show you those tasks. It doesn't require that you use Emacs or todo-list-mode either, just make your todo list follow the same format.
Getting involved
I've been really happy with sinatra_todo so far but if you would like to modify it in any way, feel free to fork it on Github and hack away. There are a things I might add in the future:
- Tests. Normally I use TDD but this started as a late night hack to see if I could throw something together.
- The ability to add a new todo item. I think this could be a simple form that will just append the content to the Todo file.
- The ability to edit an existing item. This could work like adding a new todo item but would overwrite an existing line.
- Some kind of integration with Redmine's issues. I'm thinking of just using a simple cronjob to export my issue list to a file will work though.
If you add something useful to your fork, please send me a pull request and I'll see about integrating it.
Post a comment if you try it out, I'd love to see if anyone else found this valuable.
Tagged: sinatra gtd open source
written by Eric Davis on January 29, 2010
0 Comments
Today is another simple refactoring to the BulkTimeEntry plugin's helper. A common function is needing to sort one set of data into two different sets. The quick and dirty way is to just loop over each item in the set and build up the two sets yourself. But Ruby's Arrays have another way...
The Refactoring
Before
# bulk_time_entries_helper.rb
def grouped_options_for_issues(issues)
open_issues = []
closed_issues = []
issues.each do |issue|
if issue.closed?
closed_issues << issue
else
open_issues << issue
end
end
html = '<option></option>'
unless open_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_open_issues, open_issues)
end
unless closed_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_closed_issues, closed_issues)
end
html
end
After
# bulk_time_entries_helper.rb
def grouped_options_for_issues(issues)
closed_issues, open_issues = *issues.partition {|issue| issue.closed?}
html = '<option></option>'
unless open_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_open_issues, open_issues)
end
unless closed_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_closed_issues, closed_issues)
end
html
end
Review
Ruby's Array implements the partition method (from Enumerable). This method takes a block and will return an Array of two Arrays:
- one with the elements that evaluated the block to true
- one with the elements that evaluated the block to false
Combining that with the splat (*), both sets can be assigned to the new variables. If you haven't looked at Enumerable recently, check it out. Many classes implement it in both Ruby and Rails, and it has methods for just about any operation you want to do on a set of data.
Reference commit
Tagged: ruby redmine plugins refactoring
written by Eric Davis on January 28, 2010
0 Comments
Today Devver team showed me a new feature they added to their Caliper project, the flay code view. I liked it so much I used it on today's BulkTimeEntry plugin refactoring.
The Refactoring
Flay is a tool that checks for duplicated Ruby code. The BulkTimeEntry plugin only showed one piece of code that was duplicated but it's perfect for a simple extract method.
Before
# bulk_time_entries_helper.rb
def grouped_options_for_issues(issues)
open_issues = []
closed_issues = []
issues.each do |issue|
if issue.closed?
closed_issues << issue
else
open_issues << issue
end
end
html = '<option></option>'
unless open_issues.empty?
html << "<optgroup label='#{l(:label_open_issues)}'>"
html << options_from_collection_for_select(open_issues, :id, :to_s)
html << "</optgroup>"
end
unless closed_issues.empty?
html << "<optgroup label='#{l(:label_closed_issues)}'>"
html << options_from_collection_for_select(closed_issues, :id, :to_s)
html << "</optgroup>"
end
html
end
After
# bulk_time_entries_helper.rb
def grouped_options_for_issues(issues)
open_issues = []
closed_issues = []
issues.each do |issue|
if issue.closed?
closed_issues << issue
else
open_issues << issue
end
end
html = '<option></option>'
unless open_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_open_issues, open_issues)
end
unless closed_issues.empty?
html << labeled_option_group_from_collection_for_select(:label_closed_issues, closed_issues)
end
html
end
def labeled_option_group_from_collection_for_select(label, collection)
html = "<optgroup label='#{l(label)}'>"
html << options_from_collection_for_select(collection, :id, :to_s)
html << "</optgroup>"
html
end
Review
This was a really easy refactor since the method generated the same HTML string but using a different label and collection. There are still a few minor style issues that I'll tackle next time.
If you haven't used Caliper yet, I'd recommend you give it a try. I have Redmine setup with a post commit hook so it's metrics are generated every time we commit. Having an extra hand watching code quality is always helpful.
Reference commit.
Tagged: ruby redmine plugins refactoring extract-method caliper metrics
written by Eric Davis on January 27, 2010
0 Comments
Today's "refactoring" isn't a refactoring in the classic sense, since it includes a behavior change. But if you squint your eyes enough to only see the Controller as the "public interface", then we can call it a refactor.
The Refactoring
Yesterday I moved the create_bulk_time_entry into the TimeEntry class where it fit better. Today I wanted to refine it more, since the Controller is still doing the actual save on the Model.
Before
# bulk_time_entries_controller.rb
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = TimeEntry.create_bulk_time_entry(entry)
unless @time_entry && @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# time_entry_patch.rb
def create_bulk_time_entry(entry)
return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
After
# bulk_time_entries_controller.rb
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = TimeEntry.create_bulk_time_entry(entry)
if @time_entry.new_record?
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# time_entry_patch.rb
def create_bulk_time_entry(entry)
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
if BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
end
time_entry.user = User.current
time_entry.save
time_entry
end
Review
The big behavior change is that instead of returning false or an unsaved TimeEntry, the method will now return a TimeEntry (saved if it's valid). This is a better design, since the caller (Controller) will not have to worry about the different types of objects that are returned. It will still need to check that the TimeEntry was saved, but the code is a lot easier to understand (if @time_entry.new_record?).
This refactoring really cleaned up the major flaws in the model method. I think it's now time to fix the controller/view interaction now, especially the inline RJS rendering.
Reference commit.
Tagged: ruby redmine plugins refactoring
written by Eric Davis on January 26, 2010
0 Comments
The Refactoring
Following up to yesterday's refactoring, today I kept refactoring the Bulk Time Entry plugin's controller. The save_time_entry_from_params method I extracted yesterday only uses the TimeEntry class, so it would be perfect to move the method to the TimeEntry class. Since the TimeEntry class is defined by Redmine, I had to monkey-patch that class. But the main content of this refactor is below:
Before
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = save_time_entry_from_params(entry)
unless @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# ...
def save_time_entry_from_params(entry)
next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
helper_method :save_time_entry_from_params
After
# bulk_time_entries_controller.rb
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = TimeEntry.create_bulk_time_entry(entry)
unless @time_entry && @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# time_entry_patch.rb
module BulkTimeEntryPlugin
module Patches
module TimeEntryPatch
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def create_bulk_time_entry(entry)
return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
end
end
end
end
Few things to note in this refactoring:
- I renamed the method to
create_bulk_time_entry to better describe it's behavior.
- I replaced the call to
save_time_entry_from_params to TimeEntry.create_bulk_time_entry.
- I changed the method body to return false if the user isn't allowed to log time to the project.
- I changed the controller to make sure the
create_bulk_time_entry didn't return false (user not allowed to access the project).
Review
This refactoring really cleaned up the model/controller interaction. There are still a few refactoring that can be done for the controller/view interaction now. But the TimeEntry patch could still use some work first, it's very procedural and has it's own code smells already.
In the reference commit I also included some unit tests for the refactoring and even happened to fix two small bugs.
Tagged: ruby redmine plugins refactoring move-method
written by Eric Davis on January 25, 2010
0 Comments
I'm having two problems right now:
- Not enough time to write to my blogs
- Not enough time to keep my Redmine plugins maintained
To try and fix both of these problems, I'm going to try and start something new here: a daily refactoring. I'm going do a small refactoring to my Redmine plugins or Redmine each day and post my thoughts about it here.
The Refactoring
I was working on my Bulk Time Entry plugin earlier today and found some very bad code in a controller.
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
@time_entry = TimeEntry.new(entry)
@time_entry.hours = nil if @time_entry.hours.blank? or @time_entry.hours <= 0
@time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
@time_entry.user = User.current
unless @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
Without getting into all of the smells (e.g. rendering and building model objects in the controller), the one that really stood out to me was that it was creating and saving a new TimeEntries in a loop. Ideally, this would be done in a model method but first I needed to extract out all of the parts used. So I used extract method with gave me this:
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = save_time_entry_from_params(entry)
unless @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# ...
def save_time_entry_from_params(entry)
next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
helper_method :save_time_entry_from_params
Review
This code still is a bit smelly, but it's getting better. Now I can decide if I want to push save_time_entry_from_params to the model or clean up save's RJS rendering.
Reference commit
Tagged: ruby redmine plugins refactoring extract-method
