CSC/ECE 517 Fall 2014/oss E1502 wwj: Difference between revisions
No edit summary |
|||
(42 intermediate revisions by the same user not shown) | |||
Line 1: | Line 1: | ||
==E1502: Questionnaire Controller Refactoring== | ==E1502: Questionnaire Controller Refactoring== | ||
In this project, we have done some refactoring on the questionnaires_controller. We moved some methods to where we thought suited them, and changed the format of language into ruby style, deleted debugging statement. Details are displayed in the following sections. | |||
==Introduction to Expertiza== | ==Introduction to Expertiza== | ||
The Expertiza project is system for using peer review to create reusable learning objects. Students do different assignments; then peer review selects the best work in each category, and assembles it to create a single unit.<ref>[http://expertiza.ncsu.edu/ Expertiza]</ref> | |||
==Project Description== | ==Project Description== | ||
===What it does:=== Used on the admin side of Expertiza for creating/ editing questionnaires (rubrics, surveys and quizzes). It helps in add/removing questions, options, etc for a questionnaire. | ===What it does:=== | ||
<li>Very big controller that handles a lot more than the name suggests. Functionalities need to be moved to appropriate controllers. | Used on the admin side of Expertiza for creating/ editing questionnaires (rubrics, surveys and quizzes). It helps in add/removing questions, options, etc for a questionnaire. | ||
<li>Quiz methods are should be treated the same as any other type of questionnaire; differences between quiz questionnaires and other questionnaires should be implemented in the model class, quiz_questionnaire | <li>Very big controller that handles a lot more than the name suggests. Functionalities need to be moved to appropriate controllers. | ||
<li>Turn the questionnaire into a “form object.” The ..._questions methods: save_new_questions, delete_questions, save_questions should be in a separate class. | <li>Quiz methods are should be treated the same as any other type of questionnaire; differences between quiz questionnaires and other questionnaires should be implemented in the model class, quiz_questionnaire | ||
<li>Turn the questionnaire into a “form object.” The ..._questions methods: save_new_questions, delete_questions, save_questions should be in a separate class. | |||
===Other classes involved:=== | ===Other classes involved:=== | ||
<li>questionnaire.rb | <li>questionnaire.rb | ||
<li>quiz_questionnaire.rb | <li>quiz_questionnaire.rb | ||
<li>questions_controller.rb | <li>questions_controller.rb | ||
===What needs to be done:=== | ===What needs to be done:=== | ||
<li>Move quiz related functions to quiz_questionnaire.rb. | <li>Move quiz related functions to quiz_questionnaire.rb. | ||
<li>copy, update_quiz, valid_quiz methods, clone_questionnaire_details is too long. | <li>copy, update_quiz, valid_quiz methods, clone_questionnaire_details is too long. | ||
<li>Debug output (print statements) should be removed. | <li>Debug output (print statements) should be removed. | ||
<li>Understand the functions in the controller and comment them. Ensure that the code is understandable to the next programmer who works on it.</ | <li>Understand the functions in the controller and comment them. Ensure that the code is understandable to the next programmer who works on it. | ||
==What We Have Done== | |||
===Method Refactoring=== | |||
{| class="wikitable" | |||
|- | |||
! style="width:13%;"|Method Name | |||
! style="width:33%;"|Changes Made | |||
! style="width:43%;"|Reason For Change | |||
|- style="vertical-align:top;" | |||
| copy | |||
| Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb | |||
| The content of this method is about operations on the database (coping a questionnaire), it is better to be put in the model | |||
|- | |||
| valid_quiz | |||
| Moved this method to quiz_questionnaire.rb | |||
| This method is about validation of the quiz, it shouldn't appear in the controller | |||
|- | |||
| export | |||
| Moved this method to questionnaire.rb | |||
| This method exports the questionnaires as csv file, it should't appear in the controller | |||
|- | |||
| import | |||
| Moved this method to questionnaire.rb | |||
| This method imports the questionnaires from csv file, it should't appear in the controller | |||
|- | |||
| clone_questionnaire_details | |||
| Deleted this method due to the duplication | |||
| Substituted by copy_questionnaires method in questionnaire.rb | |||
|- | |||
|} | |||
===Format Refactoring=== | |||
====Case 1: Loop Condition==== | |||
Change all the looping conditions into one format<br> | |||
<b>Before</b> | |||
<pre> | |||
# Remove a given questionnaire | |||
def delete | |||
. | |||
. | |||
. | |||
for question in @questionnaire.questions | |||
. | |||
. | |||
. | |||
end | |||
</pre> | |||
<b>After</b> | |||
<pre> | |||
# Remove a given questionnaire | |||
def delete | |||
. | |||
. | |||
. | |||
@questionnaire.assignments.each | |||
. | |||
. | |||
. | |||
end | |||
</pre> | |||
====Case 2: If Condition==== | |||
Change all the if conditions into ruby format<br> | |||
<b>Before</b> | |||
<pre> | |||
. | |||
. | |||
if @successful_create == true | |||
. | |||
. | |||
unless this_q.nil? | |||
. | |||
. | |||
</pre> | |||
<b>After</b> | |||
<pre> | |||
. | |||
. | |||
if @successful_create | |||
. | |||
. | |||
if this_q | |||
. | |||
. | |||
</pre> | |||
====Case 3: Name==== | |||
Change all the name from "JAVA name" to "Ruby name"<br> | |||
<b>Before</b> | |||
<pre> | |||
#save an updated quiz questionnaire to the database | |||
def update_quiz | |||
. | |||
. | |||
. | |||
questionnum=1 | |||
. | |||
. | |||
. | |||
end | |||
</pre> | |||
<b>After</b> | |||
<pre> | |||
#save an updated quiz questionnaire to the database | |||
def update_quiz | |||
. | |||
. | |||
. | |||
question_num=1 | |||
. | |||
. | |||
. | |||
end | |||
</pre> | |||
===Refactoring Example=== | |||
<p id="example">In models/questionnaires.rb, we add copy_questionnaires method, as shown below:</p> | |||
<pre> | |||
def self.copy_questionnaires(id,user) | |||
@orig_questionnaire = Questionnaire.find(id) | |||
questions = Question.where(questionnaire_id: id) | |||
@questionnaire = @orig_questionnaire.clone | |||
@questionnaire.instructor_id = user.instructor_id ## Why was TA-specific code removed here? See Project E713. | |||
@questionnaire.name = 'Copy of ' + @orig_questionnaire.name | |||
if user.role.name != "Teaching Assistant" | |||
@questionnaire.instructor_id = user.id | |||
else # for TA we need to get his instructor id and by default add it to his course for which he is the TA | |||
@questionnaire.instructor_id = Ta.get_my_instructor(user.id) | |||
end | |||
@questionnaire.name = 'Copy of '+@orig_questionnaire.name | |||
@questionnaire.created_at = Time.now | |||
@questionnaire.save! | |||
questions.each{ | question | | |||
new_question = question.clone | |||
new_question.questionnaire_id = @questionnaire.id | |||
new_question.save | |||
advice = QuestionAdvice.find_by_question_id(question.id) | |||
if advice | |||
new_advice = advice.clone | |||
new_advice.question_id = newquestion.id | |||
new_advice.save | |||
end | |||
if (@questionnaire.section == "Custom") | |||
old_question_type = QuestionType.find_by_question_id(question.id) | |||
if old_question_type | |||
new_question_type = old_question_type.clone | |||
new_question_type.question_id = newquestion.id | |||
new_question_type.save | |||
end | |||
end | |||
} | |||
pFolder = TreeFolder.find_by_name(@questionnaire.display_type) | |||
parent = FolderNode.find_by_node_object_id(pFolder.id) | |||
unless QuestionnaireNode.where(parent_id: parent.id, node_object_id: @questionnaire.id) | |||
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) | |||
end | |||
end | |||
</pre> | |||
==References== | |||
<references/> |
Latest revision as of 23:44, 21 March 2015
E1502: Questionnaire Controller Refactoring
In this project, we have done some refactoring on the questionnaires_controller. We moved some methods to where we thought suited them, and changed the format of language into ruby style, deleted debugging statement. Details are displayed in the following sections.
Introduction to Expertiza
The Expertiza project is system for using peer review to create reusable learning objects. Students do different assignments; then peer review selects the best work in each category, and assembles it to create a single unit.<ref>Expertiza</ref>
Project Description
What it does:
Used on the admin side of Expertiza for creating/ editing questionnaires (rubrics, surveys and quizzes). It helps in add/removing questions, options, etc for a questionnaire.
Other classes involved:
What needs to be done:
What We Have Done
Method Refactoring
Method Name | Changes Made | Reason For Change |
---|---|---|
copy | Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb | The content of this method is about operations on the database (coping a questionnaire), it is better to be put in the model |
valid_quiz | Moved this method to quiz_questionnaire.rb | This method is about validation of the quiz, it shouldn't appear in the controller |
export | Moved this method to questionnaire.rb | This method exports the questionnaires as csv file, it should't appear in the controller |
import | Moved this method to questionnaire.rb | This method imports the questionnaires from csv file, it should't appear in the controller |
clone_questionnaire_details | Deleted this method due to the duplication | Substituted by copy_questionnaires method in questionnaire.rb |
Format Refactoring
Case 1: Loop Condition
Change all the looping conditions into one format
Before
# Remove a given questionnaire def delete . . . for question in @questionnaire.questions . . . end
After
# Remove a given questionnaire def delete . . . @questionnaire.assignments.each . . . end
Case 2: If Condition
Change all the if conditions into ruby format
Before
. . if @successful_create == true . . unless this_q.nil? . .
After
. . if @successful_create . . if this_q . .
Case 3: Name
Change all the name from "JAVA name" to "Ruby name"
Before
#save an updated quiz questionnaire to the database def update_quiz . . . questionnum=1 . . . end
After
#save an updated quiz questionnaire to the database def update_quiz . . . question_num=1 . . . end
Refactoring Example
In models/questionnaires.rb, we add copy_questionnaires method, as shown below:
def self.copy_questionnaires(id,user) @orig_questionnaire = Questionnaire.find(id) questions = Question.where(questionnaire_id: id) @questionnaire = @orig_questionnaire.clone @questionnaire.instructor_id = user.instructor_id ## Why was TA-specific code removed here? See Project E713. @questionnaire.name = 'Copy of ' + @orig_questionnaire.name if user.role.name != "Teaching Assistant" @questionnaire.instructor_id = user.id else # for TA we need to get his instructor id and by default add it to his course for which he is the TA @questionnaire.instructor_id = Ta.get_my_instructor(user.id) end @questionnaire.name = 'Copy of '+@orig_questionnaire.name @questionnaire.created_at = Time.now @questionnaire.save! questions.each{ | question | new_question = question.clone new_question.questionnaire_id = @questionnaire.id new_question.save advice = QuestionAdvice.find_by_question_id(question.id) if advice new_advice = advice.clone new_advice.question_id = newquestion.id new_advice.save end if (@questionnaire.section == "Custom") old_question_type = QuestionType.find_by_question_id(question.id) if old_question_type new_question_type = old_question_type.clone new_question_type.question_id = newquestion.id new_question_type.save end end } pFolder = TreeFolder.find_by_name(@questionnaire.display_type) parent = FolderNode.find_by_node_object_id(pFolder.id) unless QuestionnaireNode.where(parent_id: parent.id, node_object_id: @questionnaire.id) QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) end end
References
<references/>