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:

  1. It reduces the complexity of the Controller
  2. It removes some duplication in the Controller
  3. It makes a new method available on the Model that can be reused in other places

Reference commit

Tagged: ruby redmine refactoring move-method

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:

  1. I don't have the time
  2. I don't know that code base
  3. I don't have the right environment setup
  4. I don't know what to work on
  5. 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

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

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

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

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

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

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

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:

  1. a simple Kanban.new call wouldn't have any data, and
  2. 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