CSC/ECE 517 Fall 2014/OSS E1455 skn
E1455: Refactoring the Chart helper
Overview
Code refactoring
Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior<ref>Refactoring</ref>. Refactoring adds to the value of any program that has at least one of the following shortcomings<ref>Benefits of Code Refactoring Michael Hunger. Oct. 25, 2000</ref>:
- Programs that are hard to read are hard to modify.
- Programs that have duplicate logic are hard to modify
- Programs that require additional behavior that requires you to change running code are hard to modify.
- Programs with complex conditional logic are hard to modify.
Following O-O Design Principles
Object oriented design a process of planning a software system where objects will interact with each other to solve specific problems. Though an Object oriented language provides us with highly useful and important programming concepts like Inheritance, Polymorphism, Abstraction and Encapsulation which definitely makes the code more efficient, it is equally important to have the knowledge of using them in the code.
Object Oriented Design Principles are core of OOPS programming<ref>[1] Javin Paul. Blogspot. March 3, 2012</ref>. It is important to know these design principles, to create clean and modular design. There are a number of design principles that help us produce a more understandable and elegant code.
Some of these can be looked up here.
Project Resources
Objective
The aim of the project was to refactor the chart helper module of Expertiza. The task was to analyze the helper chart.rb which failed to follow the object oriented design principles. There were two major functions being used in the class, effectively. These two functions were performing all their operations in one single block which led to code duplication and redundancy. Our task involved removing these design errors and making it more elegant and efficient.
Files Modified
Requirements
The following changes were expected,
- self.dataAdapter method
- Too long, needs to be divided
- Too many if-else statements
- self.data_template method
- Needs to be broken down or moved according to requirement
- Define each template independently instead of in the same method
- Remove unecessary data from :series for scatter plot
- Method has duplicate data. Remove the duplicate occurrences
Design Changes
To fulfill the requirements, the following changes were implemented,
Refactoring the dataAdapter method
- What it does
- The dataAdapter function is basically an initialization function which sets parameters for the graphical plot to some default starting values based on the type of analytic view the user wants to rendered. There was quite a bit of code duplication in the existing function, especially when it came to setting parameters for some specific chart types like pie charts for instance.
- The code in this function violated the 'Single Responsibility Principle'.
- Original code:
def self.dataAdapter(type,data,optionalConf)
template = data_template[type];
if (type == :pie) then
data[:type] = 'pie';
template[:series] = [data]
else
template[:series] = data
end
if optionalConf.nil? then
template[:title][:text] = ""
template.delete(:subtitle)
template.delete(:yAxis)
template.delete(:xAxis)
else
if template[:subtitle].nil? then
template[:subtitle]={}
end
if optionalConf[:title].nil? then
template[:title][:text] = ""
else
template[:title][:text] = optionalConf[:title]
end
if optionalConf[:subtitle].nil? then
template.delete(:subtitle)
else
template[:subtitle][:text]=optionalConf[:subtitle]
end
if optionalConf[:y_axis].nil? then
template.delete(:yAxis)
else
template[:yAxis][:title][:text]=optionalConf[:y_axis]
end
if optionalConf[:x_axis].nil? then
template[:xAxis].delete(:title)
else
template[:xAxis][:title][:text] = optionalConf[:x_axis]
end
if optionalConf[:x_axis_categories].nil? then
template[:xAxis].delete(:categories)
else
template[:xAxis][:categories]=optionalConf[:x_axis_categories]
end
end
template
end
- Changes made:
- Moved code for setting optional parameters to function set_template_optional_params4
template[:title][:text] = "" template.delete(:subtitle) template.delete(:yAxis) template.delete(:xAxis)
- Moved code for validation of optional parameters to function called validate_optional_conf
if optionalConf[:subtitle].nil? then
template.delete(:subtitle)
else
template[:subtitle][:text]=optionalConf[:subtitle]
end
if optionalConf[:y_axis].nil? then
template.delete(:yAxis)
else
template[:yAxis][:title][:text]=optionalConf[:y_axis]
end
if optionalConf[:x_axis].nil? then
template[:xAxis].delete(:title)
else
template[:xAxis][:title][:text] = optionalConf[:x_axis]
end
if optionalConf[:x_axis_categories].nil? then
template[:xAxis].delete(:categories)
else
template[:xAxis][:categories]=optionalConf[:x_axis_categories]
end
end
- The new code does not violate the Single Responsibility Principle anymore
def self.dataAdapter(type,data,optionalConf)
template = data_template[type];
if (type == :pie) then
template = set_pie_data(data,template)
else
template[:series] = data
end
if optionalConf.nil? then
template = set_template_optional_params(template)
else
if optionalConf[:title].nil? then
template[:title][:text] = ""
else
template[:title][:text] = optionalConf[:title]
end
template=validate_optional_conf(optionalConf,template)
end
template
end
def self.set_template_optional_params(template) template[:title][:text] = "" template.delete(:subtitle) template.delete(:yAxis) template.delete(:xAxis) template end
def self.validate_optional_conf(optionalConf,template)
if optionalConf[:subtitle].nil? then
template.delete(:subtitle)
else
template[:subtitle]={}
template[:subtitle][:text]=optionalConf[:subtitle]
end
if optionalConf[:y_axis].nil? then
template.delete(:yAxis)
else
template[:yAxis][:title][:text]=optionalConf[:y_axis]
end
if optionalConf[:x_axis].nil? then
template[:xAxis].delete(:title)
else
template[:xAxis][:title][:text] = optionalConf[:x_axis]
end
if optionalConf[:x_axis_categories].nil? then
template[:xAxis].delete(:categories)
else
template[:xAxis][:categories]=optionalConf[:x_axis_categories]
end
template
end
Refactoring the dataTemplate method
- What it does
- The main purpose of this method was to set up the data templates for the different varieties of graphs - bar , scatter , pie and line namely.
- This method, again, was too long and executing too many different tasks in the same block of code. Furthermore, there was a lot of code duplication which did not follow object-oriented design :at all. There was also hard-coded data which was not serving much purpose which we decided to prune to make the code more elegant.
- The original method was setting up templates for all these different types of graphs in the same method which 'violated the Single Responsibility Principle'.
- Original code:
- Changes made:
- Created separate methods for each different plot - bar , line , pie and scatter
- Created one method called get_generic_template to set up some common parameters which are used by more than one plot.
- The new code does not violate the Single Responsibility Principle and also supports Separation of Concerns and Code Reuse which are both good practices when it comes to object-oriented design.
Bonus Changes
Going beyond the scope of the project, the code for the analytic controller and helper was analyzed and fixed. While testing the chart function file that had been modified, it was noted that the analytic view page that was using the chart helper class had certain issues/areas of improvement. The issues/areas of improvement identified were as follows:
Issue 1
- 'Bar graph' was the only chart supported
- The modified code now provides support for the line and pie charts along with the bar graphs using the existing underlying framework.
Issue 2
- Probably because of the way ruby makes database queries by default, data fetching takes a long time
- Order of ~10s for simple options(assignment and team comparisons) and ~5 mins for larger comparisons(course comparisons), based on VCL deployed versions.
- The page just stayed idle with no message. This can be frustrating for the user who is unsure if the chart is going to be rendered or not
Issue 3
- The existing implementation does not render the graph when the bar option is selected.
- This is because no data types are being displayed for selection, as shown in Figure 1
Extra File Changes
To render the different types of graph, apart from the given requirement of the project, additional file changes were made. Following is a list of the changes in more detail,
app/controllers/analytic_controller.rb
The file used for rendering the analytic view. The changes made include:
- Fixing a bug which was preventing the data type options from being displayed in the analytic view page
- Inside the graph_data_type_list function, the following line of code was used to get a common list of data types for the graph type and the comparison scope.
Existing code:
data_type_list = @available_data_types[params[:scope].to_sym] & @available_data_types[params[:type].to_sym]
However, this intersection was always evaluating to an empty set because @available_data_types[params[:scope].to_sym] retrieved a list of symbols and @available_data_types[params[:type].to_sym] retrieved a list of strings. Hence, the data type list was always empty as can be shown in Figure 1.
This was corrected by converting the list of string to a list of symbols and then taking an intersection as shown below:
Modified code:
data_type_list = @available_data_types[params[:scope].to_sym] & (@available_data_types[params[:type].to_sym].map &:to_sym)
This change is a 'bug fix'.
- Moving the common data type options to a separate list for reuse by bar, line and pie chart types
- In init function, there is an instance variable called as @available_data_types which has an object with key ‘bar’ and value as the list of methods supported by bar graphs. We felt it could be reused by the graphs of type, line and bar. Hence, we moved it to a common variable, @generic_supported_types which could be reused by line, bar and pie graph types. This change follows the DRY principle.
- Development Note: The bar options have been reused for line and pie graphs. Based on requirements, these can be altered. We have just enabled the functionality for pie and bar so that once the requirements are ready, they can be easily plugged in.
- Eliminating switch case statements in get_graph_data_bundle and redirecting the call to a more generic function to get the chart data
- The current implementation of get_graph_data_bundle had a set of switch case statements depending on the graph type.
case params[:type]
when "line"
chart_data = line_graph_data(params[:scope], params[:id], params[:data_type])
when "bar"
chart_data = bar_chart_data(params[:scope], params[:id], params[:data_type])
when "scatter"
chart_data = scatter_plot_data(params[:scope], params[:id], params[:data_type])
when "pie"
chart_data = pie_chart_data(params[:scope], params[:id], params[:data_type])
end
However, since the underlying principle of fetching the graph types is the same, we felt the call should be re-routed to a generic function which can fetch the data irrespective of the graph type. This has helped us to eliminate Switch Statement Smell<ref>Switch Statements Smell CGI Wiki. September 11, 2014</ref>
chart_data = get_chart_data(params[:type],params[:scope], params[:id], params[:data_type])
app/helpers/analytic_helper.rb
This file is used to redirect the calls to the appropriate method based on the data type selected for comparison and fetch the data to be sent to the Chart helper function. Changes in this file include:
- Renaming bar_chart_data(which was specific and had reusable code) to a more generic function.
- The current get_chart_data which can be used for bar, line and pie charts. The current function’s name indicated that it could be used to retrieve only bar chart data. However the data is independent of the chart type, hence we felt this should be a more generic data fetcher.
Existing signature:
def bar_chart_data(object_type, object_id_list, data_type_list)
Modified signature:
def get_chart_data(chart_type, object_type, object_id_list, data_type_list).
Also the earlier function was passing :bar to the initialize method of the chart object. The value was being hardcoded. With the current implementation, the input parameter chart_type is passed to the chart object. This helps in the code being more flexible and the re usability of the code for any type of chart type promotes the DRY principle.
app/views/analytic/index.html.erb
The file which shows the view for the analytics. The changes made to this file include:
- Enabling options for line and pie charts.
- The current implementation had only the bar option enabled. The other options(line, pie, scatter) were disabled. Since the pie and line chart types were made functional, the corresponding options were enabled. Since there was no use case for scatter plots at the moment(given the scope), the scatter option still remains disabled.
- Development Note: Pie chart currently only compares based on the first selected data type. It should either render multiple pie charts or provide radio buttons for selecting one data type at a time.
- Displaying user friendly messages while the chart is being generated.
- Currently the page shows an empty chart area when the chart is being rendered. The graph sometimes takes a long time to be rendered as mentioned in the issues. An ideal task would have been to look at the queries being made and make them faster. Given the scope of the project and the time restriction, we thought of adding basic user-friendly message. This feature promotes Feedback principle in UI design<ref>Principles of User Interface design Last Modified 24 July 2014</ref>
Development Note: The retrieval queries should be inspected to see if the data retrieval can be made faster. Since the chart rendering is fired whenever there is a change in selection of the data point and the comparison scope, we have added a friendly error message which tells the user to choose the options.
Summary of Revised Design Principles
| Sr. No. | File name | Method Name | Comment on Design Principle |
|---|---|---|---|
| 1. | chart.rb | dataAdapter | Violated the Single Responsibility Principle |
| dataTemplate | Violated the Single Responsibility Principle | ||
| 2. | analytic_controller.rb | graph_data_type_list | |
| init | Follows the DRY Principle | ||
| get_graph_data_bundle | Eliminated the Switch Statement Smell Principle | ||
| 3. | analytic_helper.rb | bar_chart_data | Violated the Single Responsibility Principle |
| get_chart_data | Promotes DRY principle | ||
| 4. | views/analytic/index.html.erb | Promotes Feedback principle in UI design |
Final Result
The analytics view now supports line and pie chart options along with the Bar Option(which was not working earlier)
Sample Bar Chart
Sample Line Graph
Sample Pie Chart
References
<references/>