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

• GitHub Repository
• VCL IP (Log in as user2/password. Go to VCL analytic page)
• 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)

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

• chart.rb
• analytic_helper.rb
• analytic_controller.rb
• views/analytic/index.html.erb

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/>