The Refactoring
I’m still going off of the flay report for today, this time I’m tackling some duplication in the IssuesController
.
Before
1
2
3
4
5
6
7
8
9
10
11
12
13
|
# app/controller/issues_controller.rb
class IssuesController "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update({'id' => "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
end |
# app/controller/issues_controller.rb
class IssuesController "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update({'id' => "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
end
1
2
3
4
|
# app/models/query.rb
class Query < ActiveRecord::Base
# ..
end |
# app/models/query.rb
class Query < ActiveRecord::Base
# ..
end
After
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
|
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def index
retrieve_query
sort_init(@query.sort_criteria.empty? ? [['id', 'desc']] : @query.sort_criteria)
sort_update(@query.sortable_columns)
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update(@query.sortable_columns)
# ...
end
end |
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def index
retrieve_query
sort_init(@query.sort_criteria.empty? ? [['id', 'desc']] : @query.sort_criteria)
sort_update(@query.sortable_columns)
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update(@query.sortable_columns)
# ...
end
end
1
2
3
4
5
6
7
|
# app/models/query.rb
class Query "#{Issue.table_name}.id"}.merge(available_columns.inject({}) {|h, column|
h[column.name.to_s] = column.sortable
h
})
end
end |
# app/models/query.rb
class Query "#{Issue.table_name}.id"}.merge(available_columns.inject({}) {|h, column|
h[column.name.to_s] = column.sortable
h
})
end
end
Review
Since the controller code depended on the state of the @query
instance, it made sense to move it into the Query
model itself. I also split up the single line into multiple lines so it’s easier to understand what’s going on. I would like to factor out that train wreck but there’s not a lot of benefit to that since it’s not used that much.
I think I’ll be doing my next set of refactorings to some of my Redmine plugins. I have a few code smells in them I want to clean up for the next few releases.
Reference commit