CSC/ECE 517 Spring 2022 - E2224: Refactor review mapping controller

This wiki page is for the description of changes made under E2224 OSS assignment for Spring 2022, CSC/ECE 517.

Expertiza Background

Expertiza is an open source project which is developed and maintained by NCSU faculty and students. It is based on Ruby on Rails framework and has many features supporting instructor-student activities. Instructors can create assignments and review student's work. Students can form groups, submit assignments and review peer's work.

Description of the project

The project aims to refactor a controller - review_mapping_controller. It is a long and a complex file and most of the methods do not include comments. The goal is to better re-structure it without modifying its original functionality and improving readability by using meaningful variable names and adding more comments.

Link to the Pull Request Submitted
Link to the deployed project
Link to the Repository

Functionality of review_mapping_controller

The functionality of review_mapping_controller is to provide mapping for reviewer and assignment. Basically, the controller handles assignment of reviews to different teams or single student user, such as the event of peer review and self review. Also, this controller is responsible to respond student user request for extra bonus reviews based on assignment policy.

Problem Statement

The review_mapping_controller is a long and complex file. Most of the methods are sparsely commented on. Some methods are way too long to understand, please break them down into pieces for better understanding. Also, the few instances of code duplication that exist should also be removed.

Tasks

-Refactor the long methods in review_mapping_controller.rb
-Rename variable names such as student_review_num, submission_review_num, calibrated_artifacts_num, participants_hash to convey what they are actually used for
-Replace switch statements with subclasses methods
-Create models for the subclasses
-Remove hardcoded parameters

Design Pattern

We followed several design patterns while refactoring our code. One of the most common ones is the “Extract Method.” There were several instances where the method was too long and complex. We moved some of the functionality to a different method to make it more readable. This way, it became easier and clearer to understand what the method achieved. Another issue we observed with the code was that it had too many conditional statements. We used the “Refactoring conditionals” design pattern, which aims to place the block of code in the conditional statement in a different method and call that method. This improves the readability of our code.

Details of the changes made

review_mapping_controller.rb

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. The following methods which were originally long and complex were broken down into simpler methods. We have added comments for every new method that was added and updated existing comments where ever required.

(i) peer_review_strategy:

The above method assigns reviews evenly among the participants for each team. It doesn't allow the participant to review his/her own work. Initially, this method was almost 80 lines long containing multiply nested if statements and it was a conglomeration of various functionalities required to achieve the strategy. It was broken down meaningfully into 4 different functions to handle different functionalities. The DRY principle is followed.
peer_review_strategy() is divided into even_out_reviews_among_teams(), modify_selected_participants_to_review(), remove_students_with_enough_reviews(), get_random_participant_index() as the core functionality blocks.

even_out_reviews_among_teams - This method is used to even out the # of reviews among teams for the specified assignment ID

modify_selected_participants_to_review - This method is used to update the selected participants corresponding to a team to review

remove_students_with_enough_reviews - This method is used to remove students who have already been assigned enough num of reviews out of participants array

get_random_participant_index - This method is used to get random index to select a random participant for assigning a review

(ii) assign_reviewer_dynamically:

The above method is used to assign submissions of students for peer reviews. It also makes sure that a reviewer can do only a certain number of reviews based on the assignment policy. We broke this method down by adding the assign_on_topic_availability method, which makes assignments based on the availability of the topic. It makes an assignment for both the cases where the topic is available and the topic is not available.

(iii) automatic_review_mapping:

The above method was also long and contained multiples tasks that can be separated into different methods. We have introduced the following methods:

create_team - creates teams for individual assignments

mapping_strategy_without_artifacts- checks for pre-conditions when there are no artifacts and applies review strategy method

mapping_strategy_on_artifacts- applies review mapping strategy based on calibrated and un-calibrated artifacts

(iv) add_reviewer :

The above method adds a reviewer to a student's work. Initially, the method checks whether the student is being assigned to review the work of his/her teammates and displays an error. If not, the student is assigned to review the work of students from a different team. The logic which handles the assignment of work from another team is moved to a separate method named as 'add_reviewer_to_another_team'.

(v) add_instructor_to_do_standard_peer_review :

The above method name was changed to add_instructor_to_do_standard_peer_review from add_calibration. The method add_instructor_to_do_standard_peer_review gets called when the instructor of this assignment wants to do the expert peer-review. It checks if a participant is an instructor of this assignment in corresponding views and in this function it checks with certain user_id exists in the AssignmentParticipant map. If it's not able to find it, it'll create one.

We found that the functionality of both the delete_metareview method and delete_metareviewer method were of the same and hence we have removed the delete_metareview method to avoid code duplication.

We have modified variable names throughout all the methods to make them more readable.

review_mapping_controller_spec.rb

Updated following params variable names for better readability:
max_team_size num_reviews_per_student
num_reviews_per_submission
num_calibrated_artifacts
num_uncalibrated_artifacts

Test Plan

Manual UI Testing

According to the introduction, the modification of this project is about review_mapping_controller.rb. The main job of this controller is to correctly assign the review to each student who is requesting them for a certain assignment. Unfortunately, the deployed project database is not complete, and student accounts cannot be created. Therefore, we cannot use the deployed project to give a complete UI test plan.

In fact, due to system reasons, the internal logic of these controllers cannot be tested using UI tests because they are work flow and logic that have nothing to do with the UI. (The work flow can be found here [1]) So we will re-introduce and emphasis the tests in Rspec unit test section to explain why the project is still complete after modification.

Rspec Unit Tests

As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run: rspec spec/controllers/review_mapping_controller_spec.rb

Refactoring changes did not break any functionality. All the existing test cases passed successfully. Adding new testcases is not in the scope of this project since its a refactoring project.

Capybara Integration and Functional Tests

Our project passed the Travis CI build test.

Project Mentor

Naman Shrimali (nshrima@ncsu.edu)

Team Members

Harini Bharata (hbharat@ncsu.edu)

Sravanth Reddy Bommana (sbomman@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)