CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller: Difference between revisions
(37 intermediate revisions by 3 users not shown) | |||
Line 24: | Line 24: | ||
*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb | *Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb | ||
* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle | * The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle. | ||
==Prior Work section with last years and last projects background== | ==Prior Work section with last years and last projects background== | ||
Line 55: | Line 55: | ||
== About Controller == | == About Controller == | ||
The sign up sheet controller performs the following functions: | The sign up sheet controller performs the following functions: | ||
* Allows an instructor to | * Allows an instructor to create/delete new topics for an assignment. | ||
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead) | * Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead) | ||
* Allows a student to see the list of available topics which can be bid on for given OSS assignment. | * Allows a student to see the list of available topics which can be bid on for given OSS assignment. | ||
== General Design Goals == | == General Design Goals == | ||
* Remove violations of Single Responsibility principle by refactoring the design of the controller. | |||
* Magic Tricks of Testing | * Magic Tricks of Testing | ||
Line 72: | Line 74: | ||
* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable | * Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable | ||
==Controller Re-design | ==Plan of Work== | ||
* '''Controller Re-design''' | |||
The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram - | |||
[[File:SUPCLASS.PNG|300px|Image: 300 pixels]] | [[File:SUPCLASS.PNG|300px|Image: 300 pixels]] | ||
Line 80: | Line 84: | ||
[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]] | [[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]] | ||
The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored. | |||
* '''@Participants variable''' | * '''@Participants variable''' | ||
Line 101: | Line 104: | ||
... | ... | ||
* '''Increase Test Coverage''' | * '''Increase Test Coverage''' | ||
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by | We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller. | ||
Line 166: | Line 167: | ||
} | } | ||
* ''' Code Climate Issues ''' | * ''' Code Climate Issues ''' | ||
Line 175: | Line 175: | ||
:* Class has too many lines. [393/100] | :* Class has too many lines. [393/100] | ||
We plan to move some methods that are not directly involves in CRUD to the helper | We plan to move some methods that are not directly involves in CRUD to the helper modules: | ||
update_waitlist | update_waitlist | ||
compute_signed_up_topics | compute_signed_up_topics | ||
Line 216: | Line 216: | ||
# The list must be visible, which indicates functionality is as before after refactoring list function. | # The list must be visible, which indicates functionality is as before after refactoring list function. | ||
== Code Modifications== | ; Verify setting up staggered deadlines of signup topics in assignments | ||
# Login as instructor | |||
# Hover on "Manage" Tab | |||
# Click on "Assignments" Tab | |||
# Click on Edit Button for "Wikipedia contribution" assignment | |||
# Select "Staggered deadline assignnment?" checkbox | |||
# Click "OK" at the popping up dialog | |||
# Click "Save" button | |||
# Click on "Topics" Tab | |||
# Scroll down to the bottom and click "Show start/due date" | |||
# Find out 2 topics, which are selected by at least 1 student, respectively | |||
# Setup different deadlines for them and make sure one topic is in submission stage and the other one is in review stage | |||
# Click "Save" button | |||
# The updates should be saved without any error | |||
# Sign in as one of the students for each topic, you should see different stages of the assignment "Wikipedia contribution" in the assignment list | |||
== Code Modifications ( Work Completed) == | |||
* ''' Renamed the attribute is_intelligent in the Assignment table''' | |||
The attribute 'is_intelligent' is an incredibly bad name as it poorly describes its function - to indicate whether it allows bidding for its topics. This attribute has been renamed to 'bid_for_topics' which is far more intuitive. A new migration was added for the same and all 66 occurrences across the project have been updated. The signup functionality has been tested both manually and through rspecs post this change to ensure that nothing was broken. | |||
class FixColumnNameAssignments < ActiveRecord::Migration[5.1] | |||
def change | |||
rename_column :assignments, :is_intelligent, :bid_for_topics | |||
end | |||
end | |||
https://github.com/weichic-ncsu/expertiza/commit/69e7fc95cc36c312d0c05399ab78eddfb4aee723 | |||
--------------------------------------------------- | |||
* '''Participants variable''' | |||
References to participants (plural) have been replaced with @team_members for clarity, consistency as team_members was used in a similar fashion elsewhere in the controller and to avoid clashing with @participant (singular), which is used in many areas. | |||
Example Old: | |||
<% @participants = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %> | |||
Example New: | |||
<% @team_members = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %> | |||
Commit : https://github.com/weichic-ncsu/expertiza/commit/7b8a230f8d0ddb3953ec3a9db8506caa2bb691e4 | |||
--------------------------------------------------- | |||
* '''Staggered Deadlines of Topics in Assignment''' | |||
:* Added back views rendered for staggered deadlines in the signup sheet | |||
:* Fixed controller error of this feature | |||
:* Tested this feature with demo video | |||
Example Old: | |||
<%= form_tag controller: 'assignment', action: 'edit', assignment_id: @assignment.id do %> | |||
<% end %> | |||
Example New: | |||
<%= form_tag controller: 'assignments', action: 'edit', assignment_id: @assignment.id do %> | |||
<% end %> | |||
Commits: | |||
# https://github.com/weichic-ncsu/expertiza/commit/6dd51b6629ac020d6dd65ef9994a638041106f87 | |||
# https://github.com/weichic-ncsu/expertiza/commit/2853eaf37f36f6e1291a14faaeca715bcbfe15e8 | |||
# https://github.com/weichic-ncsu/expertiza/commit/e38c3cba2625c613b836f056273492c53f9bbad8 | |||
--------------------------------------------------- | |||
* '''Add_signup_topics''' | |||
Previously we encountered an error page when attempting to create a topic with the same name as an existing topic. The functionality was set up to update the attributes of the existing topic and then redirect the user to add_signup_topics. However this route was broken ( from an error in a different controller class) and the functionality was very unclear from reading the code. | |||
We have changed this so that when the user attempts to create a new topic with the same name as an existing topic, the topic attributes are updated as before, but the user is instead redirected back to the topics tab of the assignment edit page. | |||
Old ( in def update_existing_topic ) | |||
topic.update_attributes(topic_params) | |||
redirect_to_sign_up(params[:id]) | |||
New: | |||
topic.update_attributes(topic_params) | |||
redirect_to edit_assignment_path(params[:id], anchor: 'tabs-2') | |||
With this change we were also able to remove the below method: | |||
def redirect_to_sign_up(assignment_id) | |||
assignment = Assignment.find(assignment_id) | |||
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id) : | |||
(redirect_to action: 'add_signup_topics', id: assignment_id) | |||
end | |||
We also updated the corresponding rspec tests to reflect the change: | |||
old: | |||
expect(response).to redirect_to('/signup_sheet/add_signup_topics?id=1') | |||
new: | |||
expect(response).to redirect_to('/assignments/1/edit#tabs-2') | |||
commits: | |||
https://github.com/weichic-ncsu/expertiza/commit/5dec776b7355290efde7e2249de83d89fd0dd259 | |||
https://github.com/weichic-ncsu/expertiza/commit/8ab175b02cf825f5f414e04e33f1790da0f1bd93 | |||
https://github.com/weichic-ncsu/expertiza/commit/1dcd1b8107657fbeaaea60a586e6705dc0cf3404 | |||
--------------------------------------------------- | |||
* '''Removed methods performing redundant/unnecessary functions''' | |||
There were a few methods in the controller performing redundant functions, like assigning every individual attribute of an | |||
Active Record object separately before calling save. This is unnecessary as it is taken care of by the new method for Active Record, | |||
by passing in the params expected. | |||
def setup_new_topic | |||
set_values_for_new_topic | |||
@signup_topic = SignUpTopic.new(topic_params) | |||
@signup_topic.assignment_id = params[:id] | |||
@assignment = Assignment.find(params[:id]) | |||
. | |||
. | |||
end | |||
For instance, this above method is calling the function "set_values_for_new_topic" | |||
def set_values_for_new_topic | |||
@signup_topic = SignUpTopic.new | |||
@signup_topic.topic_identifier = topic_params[:topic_identifier] | |||
@signup_topic.topic_name = topic_params[:topic_name] | |||
@signup_topic.max_choosers = topic_params[:max_choosers] | |||
@signup_topic.category = topic_params[:category] | |||
@signup_topic.assignment_id = params[:id] | |||
@assignment = Assignment.find(params[:id]) | |||
end | |||
This function is explicitly setting every attribute from topic_params which is redoing work that is already implemented within | |||
Active Record new function. This method has been removed entirely as it violates the DRY principle. | |||
def setup_new_topic | |||
@signup_topic = SignUpTopic.new(topic_params) | |||
@signup_topic.assignment_id = params[:id] | |||
@assignment = Assignment.find(params[:id]) | |||
end | |||
Further, a few methods that were being called only once and did very little - like the below methods to redirect and to initialize a single variable have been removed and the code has been directly placed in the caller. | |||
def redirect_to_assignment_edit(assignment_id) | |||
redirect_to controller: 'assignments', action: 'edit', id: assignment_id | |||
end | |||
def load_all_selected_topics | |||
@selected_topics = SignUpTopic.where(assignment_id: params[:assignment_id], topic_identifier: params[:topic_ids]) | |||
end | |||
def find_topic_deadlines | |||
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7) | |||
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6) | |||
end | |||
--------------------------------------------------- | |||
* '''Methods moved to the Signup Sheet Helper module''' | |||
Some methods that are not directly related to CRUD have been moved to the Signup Sheet Helper module to improve cohesion and clarity. | |||
def compute_signed_up_topics | |||
signed_up_topics = [] | |||
. | |||
. | |||
return signed_up_topics | |||
end | |||
# Update teams on the waitlist for the topic based on update of max_choosers | |||
def update_waitlist(topic) | |||
# if max choosers has increased, remove teams from the waitlist accordingly | |||
. | |||
. | |||
end | |||
==Relevant Links== | ==Relevant Links== |
Latest revision as of 14:56, 6 December 2022
Team
Mentor
- Ed Gehringer (efg)
Team Members
- Juan Benavides (jdbenavi)
- Wei-Chi Chen (wchen37)
- Swetha Sairamakrishnan (ssairam)
Problem Statement
- Provide extensive comments for all the code you view/refactor/modify.
- Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.
- Address and test effects of add_signup_topics_staggered
- Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
- Currently in RSpec, it is only implicitly tested in the method create.
- Refactor participants variable in load_add_signup_topics
- [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]
- There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
- Rename the attribute.
- Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb
- The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.
Prior Work section with last years and last projects background
- We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:
- Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.
- Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.
- Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().
- Removed load_add_signup_topics() since it does nothing for add_signup_topics().
- Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.
- List method was too long and has been refactored.
- We have also added two new test cases for the controller:
- Destroy signup_sheet when there are other topics in the assignment
- General test case added for rendering list view
- DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
- The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
- Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.
About Controller
The sign up sheet controller performs the following functions:
- Allows an instructor to create/delete new topics for an assignment.
- Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
- Allows a student to see the list of available topics which can be bid on for given OSS assignment.
General Design Goals
- Remove violations of Single Responsibility principle by refactoring the design of the controller.
- Magic Tricks of Testing
- Test incoming command messages by making assertions about direct public side effects.
- Increase Cohesion and Minimize Coupling
- DRY out the code wherever possible
- Improve code clarity by naming methods and variables appropriately
- Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable
Plan of Work
- Controller Re-design
The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -
The new design follows Single Responsibility, and the methods were split up on the basis of Information Expert pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.
- @Participants variable
References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.
There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.
def delete_signup participant = AssignmentParticipant.find(params[:id]) ... ... def list @participant = AssignmentParticipant.find(params[:id].to_i) @assignment = @participant.assignment ...
- Increase Test Coverage
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.
- Address and test effects of add_signup_topics_staggered
- Work done
- RSpec
- When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.
context 'when topic can be found' do context 'when assignment.staggered_deadline is True' do it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do ... end end context 'when assignment.staggered_deadline is False' do it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do ... end end end
- Plan
- RSpec
- Test add_signup_topics explicitly with different assignments as the input argument.
- Mandatory
- Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.
- Assignment is_intelligent attribute is a source of confusion
There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose. If this attribute were to be renamed, it would involve a lot of changes across the project -
- The schema for Assignment would have to be updated
t.boolean "is_intelligent"
- All creation of Assignment objects across the code and specs would have to be changed
context 'when current assignment is not intelligent assignment' do it 'renders signup_sheet#list page' do assignment.is_intelligent = false allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)]) request_params = { id: 1 } get :list, params: request_params expect(response).to render_template(:list) end end
- Existing test or sample DB data for Assignments would also have to be updated accordingly.
- Views including tree_display would also be affected as they contain checks like this -
(props) => props.is_intelligent ? { title: 'Intelligent Assignment', href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`, src: '/assets/tree_view/run-lottery.png' }
- Code Climate Issues
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#
Some issues observed on Code Climate for this controller -
- Class SignUpSheetController has 32 methods (exceeds 20 allowed).
- Class has too many lines. [393/100]
We plan to move some methods that are not directly involves in CRUD to the helper modules: update_waitlist compute_signed_up_topics add_signup_topics, set_values_for_new_topic, set_priority, switch_original_topic_to_approved_suggested_topic
Test Plan
Edge Cases and Pre-Conditions
- When dropping topic if submission already done.
- When deadline from dropping topic has passed.
- Deleting topic when topic cannot be found.
- Signup in case when user cannot be found.
- Create signup_sheet when sign_up_topic cannot be found.
- Destroy signup_sheet when other topics with the same assignment exists.
RSpec
- 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/signup_sheet_controller_spec.rb
- We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
- rspec spec/helpers/signup_sheet_helper_spec.rb
- rspec spec/models/signup_sheet_spec.rb
Mandatory
- Login as instructor
- Hover on Manage Tab
- Click on Assignments Tab
- Click on Edit Button for "OSS project & documentation" assignment
- Select the Topic Tab
- Add New Topic or Delete topic using links in the form below
- Login as student
- Click on OSS project/Writing assignment 2
- Click on Signup sheet
- The list must be visible, which indicates functionality is as before after refactoring list function.
- Verify setting up staggered deadlines of signup topics in assignments
- Login as instructor
- Hover on "Manage" Tab
- Click on "Assignments" Tab
- Click on Edit Button for "Wikipedia contribution" assignment
- Select "Staggered deadline assignnment?" checkbox
- Click "OK" at the popping up dialog
- Click "Save" button
- Click on "Topics" Tab
- Scroll down to the bottom and click "Show start/due date"
- Find out 2 topics, which are selected by at least 1 student, respectively
- Setup different deadlines for them and make sure one topic is in submission stage and the other one is in review stage
- Click "Save" button
- The updates should be saved without any error
- Sign in as one of the students for each topic, you should see different stages of the assignment "Wikipedia contribution" in the assignment list
Code Modifications ( Work Completed)
- Renamed the attribute is_intelligent in the Assignment table
The attribute 'is_intelligent' is an incredibly bad name as it poorly describes its function - to indicate whether it allows bidding for its topics. This attribute has been renamed to 'bid_for_topics' which is far more intuitive. A new migration was added for the same and all 66 occurrences across the project have been updated. The signup functionality has been tested both manually and through rspecs post this change to ensure that nothing was broken.
class FixColumnNameAssignments < ActiveRecord::Migration[5.1] def change rename_column :assignments, :is_intelligent, :bid_for_topics end end
https://github.com/weichic-ncsu/expertiza/commit/69e7fc95cc36c312d0c05399ab78eddfb4aee723
- Participants variable
References to participants (plural) have been replaced with @team_members for clarity, consistency as team_members was used in a similar fashion elsewhere in the controller and to avoid clashing with @participant (singular), which is used in many areas.
Example Old:
<% @participants = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %>
Example New:
<% @team_members = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %>
Commit : https://github.com/weichic-ncsu/expertiza/commit/7b8a230f8d0ddb3953ec3a9db8506caa2bb691e4
- Staggered Deadlines of Topics in Assignment
- Added back views rendered for staggered deadlines in the signup sheet
- Fixed controller error of this feature
- Tested this feature with demo video
Example Old:
<%= form_tag controller: 'assignment', action: 'edit', assignment_id: @assignment.id do %> <% end %>
Example New:
<%= form_tag controller: 'assignments', action: 'edit', assignment_id: @assignment.id do %> <% end %>
Commits:
- https://github.com/weichic-ncsu/expertiza/commit/6dd51b6629ac020d6dd65ef9994a638041106f87
- https://github.com/weichic-ncsu/expertiza/commit/2853eaf37f36f6e1291a14faaeca715bcbfe15e8
- https://github.com/weichic-ncsu/expertiza/commit/e38c3cba2625c613b836f056273492c53f9bbad8
- Add_signup_topics
Previously we encountered an error page when attempting to create a topic with the same name as an existing topic. The functionality was set up to update the attributes of the existing topic and then redirect the user to add_signup_topics. However this route was broken ( from an error in a different controller class) and the functionality was very unclear from reading the code.
We have changed this so that when the user attempts to create a new topic with the same name as an existing topic, the topic attributes are updated as before, but the user is instead redirected back to the topics tab of the assignment edit page.
Old ( in def update_existing_topic )
topic.update_attributes(topic_params) redirect_to_sign_up(params[:id])
New:
topic.update_attributes(topic_params) redirect_to edit_assignment_path(params[:id], anchor: 'tabs-2')
With this change we were also able to remove the below method:
def redirect_to_sign_up(assignment_id) assignment = Assignment.find(assignment_id) assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id) : (redirect_to action: 'add_signup_topics', id: assignment_id) end
We also updated the corresponding rspec tests to reflect the change:
old:
expect(response).to redirect_to('/signup_sheet/add_signup_topics?id=1')
new:
expect(response).to redirect_to('/assignments/1/edit#tabs-2')
commits:
https://github.com/weichic-ncsu/expertiza/commit/5dec776b7355290efde7e2249de83d89fd0dd259
https://github.com/weichic-ncsu/expertiza/commit/8ab175b02cf825f5f414e04e33f1790da0f1bd93
https://github.com/weichic-ncsu/expertiza/commit/1dcd1b8107657fbeaaea60a586e6705dc0cf3404
- Removed methods performing redundant/unnecessary functions
There were a few methods in the controller performing redundant functions, like assigning every individual attribute of an Active Record object separately before calling save. This is unnecessary as it is taken care of by the new method for Active Record, by passing in the params expected.
def setup_new_topic set_values_for_new_topic @signup_topic = SignUpTopic.new(topic_params) @signup_topic.assignment_id = params[:id] @assignment = Assignment.find(params[:id]) . . end
For instance, this above method is calling the function "set_values_for_new_topic"
def set_values_for_new_topic @signup_topic = SignUpTopic.new @signup_topic.topic_identifier = topic_params[:topic_identifier] @signup_topic.topic_name = topic_params[:topic_name] @signup_topic.max_choosers = topic_params[:max_choosers] @signup_topic.category = topic_params[:category] @signup_topic.assignment_id = params[:id] @assignment = Assignment.find(params[:id]) end
This function is explicitly setting every attribute from topic_params which is redoing work that is already implemented within Active Record new function. This method has been removed entirely as it violates the DRY principle.
def setup_new_topic @signup_topic = SignUpTopic.new(topic_params) @signup_topic.assignment_id = params[:id] @assignment = Assignment.find(params[:id]) end
Further, a few methods that were being called only once and did very little - like the below methods to redirect and to initialize a single variable have been removed and the code has been directly placed in the caller.
def redirect_to_assignment_edit(assignment_id) redirect_to controller: 'assignments', action: 'edit', id: assignment_id end
def load_all_selected_topics @selected_topics = SignUpTopic.where(assignment_id: params[:assignment_id], topic_identifier: params[:topic_ids]) end
def find_topic_deadlines @signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7) @drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6) end
- Methods moved to the Signup Sheet Helper module
Some methods that are not directly related to CRUD have been moved to the Signup Sheet Helper module to improve cohesion and clarity.
def compute_signed_up_topics signed_up_topics = [] . . return signed_up_topics end
# Update teams on the waitlist for the topic based on update of max_choosers def update_waitlist(topic) # if max choosers has increased, remove teams from the waitlist accordingly . . end
Relevant Links
- https://github.com/weichic-ncsu/expertiza
- Link to Pull Request
- We have deployed the project at http://152.7.99.122:8080/
- Instructor login: username -> instructor6, password -> password
- Student login: username -> student5899, password -> password