CSC/ECE 517 Fall 2023 - E2356. Refactor review mapping controller.rb: Difference between revisions
(Created page with "This wiki page is for information regarding the changes made for the E2356 Refactor review_mapping_controller.rb OSS assignment for Fall 2023, CSC/ECE 517. __TOC__ == Introd...") |
No edit summary |
||
Line 21: | Line 21: | ||
*Add meaningful comments and edit/remove unnecessary comments. | *Add meaningful comments and edit/remove unnecessary comments. | ||
*Ensure that code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files. | *Ensure that code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files. | ||
== 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. | |||
==Solutions== | ==Solutions== |
Revision as of 23:25, 30 October 2023
This wiki page is for information regarding the changes made for the E2356 Refactor review_mapping_controller.rb OSS assignment for Fall 2023, CSC/ECE 517.
Introduction
Expertiza is an open-source project developed on Ruby on Rails. This web application is maintained by the students and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well-developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.
About Controller
The sign up sheet controller contains all functions related to the management of the signup sheet for an assignment:
- Allows an instructor to add/remove topics to an assignment.
- Allows an instructor to edit properties of topics.
- Allows an instructor to assign/remove students to topics
- Allows a student to see the list of available topics which they can bid on for a given OSS assignment.
Issues
- Refactor the long methods in review_mapping_controller.rb like assign_reviewer_dynamically, add_reviewer, automatic_review_mapping,peer_review_strategy etc.
- Some variables in review_mapping_controller.rb are not named properly. Rename variable names to convey what they are used for.
- Functions are lengthy and difficult to read. Replace switch statements with subclasses methods, if any.
- Create subclasses and models wherever necessary.
- Add meaningful comments and edit/remove unnecessary comments.
- Ensure that code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.
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.
Solutions
1. The naming inconsistency issue has already been fixed by the previous version of the project. It has been verified by us.
2. Refactoring the Update method
Before refactoring
def update @topic = SignUpTopic.find(params[:id]) if @topic @topic.topic_identifier = params[:topic][:topic_identifier] update_max_choosers @topic @topic.category = params[:topic][:category] @topic.topic_name = params[:topic][:topic_name] @topic.micropayment = params[:topic][:micropayment] @topic.description = params[:topic][:description] @topic.link = params[:topic][:link] @topic.save undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ") else flash[:error] = 'The topic could not be updated.' end # Akshay - correctly changing the redirection url to topics tab in edit assignment view. redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2' end
After refactoring
def update @topic = SignUpTopic.find(params[:id]) if @topic update_max_choosers @topic @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description],link:params[:topic][:link] ) undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ") flash[:success] = 'The topic has been updated.' else flash[:error] = 'The topic could not be updated.' end # Akshay - correctly changing the redirection url to topics tab in edit assignment view. redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2' end
3.
4.
5. signup_as_instructor_action is used to sign up a student for a particular topic for an assignment as an instructor. signup_as_instructor function does nothing. It is an empty function. The function is not being called anywhere in the code. The method is not needed and hence removed.
signup_as_instructor_action
def signup_as_instructor_action user = User.find_by(name: params[:username]) if user.nil? # validate invalid user flash[:error] = 'That student does not exist!' else if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id]) flash[:success] = 'You have successfully signed up the student for the topic!' ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor signed up student for topic: ' + params[:topic_id].to_s) else flash[:error] = 'The student has already signed up for a topic!' ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor is signing up a student who already has a topic') end else flash[:error] = 'The student is not registered for the assignment!' ExpertizaLogger.info LoggerMessage.new(controller_name, , 'The student is not registered for the assignment: ' << user.id) end end redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id] end
signup_as_instructor function
def signup_as_instructor; end
6.
7.
8.
9. Delete_signup and delete_signup_as_instructor violate the DRY principle. Refactored this. Created a new private function handle_signup_deletion to handle deletion for both instructor as well as student. A new function, check_signup_eligibility, generates the error message that the topic cannot be dropped. The redirect_path function is used to redirect the user.
Before Refactoring Delete_signup and delete_signup_as_instructor
def delete_signup participant = AssignmentParticipant.find(params[:id]) assignment = participant.assignment drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6) # A student who has already submitted work should not be allowed to drop his/her topic! # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.) # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted) # If there is a drop topic deadline, student cannot drop topic after this deadline. if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty? flash[:error] = 'You have already submitted your work, so you are not allowed to drop your topic.' ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s) elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at) flash[:error] = 'You cannot drop your topic after the drop topic deadline!' ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s) else delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id) flash[:success] = 'You have successfully dropped your topic!' ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s) end redirect_to action: 'list', id: params[:id] end
def delete_signup_as_instructor # find participant using assignment using team and topic ids team = Team.find(params[:id]) assignment = Assignment.find(team.parent_id) user = TeamsUser.find_by(team_id: team.id).user participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id) drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6) if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty? flash[:error] = 'The student has already submitted their work, so you are not allowed to remove them.' ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s) elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at) flash[:error] = 'You cannot drop a student after the drop topic deadline!' ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s) else delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id) flash[:success] = 'You have successfully dropped the student from the topic!' ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s) end redirect_to controller: 'assignments', action: 'edit', id: assignment.id end
After Refactoring Delete_signup and delete_signup_as_instructor
def delete_signup handle_signup_deletion(params[:id], session[:user].id) end
def delete_signup_as_instructor team = Team.find(params[:id]) user = TeamsUser.find_by(team_id: team.id).user assignment = Assignment.find(team.parent_id) handle_signup_deletion(user.id, session[:user].id, assignment.id) end
private def handle_signup_deletion(participant_id, user_id, assignment_id = nil) participant = AssignmentParticipant.find(participant_id) assignment ||= participant.assignment drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6) error_message = check_signup_eligibility(participant.team, drop_topic_deadline, user_id, params[:topic_id]) if error_message flash[:error] = error_message ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, "Drop failed for #{error_message}: #{params[:topic_id]}") else delete_signup_for_topic(assignment.id, params[:topic_id], user_id) flash[:success] = 'You have successfully dropped the topic!' ExpertizaLogger.info LoggerMessage.new(controller_name, user_id, "Student has dropped the topic: #{params[:topic_id]}") end redirect_to redirect_path(participant_id, assignment_id) end def check_signup_eligibility(team, drop_topic_deadline, user_id, topic_id) if !team.submitted_files.empty? || !team.hyperlinks.empty? 'You have already submitted your work, so you are not allowed to drop your topic.' elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at) 'You cannot drop your topic after the drop topic deadline!' end end
def redirect_path(participant_id, assignment_id) if assignment_id controller: 'assignments', action: 'edit', id: assignment_id else action: 'list', id: participant_id end end
10.
Team
Mentor
- Ameya Vaichalikar (agvaicha)
Team Members
- Chirag Bheemaiah Palanganda Karumbaiah (cpalang)
- Shanmukh Pawan Moparthi (smopart2)
- Amit Bhujbal (abhujba)