CSC/ECE 517 Fall 2015/oss E1563 ASA
Introduction
Questionnaire Controller is responsible for creating, editing and reviewing quizes. The Questionnaire controller is also used for peer-review, teammate-review and creating and managing quizes. The basic functionality of adding, removing, editing questions and or options is handled by this controller. This wikipedia page explains the Open Source Project of refactoring the questionnaire controller on expertiza.
The need of refactoring
- Maintainability. It is easier to fix bugs because the source code is easy to read and the intent of its author is easy to grasp.<ref name=martin></ref> This might be achieved by reducing large monolithic routines into a set of individually concise, well-named, single-purpose methods. It might be achieved by moving a method to a more appropriate class, or by removing misleading comments.
- Extensibility. It is easier to extend the capabilities of the application if it uses recognizable design patterns, and it provides some flexibility where none before may have existed.<ref name=kerievsky/>
Project Overview
The scope of our project is to refactor the questionnaires controller. The questionnaires controller at this moment is very big and hence, difficult to understand. The aim of the project was to distribute the functionality in different controllers, so that the questionnaires controller will be slim and easy to understand.
Classes involved
- questionnaire.rb
- quiz_questionnaire.rb
- questions_controller.rb
Changes made
- copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
- Changed code to follow the global rules.
- A lot of code duplication has been removed.
- Test cases are written using rspec for the functions in questionnaire_controller.rb.
- Copy method was broken(error page was shown), fixed it partially.
- Controller was very big than what the name describes. Appropriate functionality has been moved to different controller to make this controller slim.
| S.No. | Place changes implemented | Change Made | Reason |
|---|---|---|---|
| 1 | Complete controller | Replaced 'and' with '&&' and 'or' with 'II' in all places | Refactored as per global rule 1 to keep Boolean precedence |
| 2 | Complete controller | Replaced if (var == true) with if var | Refactored as per global rule 4 as default check for 'if' is for true |
| 3 | Complete controller | Replaced :key => ‘value’ with key: ‘value’ for 88 instance in controller | Refactored as per global rules |
| 4 | assign_instructor_id | Removed duplicate code for assigning instructor id from copy and clone_questionnaire_details methods | Removed duplication suggested by code climate. |
| 5 | check_create_new_node | Removed duplicate code for checking creation of new node from copy_questionnaire,save and clone_questionnaire_details methods | Removed duplication suggested by code climate. |
| 6 | update_quiz | Refactored this method and extracted choose_question_type out of this method | Refactoring was needed for this huge method |
| 7 | valid_quiz | Refactored this method and extracted valid_quiz_option out of this method to check validity of options | Refactoring was needed as method was performing 2 actions |
| 8 | save_choices | Refactored this method and created 3 new methods namely save_truefalse_choice,save_single_choice and save_multiple_choice | Refactoring was needed for this huge method |
| 9 | view_quiz | Moved this method to Questionnaires_quiz_controller and updated routes.rb and views accordingly | Quiz related methods was needed to move out of this Questionnaire controller |
| 10 | new_quiz | Moved this method to Questionnaires_quiz_controller | Questionnaire method should not contain quiz related methods, thus moved it out to a new controller |
| 11 | update_quiz | Moved this method to Questionnaires_quiz_controller and updated routes.rb and views accordingly | Quiz related methods was needed to move out of this Questionnaire controller |
| 12 | edit_quiz | Moved this method to Questionnaires_quiz_controller and updated routes.rb and views accordingly | Questionnaire method should not contain quiz related methods, thus moved it out to a new controller |
| 13 | create_quiz_questionnaire | Moved this method to Questionnaires_quiz_controller and updated routes.rb and views accordingly | Quiz related methods was needed to move out of this Questionnaire controller |
| 14 | valid_quiz | Moved this method to Questionnaires_quiz_controller and updated routes.rb and views accordingly | Quiz related methods was needed to move out of this Questionnaire controller |
Project Resources
Code Snippets
1. This code was duplicated in Copy and clone_questionnaire_details method. So new method with name assign_instructor_id is created and that duplication is removed.
2. Copy method was very big,so created new method copy_questionnaire method out of it, which helps while debugging.
3. This code was also duplicated in save,copy and clone_questionnaire_details method. New method with name check_create_new_node is created to remove this duplication.
4. Save_choices method was very big too,thus created new methods namely,save_truefalse_choice,save_single_choice and save_multiple_choice for different choice types.
5. Update quiz method was also huge, so created save_question_type out of it and called it in quiz_method.
6. Valid quiz method was checking quiz validity and options validity too. So extracted valid_quiz_options to check validity of quiz options.
7. Refactored whole controller as per global rules.
Future Consideration
- Turn the questionnaire into a “form object.” The ..._questions methods: save_new_questions, delete_questions, save_questions should be in a separate class.
- There was a valid_quiz in this controller which is moved to questionnaires_quiz_controller now, but it only check if the quiz is valid when new quiz is created, but not when the existing quiz is updated. The reason is that, the format for params is not the same. Please fix this issue and make the valid _quiz work both for quiz creation and quiz_update.
- Write tests for quiz creation, for both scenario which the input quiz questions are valid and not valid.
- Test (and fix if they are broken) the export and import functionality. Write test for importing and exporting questionnaires (Issue 577).
Object Oriented Design Principles Followed
The following object oriented design principles were followed during refactoring:
- Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
- DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.
Running Project & Testing Functionality
- Test Cases have been written for this functionality under spec/models with testcase name questionnaire_spec.rb
- This testing has been done using RSpec, a behavior-driven development framework.
Test from UI
- Navigate to [1] login with username: instructor6 and password: password.
- Go to Manage>Questionnaire.
- Click on Questionnaire link and create a new public item in review section.
- Expand Review portion and click on edit action for questionnaire create above.
- Here you can add different questions and its weight.
- Go back to questionnaire list and try copy,edit,delete Questionnaire.
Conclusion
- Refactoring was performed as per the requirements in the files questionnaire_controller.rb
- Code duplication has been removed efficiently by extracting them into separate methods.
- Test cases were written to some of the functionality in questionnaire_controller.rb.
References
Expertiza
Code Refactoring Wiki
Global Rules for Refactoring
Why Refactor
Single Responsibility Principle