CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb: Difference between revisions
No edit summary |
No edit summary |
||
Line 132: | Line 132: | ||
flash[:error] = "The topic could not be updated." | flash[:error] = "The topic could not be updated." | ||
end | end | ||
end | |||
</pre> | |||
<br> | |||
* Fixed the if-else ladder in '''signup_as_instructor_action''' action: | |||
'''Before''' | |||
<pre> | |||
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 | |||
</pre> | |||
<br> | |||
'''After''' | |||
<pre> | |||
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] and 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) | |||
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] | |||
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') | |||
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 | end | ||
</pre> | </pre> |
Revision as of 18:20, 6 November 2019
Refactor sign up sheet controller.rb
This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.
(NOTE: This wiki is not final and is still a work in progress.)
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Description of the project
The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.
About Signup Sheet Controller
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.
Issues and Solutions
- Issue 1 - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
- Solution- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
- Issue 2 - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
- Solution-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
- Issue 3 - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
- Solution-Used a single update_attributes method in place of the many instance variables.
- Issue 4 - Destroy has a misleading else flash message.
- Solution-Changed the misleading flash message in the else condition to be more straightforward.
- Issue 5 - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
- Solution-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
- Issue 6 - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
- Solution-Several method names were appropriately improved and renamed.
- Issue 7 - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
- Solution-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
- Issue 8 - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
- Solution-Provided comments for blocks of code and created helper methods.
- Issue 9 - Refactor participants variable in load_add_signup_topics
- Solution-Refactored the participants variable and improved readability.
- Issue 10 - Signup_as_instructor_action has a complicated if-else ladder.
- Solution-The if-else ladder was made more elegant and readable.
- Issue 11 - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
- Solution-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.
Code Improvements
- The create method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
Before
def create topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first if topic.nil? setup_new_topic else update_existing_topic topic end end
After
def create topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first call_create_or_update(topic) end # This method appropriately calls the create topic or update existing topic method def call_create_or_update(topic) if topic.nil? setup_new_topic else update_existing_topic topic end end
- The update method has a lot of instance variables initialized. The method has been changed in the following ways
Before
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 end
After
def update @topic = SignUpTopic.find(params[:id]) if @topic update_max_choosers @topic updated_max_choosers = @topic.max_choosers @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, 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. ") else flash[:error] = "The topic could not be updated." end end
- Fixed the if-else ladder in signup_as_instructor_action action:
Before
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
After
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] and 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) elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] 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') 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
References
- Rspec Documentation
- Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin