Daily Refactor #57: Extract Class to CustomFieldFormat

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

1
2
3
4
5
6
7
8
9
10
11
12

# app/models/custom_field.rb
class CustomField  { :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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

# 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
 
1
2
3
4
5
6
7
8
9
10

# 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
 
1
2
3

# app/models/custom_field.rb
class CustomField  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