The Refactoring
The flay report on Caliper shows that IssuesHelper#show_detail has the most duplicated code in Redmine, specifically inside of a long case statement. The first step I took was to extract a new method from the body of the case statement.
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 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 | # app/helpers/issues_helper.rb module IssuesHelper def show_detail(detail, no_html=false) case detail.property when 'attr' label = l(("field_" + detail.prop_key.to_s.gsub(/\_id$/, "")).to_sym) case detail.prop_key when 'due_date', 'start_date' value = format_date(detail.value.to_date) if detail.value old_value = format_date(detail.old_value.to_date) if detail.old_value when 'project_id' p = Project.find_by_id(detail.value) and value = p.name if detail.value p = Project.find_by_id(detail.old_value) and old_value = p.name if detail.old_value when 'status_id' s = IssueStatus.find_by_id(detail.value) and value = s.name if detail.value s = IssueStatus.find_by_id(detail.old_value) and old_value = s.name if detail.old_value when 'tracker_id' t = Tracker.find_by_id(detail.value) and value = t.name if detail.value t = Tracker.find_by_id(detail.old_value) and old_value = t.name if detail.old_value when 'assigned_to_id' u = User.find_by_id(detail.value) and value = u.name if detail.value u = User.find_by_id(detail.old_value) and old_value = u.name if detail.old_value when 'priority_id' e = IssuePriority.find_by_id(detail.value) and value = e.name if detail.value e = IssuePriority.find_by_id(detail.old_value) and old_value = e.name if detail.old_value when 'category_id' c = IssueCategory.find_by_id(detail.value) and value = c.name if detail.value c = IssueCategory.find_by_id(detail.old_value) and old_value = c.name if detail.old_value when 'fixed_version_id' v = Version.find_by_id(detail.value) and value = v.name if detail.value v = Version.find_by_id(detail.old_value) and old_value = v.name if detail.old_value when 'estimated_hours' value = "%0.02f" % detail.value.to_f unless detail.value.blank? old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank? end when 'cf' custom_field = CustomField.find_by_id(detail.prop_key) if custom_field label = custom_field.name value = format_value(detail.value, custom_field.field_format) if detail.value old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value end when 'attachment' label = l(:label_attachment) end # ... more implementation 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 54 55 56 57 58 59 | module IssuesHelper def show_detail(detail, no_html=false) case detail.property when 'attr' field = detail.prop_key.to_s.gsub(/\_id$/, "") label = l(("field_" + field).to_sym) case detail.prop_key when 'due_date', 'start_date' value = format_date(detail.value.to_date) if detail.value old_value = format_date(detail.old_value.to_date) if detail.old_value when 'project_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'status_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'tracker_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'assigned_to_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'priority_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'category_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'fixed_version_id' value = find_name_by_reflection(field, detail.value) old_value = find_name_by_reflection(field, detail.old_value) when 'estimated_hours' value = "%0.02f" % detail.value.to_f unless detail.value.blank? old_value = "%0.02f" % detail.old_value.to_f unless detail.old_value.blank? end when 'cf' custom_field = CustomField.find_by_id(detail.prop_key) if custom_field label = custom_field.name value = format_value(detail.value, custom_field.field_format) if detail.value old_value = format_value(detail.old_value, custom_field.field_format) if detail.old_value end when 'attachment' label = l(:label_attachment) end # ... more implementation end # Find the name of an associated record stored in the field attribute def find_name_by_reflection(field, id) association = Issue.reflect_on_association(field.to_sym) if association record = association.class_name.constantize.find_by_id(id) return record.name if record end end end |
Review
This refactoring changed the case statement so that most of the body methods are identical. This will make tomorrow’s refactoring even easier, since the duplication is now really visible.
I think #find_name_by_reflection has an interesting implementation. It uses ActiveRecord’s #reflect_on_association which is a very easy way to dynamically walk the association tree for classes. Here I used it to dynamically get the class of a model in order to fetch it’s record. Another useful place for #reflect_on_association is in unit tests. I’ve worked on a project a long time ago that used this with RSpec to provide custom matchers for associations (e.g. Class.should belong_to(:other_class)).
Share
Related posts:
