written by Eric Davis on April 19, 2010
0 Comments
The Refactoring
Using move method I was able to move a chunk of duplicated code from the QueriesController into the Query model.
Before
# app/controllers/queries_controller.rb
def new
# ...
params[:fields].each do |field|
@query.add_filter(field, params[:operators][field], params[:values][field])
end if params[:fields]
@query.group_by ||= params[:group_by]
# ...
end
def edit
# ...
params[:fields].each do |field|
@query.add_filter(field, params[:operators][field], params[:values][field])
end if params[:fields]
@query.attributes = params[:query]
# ...
end
# app/models/query.rb
class Query < ActiveRecord::Base
# ...
end
After
# app/controllers/queries_controller.rb
class QueriesController < ApplicationController
def new
# ...
@query.add_filters(params[:fields], params[:operators], params[:values]) if params[:fields]
@query.group_by ||= params[:group_by]
# ...
end
def edit
# ...
@query.add_filters(params[:fields], params[:operators], params[:values]) if params[:fields]
@query.attributes = params[:query]
# ...
end
end
class Query < ActiveRecord::Base
# Add multiple filters using +add_filter+
def add_filters(fields, operators, values)
fields.each do |field|
add_filter(field, operators[field], values[field])
end
end
end
Review
Even though this refactoring is simple, it's useful for three reasons:
- It reduces the complexity of the Controller
- It removes some duplication in the Controller
- It makes a new method available on the Model that can be reused in other places
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on April 16, 2010
0 Comments
I want to share the two steps that I'm using to walk the path to becoming a great developer. Becoming a great developer is a constant work in progress, but it's a pattern that I've seen many other great developers follow.
Step one: Write more code
This might sound easy but trust me, it's not easy. There are an infinite number of reasons we developers don't write code:
- I don't have the time
- I don't know that code base
- I don't have the right environment setup
- I don't know what to work on
- I'm tired
They all boil down to fear. You're afraid of something. Afraid of wasting time, afraid of being embarrassed publicly, afraid of making a mistake, afraid of being afraid.
Let me share two stories with you about my fears:
I've been a contributor to Redmine for a couple of years now, but I haven't been very active in the code base. Why? Redmine is a large complex code base and I didn't know how everything worked. So I stayed in my corner and only committed minor changes. Yet I still found a way to break those sections. Self fulfilling prophecy?
With my product, SeeProjectRun, I have to charge users' credit cards. Taking actual money is scary. After hearing all of the horror stories about companies screwing this up, I became deathly afraid of this and put off writing any billing code. Yes, me a developer who has written four credit card interfaces for active_merchant was afraid of writing code to bill his users. WTF is going on here?
Fear is a mistress that will steal your life if you let her. So how do you get over your fear of writing more code?
Write more code
As odd as it sounds, the only way I found to get through my fear of writing code was to crank it out like it was going out of style. The easiest way to do this? Start new side projects and contribute simple patches to Open Source. Every time you write code, you will learn something about the code, your tools, or yourself. Did you really think my 57 plus daily refactoring posts were only about fixing bad code? Nope, they are my sledgehammer against coder's block.
Oh and the ending to my stories about fear:
I just spent last night rewriting a core component of Redmine and committed it to the project this morning. It if breaks, I'll fix it. If it's really crap code, I'll revert it. No one will care and no one will remember the mistake.
And for the billing code I strapped myself down and finished the credit card billing code for SeeProjectRun in two days. Throwing two hundred test cases at it proved to me that it will work good enough to get over my fear.
Don't let fear hold you back from writing code.
Step two: Work with great developers
Now that you're creating code, you need to work with great developers so you can see how to they write great code.
Just take:
- 1 passionate developer (you)
- 1 great developer (them)
- a dash of code
Mix well daily and after a short rise in the over, you'll have two great developers. Feel free to add a few nuts (other great developers) and bake again.
You don't need to search for the greatest developers of all time, you just need developers smarter and further along in their skills than yourself. This can be easy if you work in a company that has hired great developers. But what do you do if your company doesn't hire any great developers or you are a solo freelancer like me?
Start reading great developers' code.
I'm making it a habit to start reading great developer's code. They put out so much code, you will find yourself reading so much of it that you start to dream about code. (Notice that the smart developers are always producing new code.... they are following step #1)
Getting started
Now here's the call to action, because you will never become a great developer without taking action.
1. Write at least one line of code in a new code base every day for a week. Switch code bases after each week.
This can be a new feature, a bugfix, a refactoring, or just monkeying around with an idea. It doesn't matter, the act of thinking through the code and writing is what you are after. Don't know one a good code base to start on? Do a refactoring on Redmine and tell me about it in the comments below.
2. Find a way to learn from a great developer every week.
If you are working with a great developer:
- do an informal code review their last commit
- ask to pair program with them, or
- buy them lunch and ask them about their favorite hack
If you are working solo:
- download some popular projects and read through a single class every week
- get some API documentation that shows the method's source code inline and read the source each time you look up a method, or
- find a mentor and work with them on some real code
Shameless self promotion
I want to make one offer. If you don't want to read it, skip the next paragraph. I'll wait a second for you to decide...
Great, you didn't skip. I've learned a lot from creating my Redmine plugins. I've tried to share this knowledge but no writing can substitute for one on one interaction. So this morning I launched a service where I'm offering personal 1-on-1 help to subscribers who want to take their Redmine development skills to the next level. It's a completely custom session for $179 per month, so we can jump right in and start working at your skill level. I'm only keeping it open for five subscribers, after which I'll be closing it down for awhile. If you're interested, all of the details are on my website.
So whatever you do, take action today. Unless you're afraid of becoming a great developer...
But there is plenty of room at the top.
Eric Davis
Tagged: development learning fear growth business
written by Eric Davis on April 16, 2010
0 Comments
I'm going to take a break from my Kanban plugin today to show a refactoring I did to Redmine last night for a client.
Redmine has a model called CustomField. This is shared by several other models to let users add custom data to their issues, projects, users, etc. Each of these custom fields has a specific format:
- String
- Text
- Integer
- Float
- List
- Date
- Boolean
For example, a shipping company would use a List type for a CustomField that tracks if a package was shipped via UPS, Fedex, or USPS.
The Refactoring
I did this refactoring because CustomField embeds the formats as a frozen Hash of Hashes. Using extract class, I converted this data clump into a full fledged class with a proper API.
Before
# app/models/custom_field.rb
class CustomField < ActiveRecord::Base
FIELD_FORMATS = { "string" => { :name => :label_string, :order => 1 },
"text" => { :name => :label_text, :order => 2 },
"int" => { :name => :label_integer, :order => 3 },
"float" => { :name => :label_float, :order => 4 },
"list" => { :name => :label_list, :order => 5 },
"date" => { :name => :label_date, :order => 6 },
"bool" => { :name => :label_boolean, :order => 7 }
}.freeze
validates_inclusion_of :field_format, :in => FIELD_FORMATS.keys
end
After
# lib/redmine/custom_field_format.rb
module Redmine
class CustomFieldFormat
include Redmine::I18n
cattr_accessor :available
@@available = {}
attr_accessor :name, :order, :label
def initialize(name, options={})
self.name = name
self.label = options[:label]
self.order = options[:order]
end
class << self
def map(&block)
yield self
end
# Registers a custom field format
def register(custom_field_format, options={})
@@available[custom_field_format.name] = custom_field_format unless @@available.keys.include?(custom_field_format.name)
end
def available_formats
@@available.keys
end
end
end
end
# lib/redmine.rb
Redmine::CustomFieldFormat.map do |fields|
fields.register Redmine::CustomFieldFormat.new('string', :label => :label_string, :order => 1)
fields.register Redmine::CustomFieldFormat.new('text', :label => :label_text, :order => 2)
fields.register Redmine::CustomFieldFormat.new('int', :label => :label_integer, :order => 3)
fields.register Redmine::CustomFieldFormat.new('float', :label => :label_float, :order => 4)
fields.register Redmine::CustomFieldFormat.new('list', :label => :label_list, :order => 5)
fields.register Redmine::CustomFieldFormat.new('date', :label => :label_date, :order => 6)
fields.register Redmine::CustomFieldFormat.new('bool', :label => :label_boolean, :order => 7)
end
# app/models/custom_field.rb
class CustomField < ActiveRecord::Base
validates_inclusion_of :field_format, :in => Redmine::CustomFieldFormat.available_formats
end
Review
Was this refactoring worth it, since the Hash of Hashes was working? I think so, since there were several limitations that many Redmine users were starting to hit with the old Hash data structure:
- Any code that checked custom field formats has to access the
FIELD_FORMATS constant directly (implementation leak)
- Showing custom field formats required reimplementing the view helpers if you wanted them shown differently (no reuse)
- It wasn't possible to add new
FIELD_FORMATS (closed data model)
I'm hoping that with this refactoring and some more changes to the CustomFieldFormat class, Redmine will be able to let developers start to define more complex custom data fields. I'm already working on one that adds a URL format to Redmine.
Reference commit
Tagged: ruby redmine refactoring extract-class
written by Eric Davis on April 15, 2010
0 Comments
The Refactoring
Today's refactoring is a simple one that makes use of Ruby's #inject. #inject is a great method for building accumulators but I don't see it that often in Rails code that much (including my own).
Before
# app/models/kanban_pane/active_pane.rb
class KanbanPane::ActivePane < KanbanPane
def get_issues(options={})
users = options.delete(:users)
issues = {}
users.each do |user|
issues[user] = KanbanIssue.find_active(user.id)
end unless users.blank?
issues
end
end
After
# app/models/kanban_pane/active_pane.rb
class KanbanPane::ActivePane < KanbanPane
def get_issues(options={})
users = options.delete(:users)
users.inject({}) do |result, user|
result[user] = KanbanIssue.find_active(user.id)
result
end
end
end
Review
Jay Fields has posted several other examples of using #inject. I still go back to that blog post every few months and learn something new.
Reference commit
Tagged: ruby redmine refactoring
written by Eric Davis on April 14, 2010
0 Comments
The Refactoring
I picked a big refactoring today. What started out as a small isolated change, grew and grew as I found dependencies all over the plugin. I used move method primarily but there are several other refactorings along for the ride.
Before you get hit with the wall of code, look for the code that's calling pane_configured? and compare it before and after.
Before
# app/helpers/kanbans_helper.rb
module KanbansHelper
def pane_configured?(pane)
(@settings['panes'] && @settings['panes'][pane] && !@settings['panes'][pane]['status'].blank?)
end
def display_pane?(pane)
if pane == 'quick-tasks'
pane_configured?('backlog') &&
@settings['panes']['quick-tasks']['limit'].present? &&
@settings['panes']['quick-tasks']['limit'].to_i > 0
else
pane_configured?(pane)
end
end
def column_configured?(column)
case column
when :unstaffed
pane_configured?('incoming') || pane_configured?('backlog')
when :selected
display_pane?('quick-tasks') || pane_configured?('selected')
when :staffed
true # always
end
end
# Calculates the width of the column. Max of 96 since they need
# some extra for the borders.
def staffed_column_width(column)
# weights of the columns
column_ratios = {
:user => 1,
:active => 2,
:testing => 2,
:finished => 2,
:canceled => 2
}
return 0.0 if column == :active && !pane_configured?(:active)
return 0.0 if column == :testing && !pane_configured?(:testing)
return 0.0 if column == :finished && !pane_configured?(:finished)
return 0.0 if column == :canceled && !pane_configured?(:canceled)
visible = 0
visible += column_ratios[:user]
visible += column_ratios[:active] if pane_configured?(:active)
visible += column_ratios[:testing] if pane_configured?(:testing)
visible += column_ratios[:finished] if pane_configured?(:finished)
visible += column_ratios[:canceled] if pane_configured?(:canceled)
return ((column_ratios[column].to_f / visible) * 96).round(2)
end
end
# app/models/kanban_pane.rb
class KanbanPane
# ...
end
# app/views/kanbans/show.html.erb
<% if pane_configured?('incoming') || pane_configured?('backlog') %>
<div id="unstaffed-requests" class="column" style="width: <%= column_width(:unstaffed) %>%">
<% if pane_configured?('incoming') %>
# ...
<% end %>
<% if pane_configured?('backlog') %>
# ...
<% end %>
</div>
<% end %>
<% if display_pane?('quick-tasks') || pane_configured?('selected') %>
<div id="selected-requests" class="column" style="width: <%= column_width(:selected) %>%">
<% if display_pane?('quick-tasks') %>
# ...
<% end %>
<% if pane_configured?('selected') %>
# ...
<% end %>
</div>
<% end %>
After
# app/helpers/kanbans_helper.rb
module KanbansHelper
# pane_configured? removed
# display_pane? removed
def column_configured?(column)
case column
when :unstaffed
KanbanPane::IncomingPane.configured? || KanbanPane::BacklogPane.configured?
when :selected
KanbanPane::QuickPane.configured? || KanbanPane::SelectedPane.configured?
when :staffed
true # always
end
end
# Calculates the width of the column. Max of 96 since they need
# some extra for the borders.
def staffed_column_width(column)
# weights of the columns
column_ratios = {
:user => 1,
:active => 2,
:testing => 2,
:finished => 2,
:canceled => 2
}
return 0.0 if column == :active && !KanbanPane::ActivePane.configured?
return 0.0 if column == :testing && !KanbanPane::TestingPane.configured?
return 0.0 if column == :finished && !KanbanPane::FinishedPane.configured?
return 0.0 if column == :canceled && !KanbanPane::CanceledPane.configured?
visible = 0
visible += column_ratios[:user]
visible += column_ratios[:active] if KanbanPane::ActivePane.configured?
visible += column_ratios[:testing] if KanbanPane::TestingPane.configured?
visible += column_ratios[:finished] if KanbanPane::FinishedPane.configured?
visible += column_ratios[:canceled] if KanbanPane::CanceledPane.configured?
return ((column_ratios[column].to_f / visible) * 96).round(2)
end
end
# app/models/kanban_pane.rb
class KanbanPane
def self.pane_name
self.to_s.demodulize.gsub(/pane/i, '').downcase
end
def self.configured?
pane = self.pane_name
(settings['panes'] && settings['panes'][pane] && !settings['panes'][pane]['status'].blank?)
end
end
# app/views/kanbans/show.html.erb
<% if KanbanPane::IncomingPane.configured? || KanbanPane::BacklogPane.configured? %>
<div id="unstaffed-requests" class="column" style="width: <%= column_width(:unstaffed) %>%">
<% if KanbanPane::IncomingPane.configured? %>
<div id="incoming" class="pane">
<%= render :partial => 'incoming' %>
</div>
<% end %>
<% if KanbanPane::BacklogPane.configured? %>
# ...
<% end %>
</div>
<% end %>
<% if KanbanPane::QuickPane.configured? || KanbanPane::SelectedPane.configured? %>
<div id="selected-requests" class="column" style="width: <%= column_width(:selected) %>%">
<% if KanbanPane::QuickPane.configured? %>
# ...
<% end %>
<% if KanbanPane::SelectedPane.configured? %>
# ...
<% end %>
</div>
<% end %>
Review
There is a lot of code here but it all comes down to moving the KanbansHelper#pane_configured? into the KanbanPane class. The nice thing about it is that the KanbanPane#configured? doesn't use a parameter anymore and just uses the class to check the configuration. This will make it easier to add more panes over time.
Reference commit (has a lot more code and test changes than shown here)
Tagged: ruby redmine refactoring move-method
written by Eric Davis on April 13, 2010
0 Comments
The Refactoring
Today I did another simple refactoring by moving a method from Kanban to KanbanPane.
Before
# app/models/kanban.rb
class Kanban
# Sort and group a set of issues based on IssuePriority#position
def group_by_priority_position(issues)
return issues.group_by {|issue|
issue.priority
}.sort {|a,b|
a[0].position <=> b[0].position
}
end
end
# app/models/kanban_pane.rb
class KanbanPane
# ...
end
# app/models/kanban_pane/backlog_pane.rb
class KanbanPane::BacklogPane < KanbanPane
def get_issues(options={})
# ...
# TODO: Remove wrapper
kanban = Kanban.new
return kanban.send(:group_by_priority_position, issues)
end
end
# app/models/kanban_pane/quick_pane.rb
class KanbanPane::QuickPane < KanbanPane
def get_issues(options={})
# ...
# TODO: Remove wrapper
kanban = Kanban.new
return kanban.send(:group_by_priority_position, issues)
end
end
After
# app/models/kanban.rb
class Kanban
# ...
end
# app/models/kanban_pane.rb
class KanbanPane
# Sort and group a set of issues based on IssuePriority#position
def group_by_priority_position(issues)
return issues.group_by {|issue|
issue.priority
}.sort {|a,b|
a[0].position <=> b[0].position
}
end
end
# app/models/kanban_pane/backlog_pane.rb
class KanbanPane::BacklogPane < KanbanPane
def get_issues(options={})
# ...
return group_by_priority_position(issues)
end
end
# app/models/kanban_pane/quick_pane.rb
class KanbanPane::QuickPane < KanbanPane
def get_issues(options={})
# ...
return group_by_priority_position(issues)
end
end
Review
Similar to yesterday's refactoring, this one moves the #group_by_priority_position method onto KanbanPane where it's used.
Reference commit
Tagged: ruby redmine refactoring move-method
written by Eric Davis on April 12, 2010
0 Comments
The Refactoring
Since creating the new KanbanPane class, there has been some wrapper methods to access Kanban's private utility methods. This refactoring uses move method to move that utility method from Kanban to KanbanPane.
Before
# app/models/kanban.rb
class Kanban
def missing_settings(pane, options={})
skip_status = options.delete(:skip_status)
@settings.blank? ||
@settings['panes'].blank? ||
@settings['panes'][pane].blank? ||
@settings['panes'][pane]['limit'].blank? ||
(@settings['panes'][pane]['status'].blank? && !skip_status)
end
end
# app/models/kanban_pane.rb
class KanbanPane
# TODO: Wrapper until moved from Kanban
def missing_settings(pane, options={})
kanban = Kanban.new
kanban.settings = Setting.plugin_redmine_kanban
kanban.send(:missing_settings, pane, options)
end
end
After
# app/models/kanban.rb
class Kanban
# ...
end
# app/models/kanban_pane.rb
class KanbanPane
def missing_settings(pane, options={})
skip_status = options.delete(:skip_status)
settings.blank? ||
settings['panes'].blank? ||
settings['panes'][pane].blank? ||
settings['panes'][pane]['limit'].blank? ||
(settings['panes'][pane]['status'].blank? && !skip_status)
end
end
Review
Move method turned out to get a good refactoring for this code. Not only did it put #missing_settings into the class where it belongs, but it also helped to get rid of the ugly #send hack that KanbanPane was using. There is another similar method I need to take care of tomorrow.
Reference commit
Tagged: ruby redmine refactoring move-method
A new release of the Redmine Stuff To Do plugin has been tested and released. This release includes over 13 changes since the last release including the new Time Grid feature. This will be the final version that is supported on Redmine 0.8, the next versions will require Redmine 0.9.
Download
The plugin can be download from the Little Stream Software project or from GitHub.
Changes
Help
If you need help, my Redmine bug tracker is open to the public and you are welcome to ask for help there.
Eric
Tagged: open source redmine redmine plugins
written by Eric Davis on April 09, 2010
0 Comments
The Refactoring
Now that everything has been refactored out of Kanban#find, there really is no use for it. All that it does is to wrap the constructor so it's now useless code.
Before
# app/models/kanban.rb
class Kanban
def self.find
Kanban.new
end
end
After
# app/models/kanban.rb
class Kanban
# ...
end
Review
After every refactoring session you will have some useless code left over. Sometimes it's to keep a public API stable, sometimes it's a simple delegate, and sometimes (like now) it's just garbage that should be thrown away. Whenever possible, throw it away. If you need it back, git will be there.
Reference commit
Tagged: ruby redmine refactoring remove-middle-man
written by Eric Davis on April 08, 2010
0 Comments
This change isn't a strict refactoring since it's adding a new feature, but since it was done with the intent of cleaning up the code I'm going to let it pass.... this time.
The Refactoring
The big problem with Kanban is that all callers have to use Kanban#find in order to setup the data. This meant:
- a simple
Kanban.new call wouldn't have any data, and
- there was no way to load just one or two panes of data (it's all or nothing)
This coupled all callers to Kanban#find and the performance of that method.
Before
# app/models/kanban.rb
class Kanban
def initialize
@incoming_pane = KanbanPane::IncomingPane.new
@backlog_pane = KanbanPane::BacklogPane.new
@quick_pane = KanbanPane::QuickPane.new
@canceled_pane = KanbanPane::CanceledPane.new
@finished_pane = KanbanPane::FinishedPane.new
@active_pane = KanbanPane::ActivePane.new
@testing_pane = KanbanPane::TestingPane.new
end
def self.find
kanban = Kanban.new
kanban.settings = Setting.plugin_redmine_kanban
kanban.users = kanban.get_users
kanban.incoming_issues = kanban.incoming_pane.get_issues
kanban.quick_issues = kanban.quick_pane.get_issues
kanban.backlog_issues = kanban.backlog_pane.get_issues(:exclude_ids => kanban.quick_issue_ids)
kanban.selected_issues = KanbanIssue.find_selected
kanban.active_issues = kanban.active_pane.get_issues(:users => kanban.users)
kanban.testing_issues = kanban.testing_pane.get_issues(:users => kanban.users)
kanban.finished_issues = kanban.finished_pane.get_issues
kanban.canceled_issues = kanban.canceled_pane.get_issues
kanban
end
end
After
# app/models/kanban.rb
class Kanban
def initialize
@incoming_pane = KanbanPane::IncomingPane.new
@backlog_pane = KanbanPane::BacklogPane.new
@quick_pane = KanbanPane::QuickPane.new
@canceled_pane = KanbanPane::CanceledPane.new
@finished_pane = KanbanPane::FinishedPane.new
@active_pane = KanbanPane::ActivePane.new
@testing_pane = KanbanPane::TestingPane.new
@settings = Setting.plugin_redmine_kanban
@users = get_users
end
def self.find
Kanban.new
end
def incoming_issues
@incoming_issues ||= incoming_pane.get_issues
end
def quick_issues
@quick_issues ||= quick_pane.get_issues
end
def backlog_issues
quick_issues # Needs to load quick_issues
@backlog_issues ||= backlog_pane.get_issues(:exclude_ids => quick_issue_ids)
end
def selected_issues
@selected_issues ||= KanbanIssue.find_selected
end
def active_issues
@active_issues ||= active_pane.get_issues(:users => get_users)
end
def testing_issues
@testing_issues ||= testing_pane.get_issues(:users => get_users)
end
def finished_issues
@finished_issues ||= finished_pane.get_issues
end
def canceled_issues
@canceled_issues ||= canceled_pane.get_issues
end
end
Review
Basically what I did was to move all of the data access to the accessors, so no data would be loaded until it was tried to be accessed (called Lazy Load). This separated the Kanban initialization from the data loading and makes it's easier to only load up one or two panes of data.
I thought this would be a huge performance upgrade since the Kanban board only updates one or two panes of data at a time. Sadly, it looks like there are other performance issues so I saw less than a 1% performance improvement. But I decided to keep this change since it makes the code easier to read. Performance improvements can be done later, once the code is well factored.
Reference commit
Tagged: ruby redmine refactoring