CSC/ECE 517 Fall 2016/E1673. Refactor question type.rb: Difference between revisions
No edit summary |
No edit summary |
||
(53 intermediate revisions by 3 users not shown) | |||
Line 1: | Line 1: | ||
==Background== | |||
Expertiza is an open source software to create reusable learning objects through peer review. It also supports team projects, and the submission of almost any document type, including URLs and wiki pages. It has following features: | |||
*Enables students working together in a class and a topic | |||
*Enables studnets to do peer review and improve both him/her and others' learn experience | |||
== Reason for Refractor== | == Reason for Refractor== | ||
The purpose of this work is to rewrite part of the codes in the Criterion.rb, Scale.rb, Checkbox.rb, Dropdown.rb, Text_response.rb, Text_area.rb, Text_field.rb. This is because some of codes have bad writing sytle, or not followingg OO language rules, which confuse others to understand these code. The detail of the requirements are list below: | |||
*Line is too long | |||
*Some code found in other location and should put them in the parent class | |||
*Method has too many lines | |||
*Some of the code will cause security risk | |||
*Using the old style validations | |||
*Using a lower efficent way to do the loop | |||
All of these requirements are based on a analysis software called [https://codeclimate.com code climate] | |||
==Result== | |||
[[File:Lzhang45_1.png]] | |||
[[File:Lzhang45_2.png]] | |||
== Code Changes == | == Code Changes == | ||
===text_field.rb=== | |||
Although it increase the number of classes, the length of function decrease and it's easy to read than before | |||
1. Short the long line | |||
2. Improve security by using safe_join | |||
Previous | |||
html.html_safe | |||
end | Changed | ||
safe_join([" ".html_safe, " ".html_safe], html.html_safe) | |||
3. Short long method | |||
Previous | |||
html += '<label for="responses_' + count.to_s + '">' + self.txt + '</label>' | |||
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden" value="">' | |||
html += '<input id="responses_' + count.to_s + '_comments" label=' + self.txt + ' name="responses[' + count.to_s + '][comment]" size=' + self.size.to_s + ' type="text"' | |||
html += 'value="' + answer.comments unless answer.nil? | |||
html += '">' | |||
html += '</li><BR/><BR/>' if self.type == 'TextField' and self.break_before == false | |||
def complete(count, answer = nil) | |||
html = if self.type == 'TextField' and self.break_before == true | |||
html += combine(count, answer) | |||
end | |||
Changed | |||
def combline(count, answer = nil) | |||
html = complete_helper_response_scores(count) + complete_helper_response_comments(count) | |||
html += complete_helper_text(answer) + complete_helper_response_comments | |||
html += complete_helper_final_judge | |||
html | |||
end | |||
===text_response.rb=== | |||
It makes codes more readable. We also choose safe_join instead of html_safe to prevent security risks. However, we must use html_safe to keep the character encoding. | |||
1. Use new style validations | |||
Previous | |||
validates_presence_of :size | |||
Changed | |||
validates :size, presence: true | |||
2. Short the long line | |||
3. Improve security by safe_join | |||
Preivious | |||
html.html_safe | |||
Changed | |||
safe_join(["<tr>".html_safe, "</tr>".html_safe], html.html_safe) | |||
4. Short long method | |||
Preivious | |||
def edit(_count) | |||
html = '<tr>' | |||
html += '<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a></td>' | |||
html += '<td><input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text"></td>' | |||
html += '<td><textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea></td>' | |||
html += '<td><input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">''</td>' | |||
html += '<td><!--placeholder (TextResponse does not need weight)--></td>' | |||
html += '<td>text area size <input size="6" value="' + self.size.to_s + '" name="question[' + self.id.to_s + '][size]" id="question_' + self.id.to_s + '_size" type="text"></td>' | |||
html += '</tr>' | |||
html.html_safe | |||
end | end | ||
def | Changed | ||
html = self.text | def edit(_count) | ||
html += | html = edit_link + edit_question_lable | ||
html += '< | html += edit_question_textarea + edit_question_textarea_size | ||
html | safe_join(["<tr>".html_safe, "</tr>".html_safe], html.html_safe) | ||
end | end | ||
===scale.rb=== | |||
1. Use each loop instead of for loop | |||
Previous | |||
for j in questionnaire_min..questionnaire_max | |||
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"' | |||
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) | |||
html += '></td>' | |||
end | |||
Changed | |||
(questionnaire_min..questionnaire_max).each do |j| | |||
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"' | |||
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) | |||
html += '></td>' | |||
end | |||
2. Short the long line | |||
3. Use safe_join to be more security | |||
Previous | |||
html.html_safe | |||
Changed | |||
safe_join(["".html_safe, "".html_safe], html.html_safe | |||
4. Delete the code similar with other file and put the duplicate into parent class. | |||
5. Modify some method that is too long | |||
Previous | |||
def edit(_count) | |||
html = '<tr>' | |||
html += '<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a></td>' | |||
html += '<td><input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text"></td>' | |||
html += '<td><textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea></td>' | |||
html += '<td><input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">''</td>' | |||
html += '<td><input size="2" value="' + self.weight.to_s + '" name="question[' + self.id.to_s + '][weight]" id="question_' + self.id.to_s + '_weight" type="text">''</td>' | |||
html += '<td> max_label <input size="10" value="' + self.max_label.to_s + '" name="question[' + self.id.to_s + '][max_label]" id="question_' + self.id.to_s + '_max_label" type="text"> min_label <input size="12" value="' + self.min_label.to_s + '" name="question[' + self.id.to_s + '][min_label]" id="question_' + self.id.to_s + '_min_label" type="text"></td>' | |||
html += '</tr>' | |||
html.html_safe | |||
end | |||
Changed | |||
def edit(count) | |||
html = edit_remove_button(count) + edit_seq(count) | |||
html += edit_question_type(count) + edit_weight(count) | |||
html += edit_max_label(count) + edit_min_label(count) | |||
safe_join([raw("<tr>"), raw("</tr>")], raw(html)) | |||
end | |||
===checkbox.rb=== | |||
1. Short the long line | |||
2. Modify some method that is too long | |||
Previous | |||
def edit(_count) | |||
html = '<tr>' | |||
html += '<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a></td>' | |||
html += '<td><input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text"></td>' | |||
html += '<td><textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea></td>' | |||
html += '<td><input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">''</td>' | |||
html += '<td><!--placeholder (UnscoredQuestion does not need weight)--></td>' | |||
html += '</tr>' | |||
html.html_safe | |||
end | |||
Changed | |||
def edit(count) | |||
html = edit_remove_button(count) + edit_seq(count) + edit_question(count) | |||
html += edit_type(count) + edit_weight(count) | |||
safe_join(["<tr>".html_safe, "</tr>".html_safe], html.html_safe) | |||
end | |||
3. Use safe_join to be more security | |||
Previous | |||
html.html_safe | |||
Changed | |||
safe_join(["".html_safe, "".html_safe], html.html_safe | |||
===criterion.rb=== | |||
1. Improve security by using safe_join | |||
Previous | |||
html.html_safe | |||
Changed | |||
safe_join(["".html_safe, "".html_safe], html.html_safe) | |||
2. Modify some method that is too long | |||
3. Use new style validations | |||
Previous | |||
validates_presence_of :size | |||
Changed | |||
validates :size, presence: true | |||
4. Use each loop instead of for loop | |||
Previous | |||
for j in questionnaire_min..questionnaire_max | |||
Changed | |||
(questionnaire_min..questionnaire_max).each do |j| | |||
===dropdown.rb=== | |||
1. Use new style validations | |||
Previous | |||
validates_presence_of :alternatives | |||
Changed | |||
validates :alternatives, presence: true | |||
2. Modify some method that is too long | |||
3. Improve security by using safe_join | |||
Previous | |||
html.html_safe | |||
Changed | |||
safe_join(["".html_safe, "".html_safe], html.html_safe) | |||
===text_area.rb=== | |||
1. Replace condition statements in one line | |||
Previous | |||
if self.size.nil? | |||
cols = '70' | |||
ows = '1' | |||
elsif | |||
cols = self.size.split(',')[0] | |||
rows = self.size.split(',')[1] | |||
end | |||
Changed | |||
def complete_get_cols_rows | |||
return '70', '1' if self.size.nil? | |||
[self.size.split(',')[0], self.size.split(',')[1]] | |||
end | |||
2. Improve security by using safe_join | |||
Previous | |||
html.html_safe | |||
Changed | |||
safe_join(["".html_safe, "".html_safe], html.html_safe) | |||
==Test from UI== | |||
* Login in as an instructor or admin using credentials (admin with password: admin or instructor6 with password: password) | |||
*Click the Manage in the top bar,then click the Questionnaires which is down left of the the text:Manage content | |||
*Add the Name and click create | |||
*Click the any button you want |
Latest revision as of 01:24, 5 November 2016
Background
Expertiza is an open source software to create reusable learning objects through peer review. It also supports team projects, and the submission of almost any document type, including URLs and wiki pages. It has following features:
- Enables students working together in a class and a topic
- Enables studnets to do peer review and improve both him/her and others' learn experience
Reason for Refractor
The purpose of this work is to rewrite part of the codes in the Criterion.rb, Scale.rb, Checkbox.rb, Dropdown.rb, Text_response.rb, Text_area.rb, Text_field.rb. This is because some of codes have bad writing sytle, or not followingg OO language rules, which confuse others to understand these code. The detail of the requirements are list below:
- Line is too long
- Some code found in other location and should put them in the parent class
- Method has too many lines
- Some of the code will cause security risk
- Using the old style validations
- Using a lower efficent way to do the loop
All of these requirements are based on a analysis software called code climate
Result
Code Changes
text_field.rb
Although it increase the number of classes, the length of function decrease and it's easy to read than before
1. Short the long line
2. Improve security by using safe_join
Previous
html.html_safe
Changed
safe_join([" ".html_safe, " ".html_safe], html.html_safe)
3. Short long method
Previous
html += '<label for="responses_' + count.to_s + '">' + self.txt + '</label>' html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden" value="">' html += '<input id="responses_' + count.to_s + '_comments" label=' + self.txt + ' name="responses[' + count.to_s + '][comment]" size=' + self.size.to_s + ' type="text"' html += 'value="' + answer.comments unless answer.nil? html += '">'
html += '
' if self.type == 'TextField' and self.break_before == false
def complete(count, answer = nil)
html = if self.type == 'TextField' and self.break_before == true
html += combine(count, answer)
end
Changed
def combline(count, answer = nil)
html = complete_helper_response_scores(count) + complete_helper_response_comments(count)
html += complete_helper_text(answer) + complete_helper_response_comments
html += complete_helper_final_judge
html
end
text_response.rb
It makes codes more readable. We also choose safe_join instead of html_safe to prevent security risks. However, we must use html_safe to keep the character encoding.
1. Use new style validations Previous
validates_presence_of :size
Changed
validates :size, presence: true
2. Short the long line
3. Improve security by safe_join
Preivious
html.html_safe
Changed
safe_join(["".html_safe, "".html_safe], html.html_safe) 4. Short long method Preivious def edit(_count) html = '' html += '<a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a>' html += '<input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text">' html += '<textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea>' html += '<input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">' html += '' html += 'text area size <input size="6" value="' + self.size.to_s + '" name="question[' + self.id.to_s + '][size]" id="question_' + self.id.to_s + '_size" type="text">' html += '' html.html_safe end Changed def edit(_count) html = edit_link + edit_question_lable html += edit_question_textarea + edit_question_textarea_size safe_join(["".html_safe, "".html_safe], html.html_safe) end
scale.rb
1. Use each loop instead of for loop
Previous
for j in questionnaire_min..questionnaire_max
html += '<input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '>' end Changed (questionnaire_min..questionnaire_max).each do |j| html += '<input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '>' end 2. Short the long line 3. Use safe_join to be more security Previous html.html_safe Changed safe_join(["".html_safe, "".html_safe], html.html_safe 4. Delete the code similar with other file and put the duplicate into parent class. 5. Modify some method that is too long Previous def edit(_count) html = '' html += '<a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a>' html += '<input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text">' html += '<textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea>' html += '<input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">' html += '<input size="2" value="' + self.weight.to_s + '" name="question[' + self.id.to_s + '][weight]" id="question_' + self.id.to_s + '_weight" type="text">' html += ' max_label <input size="10" value="' + self.max_label.to_s + '" name="question[' + self.id.to_s + '][max_label]" id="question_' + self.id.to_s + '_max_label" type="text"> min_label <input size="12" value="' + self.min_label.to_s + '" name="question[' + self.id.to_s + '][min_label]" id="question_' + self.id.to_s + '_min_label" type="text">' html += '' html.html_safe end Changed def edit(count) html = edit_remove_button(count) + edit_seq(count) html += edit_question_type(count) + edit_weight(count) html += edit_max_label(count) + edit_min_label(count) safe_join([raw(""), raw("")], raw(html)) end
checkbox.rb
1. Short the long line
2. Modify some method that is too long Previous
def edit(_count)
html = '' html += '<a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a>' html += '<input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]" id="question_' + self.id.to_s + '_seq" type="text">' html += '<textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]" id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea>' html += '<input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]" id="question_' + self.id.to_s + '_type" type="text">' html += '' html += '' html.html_safe end Changed def edit(count) html = edit_remove_button(count) + edit_seq(count) + edit_question(count) html += edit_type(count) + edit_weight(count) safe_join(["".html_safe, "".html_safe], html.html_safe) end 3. Use safe_join to be more security Previous html.html_safe Changed safe_join(["".html_safe, "".html_safe], html.html_safe
criterion.rb
1. Improve security by using safe_join
Previous
html.html_safe
Changed
safe_join(["".html_safe, "".html_safe], html.html_safe)
2. Modify some method that is too long
3. Use new style validations
Previous
validates_presence_of :size
Changed
validates :size, presence: true
4. Use each loop instead of for loop
Previous
for j in questionnaire_min..questionnaire_max
Changed
(questionnaire_min..questionnaire_max).each do |j|
dropdown.rb
1. Use new style validations
Previous
validates_presence_of :alternatives
Changed
validates :alternatives, presence: true
2. Modify some method that is too long
3. Improve security by using safe_join
Previous
html.html_safe
Changed
safe_join(["".html_safe, "".html_safe], html.html_safe)
text_area.rb
1. Replace condition statements in one line
Previous
if self.size.nil? cols = '70' ows = '1' elsif cols = self.size.split(',')[0] rows = self.size.split(',')[1] end
Changed
def complete_get_cols_rows return '70', '1' if self.size.nil? [self.size.split(',')[0], self.size.split(',')[1]] end
2. Improve security by using safe_join
Previous
html.html_safe
Changed
safe_join(["".html_safe, "".html_safe], html.html_safe)
Test from UI
- Login in as an instructor or admin using credentials (admin with password: admin or instructor6 with password: password)
- Click the Manage in the top bar,then click the Questionnaires which is down left of the the text:Manage content
- Add the Name and click create
- Click the any button you want