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
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
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:
- 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).
- 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?
- 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
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
