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>http://refactoring.com/</ref>
Following O-O Design Principles
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:
- Changes made:
- Moved code for setting optional parameters to function set_template_optional_params
- Moved code for validation of optional parameters to function called validate_optional_conf
- The new code now does not violate the single responsibility principle.
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 support separation of concerns and code reuse which are both good practices when it comes to object-oriented design.
The other files we touched are:
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. (in graph_data_type_list function)
Moving the common data type options to a separate list for reuse by bar, line and pie chart types. (in init function, added generic_supported_types array)
Eliminating switch case statements in get_graph_data_bundle and redirecting the call to a more generic function to get the chart data. (in get_graph_data_bundle function)
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:
Removing bar_chart_data(which was specific and had reusable code) and creating a more generic function, get_chart_data which can be used for bar, line and pie charts.
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.
Displaying user friendly messages while the chart is being generated.
Note: Pie chart currently only compares based on the first selected data type.
WIKI STUFF
Extended Changes:
We have also gone beyond the scope of the project by analyzing and fixing the code of the analytic controller and helper respectively. When we try to test the chart function file we had modified, we realized that the analytic view page that was using the chart helper class had certain issues/areas of improvement. The issues/areas of improvement we identified were:
Only bar option is enabled in the UI.
The chart takes a long time to render as the data retrieval queries take a long time. There was no proper notification to the user as to what is happening.
Selecting the bar option was still not getting any graph being displayed.
Issue 1: Currently, the code has support only for bar graphs.
Figure 1: The existing page
We felt that the line and pie charts could be shown as well along with the bar graphs as most of the underlying framework was already present. Hence, we made line and bar selected as well.
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.
To render the different types of graph, apart from the given requirement of the project, here is a list of the changes in more detail. 1) 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. (in graph_data_type_list function) data_type_list = @available_data_types[params[:scope].to_sym] & @available_data_types[params[:type].to_sym]
The above line is used to get the data type based on the comparison scope and the graph type. 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 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: 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, added generic_supported_types array) There is a 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. (in get_graph_data_bundle function) The current implementation had a set of switch case statements depending on the graph type. 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 irresepective of the graph type. This has helped us to eliminate Switch Statement Smell(http://c2.com/cgi/wiki?SwitchStatementsSmell)
2) 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:
Removing bar_chart_data(which was specific and had reusable code) and creating a more generic function, 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 chart data. However the data is independent of the chart type, hence we felt this should be a more generic data fetcher.
The signature of the function was changed from def bar_chart_data(object_type, object_id_list, data_type_list) to def get_chart_data(chart_type, object_type, object_id_list, data_type_list) Note that even though chart_type is being accepted here, it is just used to send it to the initialize method for the chart object. This was being harcoded earlier. This change in principle 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. Development Note: Pie chart currently only compares based on the first selected data type. Either, it should render multiple pie charts or radio buttons for selecting data type.
Displaying user friendly messages while the chart is being generated. Currently the page shows an empty chart area when the chart is being rendered. This process sometimes takes a long time to be rendered. 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 (http://en.wikipedia.org/wiki/Principles_of_user_interface_design)
Figure 2: Message to show chart is being processed
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.
Figure 3: Friendly error message to guide users
The END Result
The analytics view now supports line and pie chart options along with the Baroption(which was not working earlier)
Figure 4: Bar Chart
Figure 5: Line Graph
Figure 6: Pie chart <references/>