CSC/ECE 517 Fall 2014/OSS E1455 skn
Jump to navigation
Jump to search
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
- As a safe option, try these, :)
- 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
- Too long, needs to be divided
- Too many if-else statements
- self.data_template method
- Needs to be broken down or moved according to requirement
- Define each template independently instead of in the same method
- Remove unecessary data from :series for scatter plot
- Method has duplicate data. Remove the duplicate occurrences
Design Changes
To fulfill the requirements, the following changes were implemented,
Refactoring the dataAdapter method
What it does
- The dataAdapter function is basically an initialization function which sets parameters for the graphical plot to some default starting values based on the type of analytic view the user wants to rendered. There was quite a bit of code duplication in the existing function, especially when it came to setting parameters for some specific chart types like pie charts for instance.
- The code in this function violated the 'Single Responsibility Principle'.
Original code:
def self.dataAdapter(type,data,optionalConf)
template = data_template[type];
if (type == :pie) then
data[:type] = 'pie';
template[:series] = [data]
else
template[:series] = data
end
if optionalConf.nil? then
template[:title][:text] = ""
template.delete(:subtitle)
template.delete(:yAxis)
template.delete(:xAxis)
else
if template[:subtitle].nil? then
template[:subtitle]={}
end
if optionalConf[:title].nil? then
template[:title][:text] = ""
else
template[:title][:text] = optionalConf[:title]
end
if optionalConf[:subtitle].nil? then
template.delete(:subtitle)
else
template[:subtitle][:text]=optionalConf[:subtitle]
end
if optionalConf[:y_axis].nil? then
template.delete(:yAxis)
else
template[:yAxis][:title][:text]=optionalConf[:y_axis]
end
if optionalConf[:x_axis].nil? then
template[:xAxis].delete(:title)
else
template[:xAxis][:title][:text] = optionalConf[:x_axis]
end
if optionalConf[:x_axis_categories].nil? then
template[:xAxis].delete(:categories)
else
template[:xAxis][:categories]=optionalConf[:x_axis_categories]
end
end
template
end
Changes made:
- 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
- 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>
Development Note: The retrieval queries should be inspected to see if the data retrieval can be made faster. Since the chart rendering is fired whenever there is a change in selection of the data point and the comparison scope, we have added a friendly error message which tells the user to choose the options.
Summary of Revised Design Principles
| Sr. No. | File name | Method Name | Comment on Design Principle |
|---|---|---|---|
| 1. | chart.rb | dataAdapter | Violated the Single Responsibility Principle 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
Sample Line Graph
Sample Pie Chart
References
<references/>