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
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 |
# 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
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 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 |
# 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.