CSC/ECE 517 Fall 2014/OSS E1455 skn: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(76 intermediate revisions by 2 users not shown)
Line 2: Line 2:
=='''Overview'''==
=='''Overview'''==
===Code refactoring===
===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>
''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/ Refactoring]</ref>.
Refactoring adds to the value of any program that has at least one of the following shortcomings<ref>[http://jexp.de/papers/refactoring/refactoring/node9.html#SECTION00330000000000000000 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.<br>


===Following O-O Design Principles===
===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>[http://javarevisited.blogspot.com/2012/03/10-object-oriented-design-principles.html#ixzz3Ha8L3cfz] 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 [http://javarevisited.blogspot.com/2012/03/10-object-oriented-design-principles.html here].<br>




Line 11: Line 20:
= Project Resources =
= Project Resources =
* [https://github.com/noelmathew/expertiza GitHub Repository]
* [https://github.com/noelmathew/expertiza GitHub Repository]
* [http://152.46.16.178:3001/ VCL IP]
* [http://152.46.16.178:3001/ VCL IP] (Log in as user2/password. Go to [http://152.46.16.178:3001/analytic/index VCL analytic page])
* [https://github.com/expertiza/expertiza/pull/439 Git Pull Link]
 
 
= Installation Instructions =
Follow the steps to view the changes,
 
'''Step 1:''' Use URL http://152.46.16.178:3001/ to connect to the
expertiza vcl reservation
 
'''Step 2:''' Log into expertiza with the following credentials,
user: user2
password: password
'''Step 3:''' Type in http://152.46.16.178:3001/analytic/index.html
in your browser
'''Step 4:''' Select the appropriate options for comparison
 
'''Note:'''
* There are some courses that have empty data and may not work.
::As a safe option, try these, :)
::* Graph Type: Bar chart
::* Comparison scope: team
::* Course: CSC 517, Spring 2014
::* Assignment: Backchannel, Spring 2014
::*Team: select all teams
::*Data:
:::-average review character count
:::-max review character count
:::-min review character count
* For the 'pie chart',
::The chart is generated based on the first option in the data that is selected
(Since it was beyond the scope of our project requirements,
we just got the chart working)




Line 40: Line 84:


= Design Changes =
= Design Changes =
 
To fulfill the requirements, the following changes were implemented,
== Refactoring the ''dataAdapter method'' ==
== Refactoring the ''dataAdapter method'' ==


'''What it does'''<br>
'''What it does'''<br>
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 ''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''''.
: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'' ==
 
This method was again too long and doing 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 main purpose of this method was to set up the data templates for the different varieties of graphs - bar , scatter , pie and line namely. The original method was setting up templates for all these different types of graphs in the same method which violated the single responsibility principle.
 
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.
 
 
 


'''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:'''


*The new code ''does not'' violate the Single Responsibility Principle anymore. This is achieved by Separation of Concerns. Each function is divided into a smaller module and focuses on one small task.


  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
*Moved code for setting optional parameters to function '''''set_template_optional_params'''''
  def self.set_template_optional_params(template)
    template[:title][:text] = ""
    template.delete(:subtitle)
    template.delete(:yAxis)
    template.delete(:xAxis)
    template
  end
*Moved code for validation of optional parameters to function '''''validate_optional_conf'''''
  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
The code is a lot more cleaner, readable and maintainable now.


== 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'''' (data_template function is responsible for all types of charts). It also duplicated some code(code for line and bar) which resulted in a violation of a '''DRY Principle'''.


'''Original code:'''
<pre>
  def self.data_template()
    {
      :pie => {
        :chart => {
          :plotBackgroundColor => nil,
          :plotBorderWidth => nil,
          :plotShadow => false
        },
        :title => {
          :text => 'Title'
        },
        :subtitle => {
          :text => 'Title'
        },
        :xAxis => {},
        :yAxis => {},
        :tooltip => {
          :pointFormat => '{series.name}: <b>{point.percentage}%</b>',
          :percentageDecimals => 1
        },
        :plotOptions => {
          :pie => {
            :allowPointSelect => true,
            :cursor => 'pointer',
            :dataLabels => {
              :enabled => true,
              :color => '#000000',
              :connectorColor => '#000000',
              :format => "<b>{point.name}</b>: {percentage:.2f} %"
            }
          }
        },
        :series => [
          {
            :type => 'pie',
            :name => 'XXX pie',
            :data => [
              ['part 1',45.0],
              ['part 2',26.8],
              ['part 3',8.5],
              ['part 4',6.2],
              ['part 5',0.7]
            ]
          }
        ]
      },
      :bar => {
        :chart => {
          :type => 'column'
        },
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
          :title =>{},
          :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
          :min => 0,
          :title => {
            :text => 'score'
          }
        },
        :tooltip => {
          :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
          :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f}</b></td></tr>',
          :footerFormat => '</table>',
          :shared => true,
          :useHTML => true
        },
        :plotOptions => {
          :column => {
            :pointPadding => 0.2,
            :borderWidth => 0
          }
        },
        :series => [
          {
            :name => 'review 1',
            :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
          }, {
            :name => 'review 2',
            :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
          }, {
            :name => 'review 3',
            :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
          }
        ]
      },


      :line => {
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
          :title =>{},
          :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
          :min => 0,
          :title => {
            :text => 'score'
          }
        },
        :tooltip => {
          :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
          :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f} mm</b></td></tr>',
          :footerFormat => '</table>',
          :shared => true,
          :useHTML => true
        },
        :plotOptions => {
          :column => {
            :pointPadding => 0.2,
            :borderWidth => 0
          }
        },
        :series => [
          {
            :name => 'review 1',
            :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
          }, {
            :name => 'review 2',
            :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
          }, {
            :name => 'review 3',
            :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
          }
        ]
      },


      :scatter => {
        :chart => {
          :type => 'scatter',
          :zoomType => 'xy'
        },
        :title => {
          :text => 'Height Versus Weight of 507 Individuals by Gender'
        },
        :subtitle => {
          :text => 'Source: Heinz  2003'
        },
        :xAxis => {
          :title => {
            :enabled => true,
            :text => 'Height (cm)'
          },
          :startOnTick => true,
          :endOnTick => true,
          :showLastLabel => true
        },
        :yAxis => {
          :title => {
            :text => 'Weight (kg)'
          }
        },
        :legend => {
          :layout => 'vertical',
          :align => 'left',
          :verticalAlign => 'top',
          :x => 100,
          :y => 70,
          :floating => true,
          :backgroundColor => '#FFFFFF',
          :borderWidth => 1
        },
        :plotOptions => {
          :scatter => {
            :marker => {
              :radius => 5,
              :states => {
                :hover => {
                  :enabled => true,
                  :lineColor => 'rgb(100,100,100)'
                }
              }
            },
            :states => {
              :hover => {
                :marker => {
                  :enabled => false
                }
              }
            },
            :tooltip => {
              :headerFormat => '<b>{series.name}</b><br>',
              :pointFormat => '{point.x} cm, {point.y} kg'
            }
          }
        },
        :series => [
          {
            :name => 'Female',
            :color => 'rgba(223, 83, 83, .5)',
            :data => [[161.2, 51.6], [167.5, 59.0], [159.5, 49.2], [157.0, 63.0], [155.8, 53.6],
                      [170.0, 59.0], [159.1, 47.6], [166.0, 69.8], [176.2, 66.8], [160.2, 75.2],
                      [172.5, 55.2], [170.9, 54.2], [172.9, 62.5], [153.4, 42.0], [160.0, 50.0],
                      [147.2, 49.8], [168.2, 49.2], [175.0, 73.2], [157.0, 47.8], [167.6, 68.8],
                      [159.5, 50.6], [175.0, 82.5], [166.8, 57.2], [176.5, 87.8], [170.2, 72.8],
                      [174.0, 54.5], [173.0, 59.8], [179.9, 67.3], [170.5, 67.8], [160.0, 47.0],
                      [154.4, 46.2], [162.0, 55.0], [176.5, 83.0], [160.0, 54.4], [152.0, 45.8],
                      [162.1, 53.6], [170.0, 73.2], [160.2, 52.1], [161.3, 67.9], [166.4, 56.6],
                      [168.9, 62.3], [163.8, 58.5], [167.6, 54.5], [160.0, 50.2], [161.3, 60.3],
                      [167.6, 58.3], [165.1, 56.2], [160.0, 50.2], [170.0, 72.9], [157.5, 59.8],
                      [167.6, 61.0], [160.7, 69.1], [163.2, 55.9], [152.4, 46.5], [157.5, 54.3],
                      [168.3, 54.8], [180.3, 60.7], [165.5, 60.0], [165.0, 62.0], [164.5, 60.3],
                      [156.0, 52.7], [160.0, 74.3], [163.0, 62.0], [165.7, 73.1], [161.0, 80.0],
                      [162.0, 54.7], [166.0, 53.2], [174.0, 75.7], [172.7, 61.1], [167.6, 55.7],
                      [151.1, 48.7], [164.5, 52.3], [163.5, 50.0], [152.0, 59.3], [169.0, 62.5],
                      [164.0, 55.7], [161.2, 54.8], [155.0, 45.9], [170.0, 70.6], [176.2, 67.2],
                      [170.0, 69.4], [162.5, 58.2], [170.3, 64.8], [164.1, 71.6], [169.5, 52.8],
                      [163.2, 59.8], [154.5, 49.0], [159.8, 50.0], [173.2, 69.2], [170.0, 55.9],
                      [161.4, 63.4], [169.0, 58.2], [166.2, 58.6], [159.4, 45.7], [162.5, 52.2],
                      [159.0, 48.6], [162.8, 57.8], [159.0, 55.6], [179.8, 66.8], [162.9, 59.4],
                      [161.0, 53.6], [151.1, 73.2], [168.2, 53.4], [168.9, 69.0], [173.2, 58.4],
                      [171.8, 56.2], [178.0, 70.6], [164.3, 59.8], [163.0, 72.0], [168.5, 65.2],
                      [166.8, 56.6], [172.7, 105.2], [163.5, 51.8], [169.4, 63.4], [167.8, 59.0],
                      [159.5, 47.6], [167.6, 63.0], [161.2, 55.2], [160.0, 45.0], [163.2, 54.0],
                      [162.2, 50.2], [161.3, 60.2], [149.5, 44.8], [157.5, 58.8], [163.2, 56.4],
                      [172.7, 62.0], [155.0, 49.2], [156.5, 67.2], [164.0, 53.8], [160.9, 54.4],
                      [162.8, 58.0], [167.0, 59.8], [160.0, 54.8], [160.0, 43.2], [168.9, 60.5],
                      [158.2, 46.4], [156.0, 64.4], [160.0, 48.8], [167.1, 62.2], [158.0, 55.5],
                      [167.6, 57.8], [156.0, 54.6], [162.1, 59.2], [173.4, 52.7], [159.8, 53.2],
                      [170.5, 64.5], [159.2, 51.8], [157.5, 56.0], [161.3, 63.6], [162.6, 63.2],
                      [160.0, 59.5], [168.9, 56.8], [165.1, 64.1], [162.6, 50.0], [165.1, 72.3],
                      [166.4, 55.0], [160.0, 55.9], [152.4, 60.4], [170.2, 69.1], [162.6, 84.5],
                      [170.2, 55.9], [158.8, 55.5], [172.7, 69.5], [167.6, 76.4], [162.6, 61.4],
                      [167.6, 65.9], [156.2, 58.6], [175.2, 66.8], [172.1, 56.6], [162.6, 58.6],
                      [160.0, 55.9], [165.1, 59.1], [182.9, 81.8], [166.4, 70.7], [165.1, 56.8],
                      [177.8, 60.0], [165.1, 58.2], [175.3, 72.7], [154.9, 54.1], [158.8, 49.1],
                      [172.7, 75.9], [168.9, 55.0], [161.3, 57.3], [167.6, 55.0], [165.1, 65.5],
                      [175.3, 65.5], [157.5, 48.6], [163.8, 58.6], [167.6, 63.6], [165.1, 55.2],
                      [165.1, 62.7], [168.9, 56.6], [162.6, 53.9], [164.5, 63.2], [176.5, 73.6],
                      [168.9, 62.0], [175.3, 63.6], [159.4, 53.2], [160.0, 53.4], [170.2, 55.0],
                      [162.6, 70.5], [167.6, 54.5], [162.6, 54.5], [160.7, 55.9], [160.0, 59.0],
                      [157.5, 63.6], [162.6, 54.5], [152.4, 47.3], [170.2, 67.7], [165.1, 80.9],
                      [172.7, 70.5], [165.1, 60.9], [170.2, 63.6], [170.2, 54.5], [170.2, 59.1],
                      [161.3, 70.5], [167.6, 52.7], [167.6, 62.7], [165.1, 86.3], [162.6, 66.4],
                      [152.4, 67.3], [168.9, 63.0], [170.2, 73.6], [175.2, 62.3], [175.2, 57.7],
                      [160.0, 55.4], [165.1, 104.1], [174.0, 55.5], [170.2, 77.3], [160.0, 80.5],
                      [167.6, 64.5], [167.6, 72.3], [167.6, 61.4], [154.9, 58.2], [162.6, 81.8],
                      [175.3, 63.6], [171.4, 53.4], [157.5, 54.5], [165.1, 53.6], [160.0, 60.0],
                      [174.0, 73.6], [162.6, 61.4], [174.0, 55.5], [162.6, 63.6], [161.3, 60.9],
                      [156.2, 60.0], [149.9, 46.8], [169.5, 57.3], [160.0, 64.1], [175.3, 63.6],
                      [169.5, 67.3], [160.0, 75.5], [172.7, 68.2], [162.6, 61.4], [157.5, 76.8],
                      [176.5, 71.8], [164.4, 55.5], [160.7, 48.6], [174.0, 66.4], [163.8, 67.3]]


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
            :name => 'Male',
The file used for rendering the analytic view. The changes made include:
            :color => 'rgba(119, 152, 191, .5)',
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 => [[174.0, 65.6], [175.3, 71.8], [193.5, 80.7], [186.5, 72.6], [187.2, 78.8],
data_type_list = @available_data_types[params[:scope].to_sym] & @available_data_types[params[:type].to_sym]
                      [181.5, 74.8], [184.0, 86.4], [184.5, 78.4], [175.0, 62.0], [184.0, 81.6],
                      [180.0, 76.6], [177.8, 83.6], [192.0, 90.0], [176.0, 74.6], [174.0, 71.0],
                      [184.0, 79.6], [192.7, 93.8], [171.5, 70.0], [173.0, 72.4], [176.0, 85.9],
                      [176.0, 78.8], [180.5, 77.8], [172.7, 66.2], [176.0, 86.4], [173.5, 81.8],
                      [178.0, 89.6], [180.3, 82.8], [180.3, 76.4], [164.5, 63.2], [173.0, 60.9],
                      [183.5, 74.8], [175.5, 70.0], [188.0, 72.4], [189.2, 84.1], [172.8, 69.1],
                      [170.0, 59.5], [182.0, 67.2], [170.0, 61.3], [177.8, 68.6], [184.2, 80.1],
                      [186.7, 87.8], [171.4, 84.7], [172.7, 73.4], [175.3, 72.1], [180.3, 82.6],
                      [182.9, 88.7], [188.0, 84.1], [177.2, 94.1], [172.1, 74.9], [167.0, 59.1],
                      [169.5, 75.6], [174.0, 86.2], [172.7, 75.3], [182.2, 87.1], [164.1, 55.2],
                      [163.0, 57.0], [171.5, 61.4], [184.2, 76.8], [174.0, 86.8], [174.0, 72.2],
                      [177.0, 71.6], [186.0, 84.8], [167.0, 68.2], [171.8, 66.1], [182.0, 72.0],
                      [167.0, 64.6], [177.8, 74.8], [164.5, 70.0], [192.0, 101.6], [175.5, 63.2],
                      [171.2, 79.1], [181.6, 78.9], [167.4, 67.7], [181.1, 66.0], [177.0, 68.2],
                      [174.5, 63.9], [177.5, 72.0], [170.5, 56.8], [182.4, 74.5], [197.1, 90.9],
                      [180.1, 93.0], [175.5, 80.9], [180.6, 72.7], [184.4, 68.0], [175.5, 70.9],
                      [180.6, 72.5], [177.0, 72.5], [177.1, 83.4], [181.6, 75.5], [176.5, 73.0],
                      [175.0, 70.2], [174.0, 73.4], [165.1, 70.5], [177.0, 68.9], [192.0, 102.3],
                      [176.5, 68.4], [169.4, 65.9], [182.1, 75.7], [179.8, 84.5], [175.3, 87.7],
                      [184.9, 86.4], [177.3, 73.2], [167.4, 53.9], [178.1, 72.0], [168.9, 55.5],
                      [157.2, 58.4], [180.3, 83.2], [170.2, 72.7], [177.8, 64.1], [172.7, 72.3],
                      [165.1, 65.0], [186.7, 86.4], [165.1, 65.0], [174.0, 88.6], [175.3, 84.1],
                      [185.4, 66.8], [177.8, 75.5], [180.3, 93.2], [180.3, 82.7], [177.8, 58.0],
                      [177.8, 79.5], [177.8, 78.6], [177.8, 71.8], [177.8, 116.4], [163.8, 72.2],
                      [188.0, 83.6], [198.1, 85.5], [175.3, 90.9], [166.4, 85.9], [190.5, 89.1],
                      [166.4, 75.0], [177.8, 77.7], [179.7, 86.4], [172.7, 90.9], [190.5, 73.6],
                      [185.4, 76.4], [168.9, 69.1], [167.6, 84.5], [175.3, 64.5], [170.2, 69.1],
                      [190.5, 108.6], [177.8, 86.4], [190.5, 80.9], [177.8, 87.7], [184.2, 94.5],
                      [176.5, 80.2], [177.8, 72.0], [180.3, 71.4], [171.4, 72.7], [172.7, 84.1],
                      [172.7, 76.8], [177.8, 63.6], [177.8, 80.9], [182.9, 80.9], [170.2, 85.5],
                      [167.6, 68.6], [175.3, 67.7], [165.1, 66.4], [185.4, 102.3], [181.6, 70.5],
                      [172.7, 95.9], [190.5, 84.1], [179.1, 87.3], [175.3, 71.8], [170.2, 65.9],
                      [193.0, 95.9], [171.4, 91.4], [177.8, 81.8], [177.8, 96.8], [167.6, 69.1],
                      [167.6, 82.7], [180.3, 75.5], [182.9, 79.5], [176.5, 73.6], [186.7, 91.8],
                      [188.0, 84.1], [188.0, 85.9], [177.8, 81.8], [174.0, 82.5], [177.8, 80.5],
                      [171.4, 70.0], [185.4, 81.8], [185.4, 84.1], [188.0, 90.5], [188.0, 91.4],
                      [182.9, 89.1], [176.5, 85.0], [175.3, 69.1], [175.3, 73.6], [188.0, 80.5],
                      [188.0, 82.7], [175.3, 86.4], [170.5, 67.7], [179.1, 92.7], [177.8, 93.6],
                      [175.3, 70.9], [182.9, 75.0], [170.8, 93.2], [188.0, 93.2], [180.3, 77.7],
                      [177.8, 61.4], [185.4, 94.1], [168.9, 75.0], [185.4, 83.6], [180.3, 85.5],
                      [174.0, 73.9], [167.6, 66.8], [182.9, 87.3], [160.0, 72.3], [180.3, 88.6],
                      [167.6, 75.5], [186.7, 101.4], [175.3, 91.1], [175.3, 67.3], [175.9, 77.7],
                      [175.3, 81.8], [179.1, 75.5], [181.6, 84.5], [177.8, 76.6], [182.9, 85.0],
                      [177.8, 102.5], [184.2, 77.3], [179.1, 71.8], [176.5, 87.9], [188.0, 94.3],
                      [174.0, 70.9], [167.6, 64.5], [170.2, 77.3], [167.6, 72.3], [188.0, 87.3],
                      [174.0, 80.0], [176.5, 82.3], [180.3, 73.6], [167.6, 74.1], [188.0, 85.9],
                      [180.3, 73.2], [167.6, 76.3], [183.0, 65.9], [183.0, 90.9], [179.1, 89.1],
                      [170.2, 62.3], [177.8, 82.7], [179.1, 79.1], [190.5, 98.2], [177.8, 84.1],
                      [180.3, 83.2], [180.3, 83.2]]
          }]
      }
    }
  end
</pre>
'''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 data part for scatter plots contains unnecessary data which is used as sample data. A large part of can be removed as this data is only for indicating how data should be arranged for scatter 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. It also does not violate the DRY principle.


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.
  def self.get_bar_template()
Hence, the data type list was always empty as shown in figure 1.
    generic_template = get_generic_template  #Adding :chart object to the generic template
    generic_template[:chart] = {
        :type => 'column'
    }
    generic_template
  end


This was corrected by converting the list of string to a list of symbols and then taking an intersection as shown below:
  def self.get_line_template()
data_type_list =  @available_data_types[params[:scope].to_sym] & (@available_data_types[params[:type].to_sym].map &:to_sym)
    get_generic_template
This change is a bug fix.
  end
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)
<pre>
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)
  def self.get_generic_template() #Currently used for bar and line graphs
    {
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
            :title =>{},
            :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
            :min => 0,
            :title => {
                :text => 'score'
            }
        },
        :tooltip => {
            :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
            :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f} mm</b></td></tr>',
            :footerFormat => '</table>',
            :shared => true,
            :useHTML => true
        },
        :plotOptions => {
            :column => {
                :pointPadding => 0.2,
                :borderWidth => 0
            }
        },
        :series => [
            {
                :name => 'review 1',
                :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
            }, {
                :name => 'review 2',
                :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
            }, {
                :name => 'review 3',
                :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
            }
        ]
    }
  end
</pre>


=Bonus Changes=


2) app/helpers/analytic_helper.rb
Going beyond the scope of the project, the code for the analytic controller and helper was analyzed and fixed.  
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.
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:
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
'''Issue 1'''
The file which shows the view for the analytics. The changes made to this file include:
----
Enabling options for line and pie charts.
:'Bar graph' was the only chart supported
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.
[[File:Existing View.png|frame|center|Figure 1: The existing page]]
:The modified code now provides support for the line and pie charts along with the bar graphs using the existing underlying framework.  
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.     
'''Issue 2'''
----
:Probably because of the way ruby makes database queries by default, data fetching takes a long time. The waiting time was in the 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


Figure 3: Friendly error message to guide users
'''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<br>


To render the different types of graph, apart from the given requirement of the project and to fix the issues mentioned above, 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: <br>
'''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>[http://c2.com/cgi/wiki?SwitchStatementsSmell 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>[http://en.wikipedia.org/wiki/Principles_of_user_interface_design Principles of User Interface design] ''Last Modified'' 24 July 2014</ref>
[[File:PleaseWait.png|frame|center|Figure 2: Message to show chart is being processed]]
'''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.
[[File:ParametersNotSelected.png|frame|center|Figure 3: Friendly error message to guide users]]


=Summary of Revised Design Principles=


{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|File name
! style="width:13%;"|Method Name
! style="width:33%;"|Comment on Design Principle
|- style="vertical-align:top;"
|''''' 1. '''''
|chart.rb
|dataAdapter
|Violated the Single Responsibility Principle and the DRY Principle
|-
|
|
|dataTemplate
|Violated the Single Responsibility Principle and the DRY 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)


The END Result
==Sample Bar Chart==
The analytics view now supports line and pie chart options along with the Baroption(which was not working earlier)
[[File:BarChart.png|frame|center|Figure 4: Bar Chart]]
Figure 4: Bar Chart




Figure 5: Line Graph
==Sample Line Graph==
[[File:LineGraph.png|frame|center|Figure 5: Line Graph]]




==Sample Pie Chart==
[[File:PieChart.png|frame|center|Figure 6: Pie chart]]


Figure 6: Pie chart
=References=
<references/>
<references/>

Latest revision as of 03:42, 6 November 2014

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


Installation Instructions

Follow the steps to view the changes,

Step 1: Use URL http://152.46.16.178:3001/ to connect to the expertiza vcl reservation

Step 2: Log into expertiza with the following credentials, user: user2 password: password

Step 3: Type in http://152.46.16.178:3001/analytic/index.html in your browser

Step 4: Select the appropriate options for comparison

Note:

  • There are some courses that have empty data and may not work.
As a safe option, try these, :)
  • Graph Type: Bar chart
  • Comparison scope: team
  • Course: CSC 517, Spring 2014
  • Assignment: Backchannel, Spring 2014
  • Team: select all teams
  • Data:
-average review character count
-max review character count
-min review character count
  • For the 'pie chart',
The chart is generated based on the first option in the data that is selected

(Since it was beyond the scope of our project requirements, we just got the chart working)


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
    1. Too long, needs to be divided
    2. Too many if-else statements
  • self.data_template method
    1. Needs to be broken down or moved according to requirement
    2. Define each template independently instead of in the same method
    3. Remove unecessary data from :series for scatter plot
    4. 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:

  • The new code does not violate the Single Responsibility Principle anymore. This is achieved by Separation of Concerns. Each function is divided into a smaller module and focuses on one small task.
 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
  • Moved code for setting optional parameters to function set_template_optional_params
 def self.set_template_optional_params(template)
   template[:title][:text] = ""
   template.delete(:subtitle)
   template.delete(:yAxis)
   template.delete(:xAxis)
   template
 end
  • Moved code for validation of optional parameters to function validate_optional_conf
 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

The code is a lot more cleaner, readable and maintainable now.

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' (data_template function is responsible for all types of charts). It also duplicated some code(code for line and bar) which resulted in a violation of a DRY Principle.

Original code:

   def self.data_template()
    {
      :pie => {
        :chart => {
          :plotBackgroundColor => nil,
          :plotBorderWidth => nil,
          :plotShadow => false
        },
        :title => {
          :text => 'Title'
        },
        :subtitle => {
          :text => 'Title'
        },
        :xAxis => {},
        :yAxis => {},
        :tooltip => {
          :pointFormat => '{series.name}: <b>{point.percentage}%</b>',
          :percentageDecimals => 1
        },
        :plotOptions => {
          :pie => {
            :allowPointSelect => true,
            :cursor => 'pointer',
            :dataLabels => {
              :enabled => true,
              :color => '#000000',
              :connectorColor => '#000000',
              :format => "<b>{point.name}</b>: {percentage:.2f} %"
            }
          }
        },
        :series => [
          {
            :type => 'pie',
            :name => 'XXX pie',
            :data => [
              ['part 1',45.0],
              ['part 2',26.8],
              ['part 3',8.5],
              ['part 4',6.2],
              ['part 5',0.7]
            ]
          }
        ]
      },
      :bar => {
        :chart => {
          :type => 'column'
        },
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
          :title =>{},
          :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
          :min => 0,
          :title => {
            :text => 'score'
          }
        },
        :tooltip => {
          :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
          :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f}</b></td></tr>',
          :footerFormat => '</table>',
          :shared => true,
          :useHTML => true
        },
        :plotOptions => {
          :column => {
            :pointPadding => 0.2,
            :borderWidth => 0
          }
        },
        :series => [
          {
            :name => 'review 1',
            :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
          }, {
            :name => 'review 2',
            :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
          }, {
            :name => 'review 3',
            :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
          }
        ]
      },

      :line => {
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
          :title =>{},
          :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
          :min => 0,
          :title => {
            :text => 'score'
          }
        },
        :tooltip => {
          :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
          :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f} mm</b></td></tr>',
          :footerFormat => '</table>',
          :shared => true,
          :useHTML => true
        },
        :plotOptions => {
          :column => {
            :pointPadding => 0.2,
            :borderWidth => 0
          }
        },
        :series => [
          {
            :name => 'review 1',
            :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
          }, {
            :name => 'review 2',
            :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
          }, {
            :name => 'review 3',
            :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
          }
        ]
      },

      :scatter => {
        :chart => {
          :type => 'scatter',
          :zoomType => 'xy'
        },
        :title => {
          :text => 'Height Versus Weight of 507 Individuals by Gender'
        },
        :subtitle => {
          :text => 'Source: Heinz  2003'
        },
        :xAxis => {
          :title => {
            :enabled => true,
            :text => 'Height (cm)'
          },
          :startOnTick => true,
          :endOnTick => true,
          :showLastLabel => true
        },
        :yAxis => {
          :title => {
            :text => 'Weight (kg)'
          }
        },
        :legend => {
          :layout => 'vertical',
          :align => 'left',
          :verticalAlign => 'top',
          :x => 100,
          :y => 70,
          :floating => true,
          :backgroundColor => '#FFFFFF',
          :borderWidth => 1
        },
        :plotOptions => {
          :scatter => {
            :marker => {
              :radius => 5,
              :states => {
                :hover => {
                  :enabled => true,
                  :lineColor => 'rgb(100,100,100)'
                }
              }
            },
            :states => {
              :hover => {
                :marker => {
                  :enabled => false
                }
              }
            },
            :tooltip => {
              :headerFormat => '<b>{series.name}</b><br>',
              :pointFormat => '{point.x} cm, {point.y} kg'
            }
          }
        },
        :series => [
          {
            :name => 'Female',
            :color => 'rgba(223, 83, 83, .5)',
            :data => [[161.2, 51.6], [167.5, 59.0], [159.5, 49.2], [157.0, 63.0], [155.8, 53.6],
                      [170.0, 59.0], [159.1, 47.6], [166.0, 69.8], [176.2, 66.8], [160.2, 75.2],
                      [172.5, 55.2], [170.9, 54.2], [172.9, 62.5], [153.4, 42.0], [160.0, 50.0],
                      [147.2, 49.8], [168.2, 49.2], [175.0, 73.2], [157.0, 47.8], [167.6, 68.8],
                      [159.5, 50.6], [175.0, 82.5], [166.8, 57.2], [176.5, 87.8], [170.2, 72.8],
                      [174.0, 54.5], [173.0, 59.8], [179.9, 67.3], [170.5, 67.8], [160.0, 47.0],
                      [154.4, 46.2], [162.0, 55.0], [176.5, 83.0], [160.0, 54.4], [152.0, 45.8],
                      [162.1, 53.6], [170.0, 73.2], [160.2, 52.1], [161.3, 67.9], [166.4, 56.6],
                      [168.9, 62.3], [163.8, 58.5], [167.6, 54.5], [160.0, 50.2], [161.3, 60.3],
                      [167.6, 58.3], [165.1, 56.2], [160.0, 50.2], [170.0, 72.9], [157.5, 59.8],
                      [167.6, 61.0], [160.7, 69.1], [163.2, 55.9], [152.4, 46.5], [157.5, 54.3],
                      [168.3, 54.8], [180.3, 60.7], [165.5, 60.0], [165.0, 62.0], [164.5, 60.3],
                      [156.0, 52.7], [160.0, 74.3], [163.0, 62.0], [165.7, 73.1], [161.0, 80.0],
                      [162.0, 54.7], [166.0, 53.2], [174.0, 75.7], [172.7, 61.1], [167.6, 55.7],
                      [151.1, 48.7], [164.5, 52.3], [163.5, 50.0], [152.0, 59.3], [169.0, 62.5],
                      [164.0, 55.7], [161.2, 54.8], [155.0, 45.9], [170.0, 70.6], [176.2, 67.2],
                      [170.0, 69.4], [162.5, 58.2], [170.3, 64.8], [164.1, 71.6], [169.5, 52.8],
                      [163.2, 59.8], [154.5, 49.0], [159.8, 50.0], [173.2, 69.2], [170.0, 55.9],
                      [161.4, 63.4], [169.0, 58.2], [166.2, 58.6], [159.4, 45.7], [162.5, 52.2],
                      [159.0, 48.6], [162.8, 57.8], [159.0, 55.6], [179.8, 66.8], [162.9, 59.4],
                      [161.0, 53.6], [151.1, 73.2], [168.2, 53.4], [168.9, 69.0], [173.2, 58.4],
                      [171.8, 56.2], [178.0, 70.6], [164.3, 59.8], [163.0, 72.0], [168.5, 65.2],
                      [166.8, 56.6], [172.7, 105.2], [163.5, 51.8], [169.4, 63.4], [167.8, 59.0],
                      [159.5, 47.6], [167.6, 63.0], [161.2, 55.2], [160.0, 45.0], [163.2, 54.0],
                      [162.2, 50.2], [161.3, 60.2], [149.5, 44.8], [157.5, 58.8], [163.2, 56.4],
                      [172.7, 62.0], [155.0, 49.2], [156.5, 67.2], [164.0, 53.8], [160.9, 54.4],
                      [162.8, 58.0], [167.0, 59.8], [160.0, 54.8], [160.0, 43.2], [168.9, 60.5],
                      [158.2, 46.4], [156.0, 64.4], [160.0, 48.8], [167.1, 62.2], [158.0, 55.5],
                      [167.6, 57.8], [156.0, 54.6], [162.1, 59.2], [173.4, 52.7], [159.8, 53.2],
                      [170.5, 64.5], [159.2, 51.8], [157.5, 56.0], [161.3, 63.6], [162.6, 63.2],
                      [160.0, 59.5], [168.9, 56.8], [165.1, 64.1], [162.6, 50.0], [165.1, 72.3],
                      [166.4, 55.0], [160.0, 55.9], [152.4, 60.4], [170.2, 69.1], [162.6, 84.5],
                      [170.2, 55.9], [158.8, 55.5], [172.7, 69.5], [167.6, 76.4], [162.6, 61.4],
                      [167.6, 65.9], [156.2, 58.6], [175.2, 66.8], [172.1, 56.6], [162.6, 58.6],
                      [160.0, 55.9], [165.1, 59.1], [182.9, 81.8], [166.4, 70.7], [165.1, 56.8],
                      [177.8, 60.0], [165.1, 58.2], [175.3, 72.7], [154.9, 54.1], [158.8, 49.1],
                      [172.7, 75.9], [168.9, 55.0], [161.3, 57.3], [167.6, 55.0], [165.1, 65.5],
                      [175.3, 65.5], [157.5, 48.6], [163.8, 58.6], [167.6, 63.6], [165.1, 55.2],
                      [165.1, 62.7], [168.9, 56.6], [162.6, 53.9], [164.5, 63.2], [176.5, 73.6],
                      [168.9, 62.0], [175.3, 63.6], [159.4, 53.2], [160.0, 53.4], [170.2, 55.0],
                      [162.6, 70.5], [167.6, 54.5], [162.6, 54.5], [160.7, 55.9], [160.0, 59.0],
                      [157.5, 63.6], [162.6, 54.5], [152.4, 47.3], [170.2, 67.7], [165.1, 80.9],
                      [172.7, 70.5], [165.1, 60.9], [170.2, 63.6], [170.2, 54.5], [170.2, 59.1],
                      [161.3, 70.5], [167.6, 52.7], [167.6, 62.7], [165.1, 86.3], [162.6, 66.4],
                      [152.4, 67.3], [168.9, 63.0], [170.2, 73.6], [175.2, 62.3], [175.2, 57.7],
                      [160.0, 55.4], [165.1, 104.1], [174.0, 55.5], [170.2, 77.3], [160.0, 80.5],
                      [167.6, 64.5], [167.6, 72.3], [167.6, 61.4], [154.9, 58.2], [162.6, 81.8],
                      [175.3, 63.6], [171.4, 53.4], [157.5, 54.5], [165.1, 53.6], [160.0, 60.0],
                      [174.0, 73.6], [162.6, 61.4], [174.0, 55.5], [162.6, 63.6], [161.3, 60.9],
                      [156.2, 60.0], [149.9, 46.8], [169.5, 57.3], [160.0, 64.1], [175.3, 63.6],
                      [169.5, 67.3], [160.0, 75.5], [172.7, 68.2], [162.6, 61.4], [157.5, 76.8],
                      [176.5, 71.8], [164.4, 55.5], [160.7, 48.6], [174.0, 66.4], [163.8, 67.3]]

          }, {
            :name => 'Male',
            :color => 'rgba(119, 152, 191, .5)',
            :data => [[174.0, 65.6], [175.3, 71.8], [193.5, 80.7], [186.5, 72.6], [187.2, 78.8],
                      [181.5, 74.8], [184.0, 86.4], [184.5, 78.4], [175.0, 62.0], [184.0, 81.6],
                      [180.0, 76.6], [177.8, 83.6], [192.0, 90.0], [176.0, 74.6], [174.0, 71.0],
                      [184.0, 79.6], [192.7, 93.8], [171.5, 70.0], [173.0, 72.4], [176.0, 85.9],
                      [176.0, 78.8], [180.5, 77.8], [172.7, 66.2], [176.0, 86.4], [173.5, 81.8],
                      [178.0, 89.6], [180.3, 82.8], [180.3, 76.4], [164.5, 63.2], [173.0, 60.9],
                      [183.5, 74.8], [175.5, 70.0], [188.0, 72.4], [189.2, 84.1], [172.8, 69.1],
                      [170.0, 59.5], [182.0, 67.2], [170.0, 61.3], [177.8, 68.6], [184.2, 80.1],
                      [186.7, 87.8], [171.4, 84.7], [172.7, 73.4], [175.3, 72.1], [180.3, 82.6],
                      [182.9, 88.7], [188.0, 84.1], [177.2, 94.1], [172.1, 74.9], [167.0, 59.1],
                      [169.5, 75.6], [174.0, 86.2], [172.7, 75.3], [182.2, 87.1], [164.1, 55.2],
                      [163.0, 57.0], [171.5, 61.4], [184.2, 76.8], [174.0, 86.8], [174.0, 72.2],
                      [177.0, 71.6], [186.0, 84.8], [167.0, 68.2], [171.8, 66.1], [182.0, 72.0],
                      [167.0, 64.6], [177.8, 74.8], [164.5, 70.0], [192.0, 101.6], [175.5, 63.2],
                      [171.2, 79.1], [181.6, 78.9], [167.4, 67.7], [181.1, 66.0], [177.0, 68.2],
                      [174.5, 63.9], [177.5, 72.0], [170.5, 56.8], [182.4, 74.5], [197.1, 90.9],
                      [180.1, 93.0], [175.5, 80.9], [180.6, 72.7], [184.4, 68.0], [175.5, 70.9],
                      [180.6, 72.5], [177.0, 72.5], [177.1, 83.4], [181.6, 75.5], [176.5, 73.0],
                      [175.0, 70.2], [174.0, 73.4], [165.1, 70.5], [177.0, 68.9], [192.0, 102.3],
                      [176.5, 68.4], [169.4, 65.9], [182.1, 75.7], [179.8, 84.5], [175.3, 87.7],
                      [184.9, 86.4], [177.3, 73.2], [167.4, 53.9], [178.1, 72.0], [168.9, 55.5],
                      [157.2, 58.4], [180.3, 83.2], [170.2, 72.7], [177.8, 64.1], [172.7, 72.3],
                      [165.1, 65.0], [186.7, 86.4], [165.1, 65.0], [174.0, 88.6], [175.3, 84.1],
                      [185.4, 66.8], [177.8, 75.5], [180.3, 93.2], [180.3, 82.7], [177.8, 58.0],
                      [177.8, 79.5], [177.8, 78.6], [177.8, 71.8], [177.8, 116.4], [163.8, 72.2],
                      [188.0, 83.6], [198.1, 85.5], [175.3, 90.9], [166.4, 85.9], [190.5, 89.1],
                      [166.4, 75.0], [177.8, 77.7], [179.7, 86.4], [172.7, 90.9], [190.5, 73.6],
                      [185.4, 76.4], [168.9, 69.1], [167.6, 84.5], [175.3, 64.5], [170.2, 69.1],
                      [190.5, 108.6], [177.8, 86.4], [190.5, 80.9], [177.8, 87.7], [184.2, 94.5],
                      [176.5, 80.2], [177.8, 72.0], [180.3, 71.4], [171.4, 72.7], [172.7, 84.1],
                      [172.7, 76.8], [177.8, 63.6], [177.8, 80.9], [182.9, 80.9], [170.2, 85.5],
                      [167.6, 68.6], [175.3, 67.7], [165.1, 66.4], [185.4, 102.3], [181.6, 70.5],
                      [172.7, 95.9], [190.5, 84.1], [179.1, 87.3], [175.3, 71.8], [170.2, 65.9],
                      [193.0, 95.9], [171.4, 91.4], [177.8, 81.8], [177.8, 96.8], [167.6, 69.1],
                      [167.6, 82.7], [180.3, 75.5], [182.9, 79.5], [176.5, 73.6], [186.7, 91.8],
                      [188.0, 84.1], [188.0, 85.9], [177.8, 81.8], [174.0, 82.5], [177.8, 80.5],
                      [171.4, 70.0], [185.4, 81.8], [185.4, 84.1], [188.0, 90.5], [188.0, 91.4],
                      [182.9, 89.1], [176.5, 85.0], [175.3, 69.1], [175.3, 73.6], [188.0, 80.5],
                      [188.0, 82.7], [175.3, 86.4], [170.5, 67.7], [179.1, 92.7], [177.8, 93.6],
                      [175.3, 70.9], [182.9, 75.0], [170.8, 93.2], [188.0, 93.2], [180.3, 77.7],
                      [177.8, 61.4], [185.4, 94.1], [168.9, 75.0], [185.4, 83.6], [180.3, 85.5],
                      [174.0, 73.9], [167.6, 66.8], [182.9, 87.3], [160.0, 72.3], [180.3, 88.6],
                      [167.6, 75.5], [186.7, 101.4], [175.3, 91.1], [175.3, 67.3], [175.9, 77.7],
                      [175.3, 81.8], [179.1, 75.5], [181.6, 84.5], [177.8, 76.6], [182.9, 85.0],
                      [177.8, 102.5], [184.2, 77.3], [179.1, 71.8], [176.5, 87.9], [188.0, 94.3],
                      [174.0, 70.9], [167.6, 64.5], [170.2, 77.3], [167.6, 72.3], [188.0, 87.3],
                      [174.0, 80.0], [176.5, 82.3], [180.3, 73.6], [167.6, 74.1], [188.0, 85.9],
                      [180.3, 73.2], [167.6, 76.3], [183.0, 65.9], [183.0, 90.9], [179.1, 89.1],
                      [170.2, 62.3], [177.8, 82.7], [179.1, 79.1], [190.5, 98.2], [177.8, 84.1],
                      [180.3, 83.2], [180.3, 83.2]]
          }]
      }
    }
  end

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 data part for scatter plots contains unnecessary data which is used as sample data. A large part of can be removed as this data is only for indicating how data should be arranged for scatter 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. It also does not violate the DRY principle.

 def self.get_bar_template()
   generic_template = get_generic_template  #Adding :chart object to the generic template
   generic_template[:chart] = {
       :type => 'column'
   }
   generic_template
 end
 def self.get_line_template()
   get_generic_template
 end
  def self.get_generic_template() #Currently used for bar and line graphs
    {
        :title => {:text => "Review score for XXXX"},
        :subtitle => {:text => "subtitle here"},
        :xAxis => {
            :title =>{},
            :categories => [ 'Problem1', 'Problem2', 'Problem3', 'Problem4', 'Problem5', 'Problem6', 'Problem7', 'Problem8', 'Problem9', 'Problem10', 'Problem11','Problem12']
        },
        :yAxis => {
            :min => 0,
            :title => {
                :text => 'score'
            }
        },
        :tooltip => {
            :headerFormat => '<span style="font-size:10px">{point.key}</span><table>',
            :pointFormat => '<tr><td style="color:{series.color};padding:0">{series.name}: </td>' +'<td style="padding:0"><b>{point.y:.1f} mm</b></td></tr>',
            :footerFormat => '</table>',
            :shared => true,
            :useHTML => true
        },
        :plotOptions => {
            :column => {
                :pointPadding => 0.2,
                :borderWidth => 0
            }
        },
        :series => [
            {
                :name => 'review 1',
                :data => [9.9, 7.5, 6.4, 9.2, 4.0, 6.0, 5.6, 8.5, 6.4, 4.1, 5.6, 4.4]
            }, {
                :name => 'review 2',
                :data =>  [3.6, 8.8, 8.5, 3.4, 6.0, 4.5, 5.0, 4.3, 9.2, 8.5, 6.6, 9.3]
            }, {
                :name => 'review 3',
                :data => [8.9, 8.8, 9.3, 4.4, 7.0, 8.3, 9.0, 9.6, 5.4, 6.2, 9.3, 5.2]
            }
        ]
    }
  end

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
Figure 1: The existing page
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. The waiting time was in the 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 and to fix the issues mentioned above, 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>
Figure 2: Message to show chart is being processed

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.

Figure 3: Friendly error message to guide users

Summary of Revised Design Principles

Sr. No. File name Method Name Comment on Design Principle
1. chart.rb dataAdapter Violated the Single Responsibility Principle and the DRY Principle
dataTemplate Violated the Single Responsibility Principle and the DRY 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

Figure 4: Bar Chart


Sample Line Graph

Figure 5: Line Graph


Sample Pie Chart

Figure 6: Pie chart

References

<references/>