CSC/ECE 517 Spring 2013/OSS E605A

Introduction

This project involved refactoring the questionnaire portion of Expertiza. This part of the Expertiza project deals with the creation and management of questionnaires that are used during project review and feedback. It also allows for the creation of advice on what each score in a question should represent (a 5 being excellent, 3 being average, etc.). The majority of the refactoring took place in the file questionnaire_controller.rb, as it contains the CRUD functionality for questionnaires. Some refactoring also took place on the questionnaire.rb model, along with various different kinds of questionnaires that were represented as subclasses of questionnaire.rb.

List of Changes

Following is a list of the changes made during refactoring, along with the design principles and patterns that were upheld by making these changes.

Pulled up get_weighted_score

Each of the questionnaire subclasses had a get_weighted_score method that tallied up the points for a completed questionnaire. These methods were all identical, so we pulled the method up to the questionnaire superclass (questionnaire.rb). The following subclasses had the method and were refactored accordingly:

• author_feedback_questionnaire.rb
• metareview_questionnaire.rb
• review_questionnaire.rb
• teammate_review_questionnaire.rb
• questionnaire.rb (superclass that the method was moved to)

The main design principle that influenced this refactor was DRY. As the same functionality was used in multiple places, moving it to the superclass made the code easier to understand by showing that all of the subclasses were able to use the function in the same way.

Renamed create_questionnaire and save_questionnaire

The questionnaire_controller (questionnaire_controller.rb) contained CRUD functionality for several types of objects. Naturally, one of these objects was a questionnaire. For clarity, its methods had been named create_questionnaire and save_questionnaire. However, proper Ruby design emphasizes convention over configuration. Since the methods were in the questionnaire controller, the convention is to name them create and save, as the fact that they affect a questionnaire is implied. Therefore create_questionnaire (on line 156) was changed to create and save_questionnaire (on line 214) was changed to save.

Pulled methods from edit

The edit method in questionnaire_controller.rb did not need to be renamed, but it had another problem; the method was too long. Keeping methods short (around 25 lines or less) is another design principle that increases the readability of code. Therefore, we looked for chunks of the method that would make sense if pulled out as a separate method. Part of the edit method involved exporting and importing questionnaires. These chunks of code were well contained, with clear start and stop points, so we pulled them into separate methods. This resulted in creating an export method from lines 102-108 of the old edit method, and an import method from lines 110-125 of the old method. By doing this, the edit method was shortened to an acceptable length.

Created advice_controller

As mentioned above, the questionnaire controller contained some CRUD methods that dealt with another type of object besides questionnaires. This was the advice object, which is used to type notes regarding what score should be given on a question. Again, this violated the Ruby principle of convention over configuration, since these advice methods were in the questionnaire controller. Therefore, a new advice controller (advice_controller.rb) was created, and the edit_advice and save_advice methods were moved from the questionnaire controller to it.

Removed delete_question_type

The questionnaire_controller had a delete_question_type method that was only used when destroying the questions of a questionnaire. As it only consisted of two lines, and was only used in one place, we felt that having it in a separate method caused a minor, but unnecessary, case of the Ping-Pong effect. Since the code of the method was rather self-explanatory (simply finding the question type of the question to be deleted and destroying it), moving the functionality back into the delete_questions method and removing the delete_question_type method reduced the complexity of the code without affecting its readability or DRYness.

Changes to the Copy Method

The copy method was initially 78 lines long. After refactoring, the copy method is only 9 lines. We extracted 2 methods: getInstructorId, and cloneQuestionnaireDetails.

The getInstructorId method determines whether the person calling the copy method is a TA or an instructor. Then it returns the relevant instructor ID. This code was formerly implemented as a conditional statement within the copy method. Now it is one line of code that uses the ternary operator. The code is now easier to read, and easier to follow.

The cloneQuestionnaireDetails method loops through the questions and advice of the questionnaire that is being copied. Each question and its advice are copied over into the newly created questionnaire. This method is still a little long, but it was hard to break it down any smaller, without removing functionality that is essential to the method.

References

• Project Screencast
• Main Expertiza Repository
• Refactored Expertiza Fork