CSC/ECE 517 Fall 2023 - E2375. Refactor classes relating to signing up for topics: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(80 intermediate revisions by 2 users not shown)
Line 3: Line 3:
* This refactoring endeavor aims to declutter, reorganize, and enhance code readability and maintainability by removing redundant functionalities and repositioning methods in classes where they serve their intended purposes more explicitly. The end goal is a more coherent and efficient codebase that reflects the current needs of the project while laying the groundwork for future enhancements.
* This refactoring endeavor aims to declutter, reorganize, and enhance code readability and maintainability by removing redundant functionalities and repositioning methods in classes where they serve their intended purposes more explicitly. The end goal is a more coherent and efficient codebase that reflects the current needs of the project while laying the groundwork for future enhancements.


== Methods to be refactored ==
== Flowchart Illustrating Our Approach ==
* There's a need for a thorough reevaluation and reorganization of the methods within the classes related to signing up for topics. Let's break down the key observations and areas requiring attention in each class:
* Below is the flow of our approach to signing up a user for a specific topic.
===Sign Up Sheet===
[[File:E2375 FlowChart.drawio.png|400px|center]]
* Methods like '''signup_team''', '''confirmTopic''', '''create_SignUpTeam''', and '''slotAvailable?''' lack clear indication of their purpose or follow confusing naming conventions.
 
=== Signup Topic ===
== UML Diagram ==
* Methods like '''find_slots_filled''', '''find_slots_waitlisted''', and '''slotAvailable?''' lack clarity in their purpose or could be misleading.
* Below is the UML diagram of our proposed approach
=== Signed Up Team ===
[[File:E2375_UML_OODD_Final.png|600px|center]]
* Methods like '''find_team_participants''' could be named more intuitively, while others like '''topic_id suggest''' a Law of Demeter violation.
 
=== Overview ===
 
*  Many methods lack clear and descriptive names, making it challenging to discern their purpose without diving into their implementation.
'''SignUpSheet Class:'''
* There are instances of redundant functionalities and methods reimplemented in different classes, indicating potential code consolidation opportunities.
The `SignUpSheet` class has a single method:
*  Reassigning methods to classes where they logically belong and adhering to encapsulation principles could significantly enhance code readability and maintainability.
* '''`signup_team(assignment_id, user_id, topic_id = nil)`:''' Used to sign up a team for a chosen topic. It internally communicates with the `SignUpTopic` class.
To improve these classes, a systematic approach to renaming, reassigning, and potentially consolidating methods could streamline functionality, improve code readability, and adhere more closely to object-oriented design principles. Refactoring should focus on clearer method naming, reducing redundancy, adhering to encapsulation, and promoting better organization within the codebase.
 
'''SignUpTopic Class:'''
The `SignUpTopic` class has three methods:
* '''`signup_team_for_chosen_topic(team_id)`:''' Handles the process of signing up a team for a chosen topic.
* '''`assign_topic_to_team(sign_up, topic_id)`:''' Assigns a topic to a team and updates the waitlist status.
* '''`save_waitlist_entry(sign_up, team_id)`:''' Saves a team to the waitlist if no slots are available. It also logs the creation of the sign-up sheet for waitlisted users. This class communicates with the `SignedUpTeam` class.
 
'''SignedUpTeam Class:'''
The `SignedUpTeam` class has two methods:
* '''`drop_off_waitlists(team_id)`:''' Drops a team from all waitlists.
* '''`find_user_signup_topics(assignment_id, team_id)`:''' Finds all topics for a user within a team for a specific assignment.
 
'''Associations:'''
* The associations between classes represent communication or dependency. For example, `SignUpSheet` communicates with `SignUpTopic`, and `SignUpTopic` communicates with `SignedUpTeam`.
 
Each class is represented with its methods, and the associations show the flow of communication between these classes. The "1" near the associations signifies the multiplicity or the number of instances participating in the relationship.


== Design in sign_up_sheet.rb file==
== Design in sign_up_sheet.rb file==


===(1) self.signup_team===
''' Methods modified'''


'''Existing code:'''
=== self.signup_team===
<pre> def self.signup_team(assignment_id, user_id, topic_id = nil)
 
     users_team = SignedUpTeam.find_team_users(assignment_id, user_id)
Below are the changes made to the existing signup_team method
     if users_team.empty?
[[File:signup_team.png|1200px|center]]
      # if team is not yet created, create new team.
 
      # create Team and TeamNode
'''Modified Code:'''
<pre>
  def self.signup_team(assignment_id, user_id, topic_id = nil)
     # Find the team ID for the given assignment and user
    team_id = Team.find_team_users(assignment_id, user_id)&.first&.t_id
     # If the team doesn't exist, create a new team and assign the team ID
    if team_id.nil?
       team = AssignmentTeam.create_team_with_users(assignment_id, [user_id])
       team = AssignmentTeam.create_team_with_users(assignment_id, [user_id])
       # create SignedUpTeam
       team_id = team.id
      confirmationStatus = SignUpSheet.confirmTopic(user_id, team.id, topic_id, assignment_id) if topic_id
    end
     else
    # Confirm the signup topic if a topic ID is provided
       confirmationStatus = SignUpSheet.confirmTopic(user_id, users_team[0].t_id, topic_id, assignment_id) if topic_id
    @signup_topic = SignUpTopic.find_by(id: topic_id)
     unless @signup_topic.nil?
       confirmation_status = @signup_topic.signup_team_for_chosen_topic(team_id)
     end
     end
     ExpertizaLogger.info "The signup topic save status:#{confirmationStatus} for assignment #{assignment_id} by #{user_id}"
    # Log the signup topic save status
     confirmationStatus
     ExpertizaLogger.info "The signup topic save status:#{confirmation_status} for assignment #{assignment_id} by #{user_id}"
     confirmation_status
   end
   end
</pre>
</pre>


'''Code smells:'''
Eliminated Methods in sign_up_sheet.rb
* Conditional Complexity: There's conditional complexity within the method based on whether users_team is empty or not, leading to slightly repetitive code.
We removed the methods '''self.confirmTopic''','''self.create_SignUpTeam''' and '''self.slotAvailable?''' in '''signup_sheet.rb''' due to redundancy, lengthiness, and unnecessary repetitive checks. We reimplemented new methods for this functionality in '''sign_up_topic.rb''' focusing on eliminating redundant checks and optimizing the flow for better efficiency and readability.
* Lack of Clear Separation of Concerns: The method handles team creation, sign-up confirmation, and logging within the same block, which might violate the single responsibility principle.
* Magic Numbers/Variables: users_team[0].t_id uses index 0 directly, assuming there's always at least one entry in users_team.


* Change method name to Signup_sheet.
=== self.confirmTopic ===


'''What to improve'''
* Error Handling: Add error handling for potential failures during team creation or sign-up processes.
===(2) self.confirmTopic===
'''Existing Code:'''
<pre> def self.confirmTopic(user_id, team_id, topic_id, assignment_id)
<pre> def self.confirmTopic(user_id, team_id, topic_id, assignment_id)
     # check whether user has signed up already
     # check whether user has signed up already
Line 90: Line 106:
</pre>
</pre>


'''Code smells:'''
=== self.create_SignUpTeam ===
* Method Length: The method is quite lengthy and performs multiple actions. It might benefit from breaking down into smaller, more focused methods.
* Complex Conditional Flow: The conditional flow involving transactions and multiple conditions could be challenging to follow and understand at first glance.


'''Design Considerations:'''
* Separation of Concerns: Ensure clear separation of concerns within the method. Each section of code should handle a specific aspect of the logic.
'''Refactoring and Code Enhancements:'''
* Extract Conditional Blocks into Methods: Break down conditional blocks into smaller methods to improve readability and maintainability. For instance, separate logic for handling cases where the user has already signed up or is on the waitlist.
* Improve Variable Naming: Use more descriptive variable names for better readability and understanding of the code. For example, renaming result, sign_up, user_signup, and user_signup_topic to more descriptive names would enhance clarity.
===(3) self.create_SignUpTeam===
'''Existing Code:'''
<pre> def self.create_SignUpTeam(assignment_id, sign_up, topic_id, user_id)
<pre> def self.create_SignUpTeam(assignment_id, sign_up, topic_id, user_id)
     if slotAvailable?(topic_id)
     if slotAvailable?(topic_id)
Line 119: Line 123:
</pre>
</pre>


'''Issues in the above method'''
=== self.slotAvailable? ===
* Naming Convention: The method name create_SignUpTeam should follow Ruby's naming convention, which typically uses snake_case for method names. Renaming it to create_sign_up_team would align better with Ruby conventions.
* Variable Ambiguity: The method uses topic_id and team_id as both method parameters and local variables. Reassigning these variables inside the method can cause confusion and may lead to unexpected behavior. It's advisable to use different variable names to avoid ambiguity.


'''Suggestions for Improvement:'''
<pre>def self.slotAvailable?(topic_id)
* Refactor for Clarity: Separate the logic for creating sign-up sheets and managing waitlist status into distinct methods to improve readability and maintainability.
    SignUpTopic.slotAvailable?(topic_id)
* Variable Naming: Use more descriptive variable names to improve clarity and avoid reassigning values to parameters.
end</pre>
* Use Constants: Replace hardcoded values like 0 and nil with constants or meaningful representations for better code readability.
 
== Design in sign_up_topic.rb file==
 
''' Newly implemented methods for topic sign-up functionality. '''
 
=== signup_team_for_chosen_topic ===
 
<pre>
def signup_team_for_chosen_topic(team_id)
    topic_id = self.id
    team = Team.find(team_id)
    # Fetch all topics for the user within the team for the assignment
    user_signup = SignedUpTeam.find_user_signup_topics(team.parent_id, team_id)
    # Check if the user is already signed up and waitlisted for the topic
    if !user_signup.empty?
      return false unless user_signup.first&.is_waitlisted == true
    end
    # Create a new SignedUpTeam instance with the provided topic and team details
    sign_up = SignedUpTeam.new(topic_id: topic_id, team_id: team_id)
    if SignUpTopic.slot_available?(topic_id)
      # Assign the topic to the team if a slot is available and drop off the team from all waitlists
      SignUpTopic.assign_topic_to_team(sign_up, topic_id)
      # Once assigned, drop all the waitlisted topics for this team
      result = SignedUpTeam.drop_off_waitlists(team_id)
    else
      # Save the team as waitlisted if no slots are available
      result = SignUpTopic.save_waitlist_entry(sign_up, team_id)
    end
    result
end
</pre>


'''Issues with the Return Statement:'''
This method, '''signup_team_for_chosen_topic''', facilitates the signup process for a chosen topic by a team. Here's a breakdown of its functionalities:
* Lack of Clarity: The return statement doesn't explicitly communicate the method's primary purpose or what the caller should do with the returned values. It returns two values without context, which might lead to ambiguity.
* Mixed Responsibilities: The method's primary responsibility seems to be creating a sign-up team or managing waitlist status. Returning [team_id, topic_id] might not be directly related to these actions and could confuse the caller about the returned values' significance.


'''Suggestions for Improvement:'''
* '''Checking User's Signup Status:''' It verifies if the user is already signed up and waitlisted for the chosen topic within their team for a specific assignment.
* Use Struct or Hash: Instead of returning an array of values without context, consider using a Ruby Struct or a Hash to encapsulate the return values. This way, the returned data can be labeled with descriptive keys, providing better context to the caller.
 
* Example using a Struct:
* '''Creating a New Signup Entry:''' If the user is not already signed up, it creates a new SignedUpTeam entry for the provided topic and team details.
<pre>SignUpResult = Struct.new(:team_id, :topic_id)
 
return SignUpResult.new(team_id, topic_id)
* '''Slot Availability Check:''' It verifies if a slot is available for the topic. If a slot is open: Assigns the topic to the team and then removes the team from any existing waitlists.
* '''Handling Waitlist:''' If no slots are available, the team is saved to the waitlist for the topic.
 
This method effectively manages the signup process by ensuring slot availability and handling waitlist scenarios for teams signing up for specific topics.
 
=== self.assign_topic_to_team ===
 
<pre>
def self.assign_topic_to_team(sign_up, topic_id)
    # Set the team's waitlist status to false as they are assigned a topic
    sign_up.update(is_waitlisted: false)
    # Update the topic_id in the signed_up_teams table for the user
    signed_up_team = SignedUpTeam.find_by(topic_id: topic_id)
    signed_up_team.update(topic_id: topic_id) if signed_up_team
end
</pre>
</pre>


===(4) self.slotAvailable? ===
This method, '''assign_topic_to_team''', is responsible for assigning a topic to a team within the signup process. Here's a summary of its functionalities:
 
* '''Waitlist Status Update:''' It updates the team's waitlist status to false as they are assigned a topic, indicating that they are no longer on the waitlist.
 
* '''Updating Topic ID:''' It updates the topic_id in the SignedUpTeam table for the user who has been assigned the topic. It finds the relevant entry based on the topic_id and updates it if the entry exists.
 
This method manages the assignment process by updating the team's status and ensuring the appropriate update of the topic ID in the SignedUpTeam table.
 
=== self.save_waitlist_entry ===
<pre>
def self.save_waitlist_entry(sign_up, team_id)
    sign_up.is_waitlisted = true
    # Save the user's waitlist status
    result = sign_up.save
    # Log the creation of the sign-up sheet for the waitlisted user
    ExpertizaLogger.info(LoggerMessage.new('SignUpSheet', '', "Sign up sheet created for waitlisted with teamId #{team_id}"))
    result
end
</pre>


'''Existing Code:'''
<pre>def self.slotAvailable?(topic_id)
    SignUpTopic.slotAvailable?(topic_id)
end</pre>


'''Issues:'''
The '''save_waitlist_entry''' method is responsible for saving the signup entry for users who have been waitlisted. Here's a brief overview of its functionality:
* Code Duplication: By redefining the slotAvailable? method in the current file when it already exists in another file, the code introduces unnecessary duplication. This leads to maintenance issues as any changes to the logic or behavior of this method need to be updated in both places, which can be error-prone and time-consuming.
* Violation of DRY Principle: The "Don't Repeat Yourself" (DRY) principle suggests that code should have a single, unambiguous representation within a system. Redefining the same method in different places violates this principle, potentially causing inconsistencies and confusion.


== Design in sign_up_topic.rb file==
* '''Updating Waitlist Status:''' It sets the is_waitlisted attribute of the sign_up object to true, indicating that the user is being waitlisted for a particular topic.


=== (1) self.find_slots_filled===
* '''Saving Waitlist Status:''' It saves this updated status in the database, ensuring that the user's waitlist status is correctly recorded.


'''Existing Code:'''
* '''Logging Creation:''' Additionally, it logs the creation of the signup sheet for the waitlisted user, providing a record of the waitlist action for the specified team ID.
<pre>def self.find_slots_filled(assignment_id)
    # SignUpTopic.find_by_sql("SELECT topic_id as topic_id, COUNT(t.max_choosers) as count      FROM sign_up_topics t JOIN signed_up_teams u ON t.id = u.topic_id WHERE t.assignment_id =" + assignment_id+  " and u.is_waitlisted = false GROUP BY t.id")
    SignUpTopic.find_by_sql(['SELECT topic_id as topic_id, COUNT(t.max_choosers) as count FROM sign_up_topics t JOIN signed_up_teams u ON t.id = u.topic_id WHERE t.assignment_id = ? and u.is_waitlisted = false GROUP BY t.id', assignment_id])
end</pre>


'''Problems Identified:'''
This method handles the process of marking users as waitlisted, recording this status in the database, and logging this action for reference purposes.
* SQL Query Within the Method: The method contains a SQL query directly embedded in the code. Embedding SQL queries like this can make the code less maintainable and harder to understand, especially for developers who might not be familiar with SQL or the database schema.
* Complex SQL Logic: The SQL query combines multiple tables (sign_up_topics and signed_up_teams) and performs aggregations (COUNT) and joins based on certain conditions (is_waitlisted = false). Such complexity in the query can make it challenging to troubleshoot or modify in the future.


'''Suggestions for Improvement:'''
== Design in signed_up_team.rb file==
* Separation of Concerns: Extract the SQL query into a separate method or a query builder class to abstract away the SQL complexities from the method. This separation helps in better code organization and maintainability.
* Encapsulate SQL Logic: Abstract the SQL logic into a method that accepts necessary parameters, allowing better reusability across different parts of the application without duplicating code.


===(2) self.find_slots_waitlisted===
We relocated the '''self.find_team_users''' method to '''Team.rb''' to align its functionality with the appropriate file, as it pertains to the functionalities defined in '''Team.rb'''.


'''Existing Code:'''
<pre>
<pre>def self.find_slots_waitlisted(assignment_id)
  def self.find_team_users(assignment_id, user_id)
     # SignUpTopic.find_by_sql("SELECT topic_id as topic_id, COUNT(t.max_choosers) as count FROM sign_up_topics t JOIN signed_up_teams u ON t.id = u.topic_id WHERE t.assignment_id =" + assignment_id +  " and u.is_waitlisted = true GROUP BY t.id")
     TeamsUser.joins('INNER JOIN teams ON teams_users.team_id = teams.id')
    SignUpTopic.find_by_sql(['SELECT topic_id as topic_id, COUNT(t.max_choosers) as count FROM sign_up_topics t JOIN signed_up_teams u ON t.id = u.topic_id WHERE t.assignment_id = ? and u.is_waitlisted = true GROUP BY t.id', assignment_id])
            .select('teams.id as t_id')
   End</pre>
            .where('teams.parent_id = ? and teams_users.user_id = ?', assignment_id, user_id)
   end
</pre>


'''Identified Issues:'''
== Testing Plan ==
* SQL Query Embedded in Code: Similar to the previous method, this method contains a SQL query embedded directly within the code. This can lead to reduced maintainability and readability, especially for developers unfamiliar with the SQL logic or database schema.
* Complex SQL Logic: The SQL query combines tables (sign_up_topics and signed_up_teams), performs aggregations (COUNT), and includes conditional logic (is_waitlisted = true). This level of complexity in the query might hinder code readability, debugging, and future modifications.


'''Suggestions for Improvement:'''
=== RSpec===
* Separation of Concerns: Extract the SQL query logic into a separate method or a query builder class to separate it from the method. This separation aids in better code organization and maintainability.
The test scenarios cover various functionalities of the system under test across different modules. Each test scenario includes a set of detailed test cases designed to validate specific behaviors and functionalities. These test cases encompass positive and negative test scenarios, checking inputs, expected outputs, and system responses under varying conditions. The scenarios focus on functionalities such as slot availability checks, team signup operations, waitlist handling, and retrieval of topic IDs by team IDs. These tests aim to ensure the system operates as intended, detecting and reporting any anomalies or deviations from expected behavior
* Encapsulation of SQL Logic: Abstract the SQL logic into a method that accepts necessary parameters, facilitating better reuse across different sections of the application without repeating the code.


=== (3) self.slotAvailable? ===
'''spec/models/sign_up_topic_spec.rb'''


'''Existing Code:'''
'''1 : slot_available?'''
<pre>def self.slotAvailable?(topic_id)
<pre>
     topic = SignUpTopic.find(topic_id)
describe 'slot_available?' do
     no_of_students_who_selected_the_topic = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: false)
    let(:topic) { instance_double(SignUpTopic, id: 1, max_choosers: 3) }
 
    context 'when no students have selected the topic yet' do
      it 'returns true' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
       
        expect(SignUpTopic.slot_available?(1)).to eq(true)
      end
    end
 
     context 'when students have selected the topic but slots are available' do
      it 'returns true' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([double, double])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
       
        expect(SignUpTopic.slot_available?(1)).to eq(true)
      end
     end
 
    context 'when all slots for the topic are filled' do
      it 'returns false' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([double, double, double])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
       
        expect(SignUpTopic.slot_available?(1)).to eq(false)
      end
    end
end
</pre>


     if no_of_students_who_selected_the_topic.nil?
'''2 : save_waitlist_entry'''
       return true
<pre>
     else
describe 'save_waitlist_entry' do
       if topic.max_choosers > no_of_students_who_selected_the_topic.size
    let(:sign_up) { instance_double(SignedUpTeam, is_waitlisted: false, save: true) }
         return true
    let(:logger_message) { instance_double(LoggerMessage) }
      else
 
         return false
     before do
      allow(LoggerMessage).to receive(:new).and_return(logger_message)
      allow(sign_up).to receive(:is_waitlisted=)
       allow(sign_up).to receive(:save).and_return(true)
      allow(ExpertizaLogger).to receive(:info)
    end
 
     context 'when saving the user as waitlisted' do
       it 'updates the user\'s waitlist status and logs the creation of sign-up sheet' do
        result = SignUpTopic.save_waitlist_entry(sign_up, 123)
 
        expect(sign_up).to have_received(:is_waitlisted=).with(true)
        expect(sign_up).to have_received(:save)
         expect(LoggerMessage).to have_received(:new).with('SignUpSheet', '', "Sign up sheet created for waitlisted with teamId 123")
        expect(ExpertizaLogger).to have_received(:info).with(logger_message)
         expect(result).to eq(true)
       end
       end
     end
     end
Line 203: Line 294:
</pre>
</pre>


'''Issues Identified:'''
'''3 : signup_team_for_chosen_topic'''
* Instance vs. Class Method: This method could potentially be more appropriate as an instance method rather than a class method. It appears to operate on an instance of a topic (SignUpTopic), accessing specific data related to a topic instance. Refactoring it as an instance method might improve the code's readability and alignment with object-oriented principles.
<pre>
* Handling of no_of_students_who_selected_the_topic: The method checks if no_of_students_who_selected_the_topic is nil. However, the where method of ActiveRecord returns an empty ActiveRecord::Relation if no records match the query, never returning nil. Therefore, the condition if no_of_students_who_selected_the_topic.nil? will not be executed.
describe '#signup_team_for_chosen_topic' do
    let(:team) { create(:team) }
    let(:topic) { SignUpTopic.new(id: 1, max_choosers: 3) }


'''Suggestions for Improvement:'''
    context 'when the team is not already signed up or waitlisted for the topic' do
* Refactoring to Instance Method: If this method indeed pertains to a specific instance of SignUpTopic, refactor it into an instance method. This way, it can directly operate on a specific topic instance.
      before do
* Handle no_of_students_who_selected_the_topic Appropriately: Since no_of_students_who_selected_the_topic will never be nil, adjust the condition to handle an empty collection or use empty? to check if it's empty.
        allow(SignedUpTeam).to receive(:find_user_signup_topics).and_return([])
      end


== Design in signed_up_team.rb file ==
      it 'signs up the team for the chosen topic' do
        allow(topic).to receive(:signup_team_for_chosen_topic).and_return(true)
        allow(SignUpTopic).to receive(:slot_available?).and_return(true)
        expect(topic.signup_team_for_chosen_topic(team.id)).to eq(true)
      end
    end


=== (1) self.find_team_participants ===
    context 'when the team is already signed up and waitlisted for the topic' do
      before do
        allow(SignedUpTeam).to receive(:find_user_signup_topics).and_return([double(is_waitlisted: true)])
      end


'''Existing Code:'''
      it 'does not create a new signup entry' do
<pre>def self.find_team_participants(assignment_id, ip_address = nil)
        allow(topic).to receive(:signup_team_for_chosen_topic).and_return(false)
    @participants = SignedUpTeam.joins('INNER JOIN sign_up_topics ON signed_up_teams.topic_id = sign_up_topics.id')
        expect(topic.signup_team_for_chosen_topic(team.id)).to eq(false)
                                .select('signed_up_teams.id as id, sign_up_topics.id as topic_id, sign_up_topics.topic_name as name,
      end
                                  sign_up_topics.topic_name as team_name_placeholder, sign_up_topics.topic_name as user_name_placeholder,
    end
                                  signed_up_teams.is_waitlisted as is_waitlisted, signed_up_teams.team_id as team_id')
                                .where('sign_up_topics.assignment_id = ?', assignment_id)
    @participants.each_with_index do |participant, i|
      participant_names = User.joins('INNER JOIN teams_users ON users.id = teams_users.user_id')
                              .joins('INNER JOIN teams ON teams.id = teams_users.team_id')
                              .select('users.name as u_name, teams.name as team_name')
                              .where('teams.id = ?', participant.team_id)


       team_name_added = false
    context 'when there are no available slots for the topic' do
       names = '(missing team)'
       before do
        allow(SignUpTopic).to receive(:slot_available?).and_return(false)
       end


       participant_names.each do |participant_name|
       it 'creates a new waitlisted signup entry' do
         if team_name_added
         allow(topic).to receive(:signup_team_for_chosen_topic).and_return(true)
          names += User.find_by(name: participant_name.u_name).name(ip_address) + ' '
         expect(topic.signup_team_for_chosen_topic(team.id)).to eq(true)
          participant.user_name_placeholder += User.find_by(name: participant_name.u_name).name(ip_address) + ' '
         else
          names = '[' + participant_name.team_name + '] ' + User.find_by(name: participant_name.u_name).name(ip_address) + ' '
          participant.team_name_placeholder = participant_name.team_name
          participant.user_name_placeholder = User.find_by(name: participant_name.u_name).name(ip_address) + ' '
          team_name_added = true
        end
       end
       end
      @participants[i].name = names
    end
    End</pre>
end
</pre>


'''Problems Identified:'''
'''spec/models/sign_up_team_spec.rb'''
* Naming Convention: Using an instance variable @participants suggests that this method is intended to populate a class instance variable. However, this method is a class method, and using instance variables within it can cause confusion. Renaming it to a local variable like participants would be more appropriate.
* Redundant Database Queries: Within the iteration, there are multiple calls to the database (User.find_by(name: participant_name.u_name).name(ip_address)). These queries within loops can significantly impact performance, leading to potential N+1 query problems.


'''Suggestions for Improvement:'''
'''1 : topic_id_by_team_id'''
* Use of Local Variable: Instead of using @participants, use a local variable participants within the scope of the method. This change clarifies that it's not intended to store data persistently in the class.
<pre>
* Reduce Database Queries in Loops: Retrieve necessary data beforehand and avoid querying the database multiple times within loops. Utilize data structures or caching mechanisms to store and reuse fetched data efficiently.
describe '.topic_id_by_team_id' do
    let(:team_id_existing) { 1 }
    let(:team_id_non_existing) { 2 }


=== (2) self.topic_id===
    it 'returns topic_id when team has signed up and not on waitlist' do
      # Create a SignedUpTeam record with team_id_existing and is_waitlisted: false
      signed_up_team = create(:signed_up_team, team_id: team_id_existing, is_waitlisted: false)
      expect(described_class.topic_id_by_team_id(team_id_existing)).to eq(signed_up_team.topic_id)
    end


'''Existing Code:'''
    it 'returns nil when team has signed up but is on waitlist' do
<pre>def self.topic_id(assignment_id, user_id)
      # Create a SignedUpTeam record with team_id_existing and is_waitlisted: true
    # team_id variable represents the team_id for this user in this assignment
      create(:signed_up_team, team_id: team_id_existing, is_waitlisted: true)
    team_id = TeamsUser.team_id(assignment_id, user_id)
      expect(described_class.topic_id_by_team_id(team_id_existing)).to be_nil
    topic_id_by_team_id(team_id) if team_id
    end
  End</pre>


'''Issues Identified:'''
    it 'returns nil when team has not signed up' do
* LoD Violation: Indirect Access to Associated Model: The method TeamsUser.team_id(assignment_id, user_id) is accessed directly within topic_id, which in turn calls topic_id_by_team_id(team_id) if team_id is present. This pattern violates the Law of Demeter by reaching into the structure of associated models, potentially leading to tight coupling and decreased maintainability.
      expect(described_class.topic_id_by_team_id(team_id_non_existing)).to be_nil
    end
end
</pre>
 
''' Screenshot of RSpec Test Cases Running Successfully '''


'''Suggestions for Improvement:'''
[[File:E2375 testcases.jpeg|1000px]]
* Encapsulation and Modularity: To adhere more closely to the Law of Demeter, consider encapsulating this logic within the appropriate model or service. This helps in keeping responsibilities more contained within their respective models or services, promoting modularity and separation of concerns.
* Direct Association Usage: If available, utilize direct associations or relationships defined within the models to access related data more directly. This approach aligns better with the Law of Demeter by restricting access to a model's immediate relationships rather than reaching multiple levels down the association chain.


== Conclusion ==
==Demo Video Link==
In conclusion, the outlined refactoring process presents a clear path towards substantial enhancements within the sign-up management classes of Expertiza. By addressing the identified issues—confusing method names, redundant functionalities, and violations of coding principles—we're poised to elevate the functionality and maintainability of these crucial components.
*[https://www.youtube.com/watch?v=1SFfc1I1A_0 https://www.youtube.com/watch?v=1SFfc1I1A_0]


The proposed improvements, encompassing method renaming, separation of concerns, and adherence to coding conventions, signify a concerted effort to refine the codebase. These changes are not merely about tidying up the existing code; they are a strategic investment in the project's future.
==Team==
==Team==
=====Mentor=====  
=====Mentor=====  
Line 281: Line 377:
* Prateek Singamsetty <pksingam@ncsu.edu>
* Prateek Singamsetty <pksingam@ncsu.edu>
* Rushabh Shah <rshah32@ncsu.edu>
* Rushabh Shah <rshah32@ncsu.edu>
==Pull Request==
*[https://github.com/expertiza/expertiza/pull/2708 https://github.com/expertiza/expertiza/pull/2708]

Latest revision as of 05:40, 5 December 2023

Introduction

  • When managing sign-ups within expertiza, the evolution of functionality often leads to a need for refactoring existing code. In this case, several classes— sign_up_sheet.rb, sign_up_topic.rb, and signed_up_team.rb —require restructuring to streamline their functionalities and ensure they align with the current project landscape.
  • This refactoring endeavor aims to declutter, reorganize, and enhance code readability and maintainability by removing redundant functionalities and repositioning methods in classes where they serve their intended purposes more explicitly. The end goal is a more coherent and efficient codebase that reflects the current needs of the project while laying the groundwork for future enhancements.

Flowchart Illustrating Our Approach

  • Below is the flow of our approach to signing up a user for a specific topic.

UML Diagram

  • Below is the UML diagram of our proposed approach


SignUpSheet Class: The `SignUpSheet` class has a single method:

  • `signup_team(assignment_id, user_id, topic_id = nil)`: Used to sign up a team for a chosen topic. It internally communicates with the `SignUpTopic` class.

SignUpTopic Class: The `SignUpTopic` class has three methods:

  • `signup_team_for_chosen_topic(team_id)`: Handles the process of signing up a team for a chosen topic.
  • `assign_topic_to_team(sign_up, topic_id)`: Assigns a topic to a team and updates the waitlist status.
  • `save_waitlist_entry(sign_up, team_id)`: Saves a team to the waitlist if no slots are available. It also logs the creation of the sign-up sheet for waitlisted users. This class communicates with the `SignedUpTeam` class.

SignedUpTeam Class: The `SignedUpTeam` class has two methods:

  • `drop_off_waitlists(team_id)`: Drops a team from all waitlists.
  • `find_user_signup_topics(assignment_id, team_id)`: Finds all topics for a user within a team for a specific assignment.

Associations:

  • The associations between classes represent communication or dependency. For example, `SignUpSheet` communicates with `SignUpTopic`, and `SignUpTopic` communicates with `SignedUpTeam`.

Each class is represented with its methods, and the associations show the flow of communication between these classes. The "1" near the associations signifies the multiplicity or the number of instances participating in the relationship.

Design in sign_up_sheet.rb file

Methods modified

self.signup_team

Below are the changes made to the existing signup_team method

Modified Code: 

   def self.signup_team(assignment_id, user_id, topic_id = nil)
    # Find the team ID for the given assignment and user
    team_id = Team.find_team_users(assignment_id, user_id)&.first&.t_id
    # If the team doesn't exist, create a new team and assign the team ID
    if team_id.nil?
      team = AssignmentTeam.create_team_with_users(assignment_id, [user_id])
      team_id = team.id
    end
    # Confirm the signup topic if a topic ID is provided
    @signup_topic = SignUpTopic.find_by(id: topic_id)
    unless @signup_topic.nil?
      confirmation_status = @signup_topic.signup_team_for_chosen_topic(team_id)
    end
    # Log the signup topic save status
    ExpertizaLogger.info "The signup topic save status:#{confirmation_status} for assignment #{assignment_id} by #{user_id}"
    confirmation_status
  end

Eliminated Methods in sign_up_sheet.rb

We removed the methods self.confirmTopic,self.create_SignUpTeam and self.slotAvailable? in signup_sheet.rb due to redundancy, lengthiness, and unnecessary repetitive checks. We reimplemented new methods for this functionality in sign_up_topic.rb focusing on eliminating redundant checks and optimizing the flow for better efficiency and readability.

self.confirmTopic

 def self.confirmTopic(user_id, team_id, topic_id, assignment_id)
    # check whether user has signed up already
    user_signup = SignUpSheet.otherConfirmedTopicforUser(assignment_id, team_id)
    users_team = SignedUpTeam.find_team_users(assignment_id, user_id)
    team = Team.find(users_team.first.t_id)
    if SignedUpTeam.where(team_id: team.id, topic_id: topic_id).any?
      return false
    end

    sign_up = SignedUpTeam.new
    sign_up.topic_id = topic_id
    sign_up.team_id = team_id
    result = false
    if user_signup.empty?

      # Using a DB transaction to ensure atomic inserts
      ApplicationRecord.transaction do
        # check whether slots exist (params[:id] = topic_id) or has the user selected another topic
        team_id, topic_id = create_SignUpTeam(assignment_id, sign_up, topic_id, user_id)
        result = true if sign_up.save
      end
    else
      # This line of code checks if the "user_signup_topic" is on the waitlist. If it is not on the waitlist, then the code returns 
      # false. If it is on the waitlist, the code continues to execute.
      user_signup.each do |user_signup_topic|
        return false unless user_signup_topic.is_waitlisted
      end

      # Using a DB transaction to ensure atomic inserts
      ApplicationRecord.transaction do
        # check whether user is clicking on a topic which is not going to place him in the waitlist
        result = sign_up_wailisted(assignment_id, sign_up, team_id, topic_id)
      end
    end

    result
  end

self.create_SignUpTeam

 def self.create_SignUpTeam(assignment_id, sign_up, topic_id, user_id)
    if slotAvailable?(topic_id)
      sign_up.is_waitlisted = false
      # Create new record in signed_up_teams table
      team_id = TeamsUser.team_id(assignment_id, user_id)
      topic_id = SignedUpTeam.topic_id(assignment_id, user_id)
      SignedUpTeam.create(topic_id: topic_id, team_id: team_id, is_waitlisted: 0, preference_priority_number: nil)
      ExpertizaLogger.info LoggerMessage.new('SignUpSheet', user_id, "Sign up sheet created with teamId #{team_id}")
    else
      sign_up.is_waitlisted = true
    end
    [team_id, topic_id]
  end

self.slotAvailable?

def self.slotAvailable?(topic_id)
    SignUpTopic.slotAvailable?(topic_id)
end

Design in sign_up_topic.rb file

Newly implemented methods for topic sign-up functionality.

signup_team_for_chosen_topic

def signup_team_for_chosen_topic(team_id)
    topic_id = self.id
    team = Team.find(team_id)
    # Fetch all topics for the user within the team for the assignment
    user_signup = SignedUpTeam.find_user_signup_topics(team.parent_id, team_id)
    # Check if the user is already signed up and waitlisted for the topic
    if !user_signup.empty?
      return false unless user_signup.first&.is_waitlisted == true
    end
    # Create a new SignedUpTeam instance with the provided topic and team details
    sign_up = SignedUpTeam.new(topic_id: topic_id, team_id: team_id)
    if SignUpTopic.slot_available?(topic_id)
      # Assign the topic to the team if a slot is available and drop off the team from all waitlists
      SignUpTopic.assign_topic_to_team(sign_up, topic_id)
      # Once assigned, drop all the waitlisted topics for this team
      result = SignedUpTeam.drop_off_waitlists(team_id)
    else
      # Save the team as waitlisted if no slots are available
      result = SignUpTopic.save_waitlist_entry(sign_up, team_id)
    end
    result
end

This method, signup_team_for_chosen_topic, facilitates the signup process for a chosen topic by a team. Here's a breakdown of its functionalities:

  • Checking User's Signup Status: It verifies if the user is already signed up and waitlisted for the chosen topic within their team for a specific assignment.
  • Creating a New Signup Entry: If the user is not already signed up, it creates a new SignedUpTeam entry for the provided topic and team details.
  • Slot Availability Check: It verifies if a slot is available for the topic. If a slot is open: Assigns the topic to the team and then removes the team from any existing waitlists.
  • Handling Waitlist: If no slots are available, the team is saved to the waitlist for the topic.

This method effectively manages the signup process by ensuring slot availability and handling waitlist scenarios for teams signing up for specific topics.

self.assign_topic_to_team

def self.assign_topic_to_team(sign_up, topic_id)
    # Set the team's waitlist status to false as they are assigned a topic
    sign_up.update(is_waitlisted: false)
    # Update the topic_id in the signed_up_teams table for the user
    signed_up_team = SignedUpTeam.find_by(topic_id: topic_id)
    signed_up_team.update(topic_id: topic_id) if signed_up_team
end

This method, assign_topic_to_team, is responsible for assigning a topic to a team within the signup process. Here's a summary of its functionalities:

  • Waitlist Status Update: It updates the team's waitlist status to false as they are assigned a topic, indicating that they are no longer on the waitlist.
  • Updating Topic ID: It updates the topic_id in the SignedUpTeam table for the user who has been assigned the topic. It finds the relevant entry based on the topic_id and updates it if the entry exists.

This method manages the assignment process by updating the team's status and ensuring the appropriate update of the topic ID in the SignedUpTeam table.

self.save_waitlist_entry

def self.save_waitlist_entry(sign_up, team_id)
    sign_up.is_waitlisted = true
    # Save the user's waitlist status
    result = sign_up.save
    # Log the creation of the sign-up sheet for the waitlisted user
    ExpertizaLogger.info(LoggerMessage.new('SignUpSheet', '', "Sign up sheet created for waitlisted with teamId #{team_id}"))
    result
end


The save_waitlist_entry method is responsible for saving the signup entry for users who have been waitlisted. Here's a brief overview of its functionality:

  • Updating Waitlist Status: It sets the is_waitlisted attribute of the sign_up object to true, indicating that the user is being waitlisted for a particular topic.
  • Saving Waitlist Status: It saves this updated status in the database, ensuring that the user's waitlist status is correctly recorded.
  • Logging Creation: Additionally, it logs the creation of the signup sheet for the waitlisted user, providing a record of the waitlist action for the specified team ID.

This method handles the process of marking users as waitlisted, recording this status in the database, and logging this action for reference purposes.

Design in signed_up_team.rb file

We relocated the self.find_team_users method to Team.rb to align its functionality with the appropriate file, as it pertains to the functionalities defined in Team.rb. 

  def self.find_team_users(assignment_id, user_id)
    TeamsUser.joins('INNER JOIN teams ON teams_users.team_id = teams.id')
             .select('teams.id as t_id')
             .where('teams.parent_id = ? and teams_users.user_id = ?', assignment_id, user_id)
  end

Testing Plan

RSpec

The test scenarios cover various functionalities of the system under test across different modules. Each test scenario includes a set of detailed test cases designed to validate specific behaviors and functionalities. These test cases encompass positive and negative test scenarios, checking inputs, expected outputs, and system responses under varying conditions. The scenarios focus on functionalities such as slot availability checks, team signup operations, waitlist handling, and retrieval of topic IDs by team IDs. These tests aim to ensure the system operates as intended, detecting and reporting any anomalies or deviations from expected behavior

spec/models/sign_up_topic_spec.rb

1 : slot_available? 

describe 'slot_available?' do
    let(:topic) { instance_double(SignUpTopic, id: 1, max_choosers: 3) }
  
    context 'when no students have selected the topic yet' do
      it 'returns true' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
        
        expect(SignUpTopic.slot_available?(1)).to eq(true)
      end
    end
  
    context 'when students have selected the topic but slots are available' do
      it 'returns true' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([double, double])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
        
        expect(SignUpTopic.slot_available?(1)).to eq(true)
      end
    end
  
    context 'when all slots for the topic are filled' do
      it 'returns false' do
        allow(SignedUpTeam).to receive(:where).with(topic_id: 1, is_waitlisted: false).and_return([double, double, double])
        allow(SignUpTopic).to receive(:find).with(1).and_return(topic)
        
        expect(SignUpTopic.slot_available?(1)).to eq(false)
      end
    end
end

2 : save_waitlist_entry 

describe 'save_waitlist_entry' do
    let(:sign_up) { instance_double(SignedUpTeam, is_waitlisted: false, save: true) }
    let(:logger_message) { instance_double(LoggerMessage) }
  
    before do
      allow(LoggerMessage).to receive(:new).and_return(logger_message)
      allow(sign_up).to receive(:is_waitlisted=)
      allow(sign_up).to receive(:save).and_return(true)
      allow(ExpertizaLogger).to receive(:info)
    end
  
    context 'when saving the user as waitlisted' do
      it 'updates the user\'s waitlist status and logs the creation of sign-up sheet' do
        result = SignUpTopic.save_waitlist_entry(sign_up, 123)
  
        expect(sign_up).to have_received(:is_waitlisted=).with(true)
        expect(sign_up).to have_received(:save)
        expect(LoggerMessage).to have_received(:new).with('SignUpSheet', '', "Sign up sheet created for waitlisted with teamId 123")
        expect(ExpertizaLogger).to have_received(:info).with(logger_message)
        expect(result).to eq(true)
      end
    end
  end

3 : signup_team_for_chosen_topic 

describe '#signup_team_for_chosen_topic' do
    let(:team) { create(:team) }
    let(:topic) { SignUpTopic.new(id: 1, max_choosers: 3) }

    context 'when the team is not already signed up or waitlisted for the topic' do
      before do
        allow(SignedUpTeam).to receive(:find_user_signup_topics).and_return([])
      end

      it 'signs up the team for the chosen topic' do
        allow(topic).to receive(:signup_team_for_chosen_topic).and_return(true)
        allow(SignUpTopic).to receive(:slot_available?).and_return(true)
        expect(topic.signup_team_for_chosen_topic(team.id)).to eq(true)
      end
    end

    context 'when the team is already signed up and waitlisted for the topic' do
      before do
        allow(SignedUpTeam).to receive(:find_user_signup_topics).and_return([double(is_waitlisted: true)])
      end

      it 'does not create a new signup entry' do
        allow(topic).to receive(:signup_team_for_chosen_topic).and_return(false)
        expect(topic.signup_team_for_chosen_topic(team.id)).to eq(false)
      end
    end

    context 'when there are no available slots for the topic' do
      before do
        allow(SignUpTopic).to receive(:slot_available?).and_return(false)
      end

      it 'creates a new waitlisted signup entry' do
        allow(topic).to receive(:signup_team_for_chosen_topic).and_return(true)
        expect(topic.signup_team_for_chosen_topic(team.id)).to eq(true)
      end
    end
end

spec/models/sign_up_team_spec.rb

1 : topic_id_by_team_id 

describe '.topic_id_by_team_id' do
    let(:team_id_existing) { 1 }
    let(:team_id_non_existing) { 2 }

    it 'returns topic_id when team has signed up and not on waitlist' do
      # Create a SignedUpTeam record with team_id_existing and is_waitlisted: false
      signed_up_team = create(:signed_up_team, team_id: team_id_existing, is_waitlisted: false)
      expect(described_class.topic_id_by_team_id(team_id_existing)).to eq(signed_up_team.topic_id)
    end

    it 'returns nil when team has signed up but is on waitlist' do
      # Create a SignedUpTeam record with team_id_existing and is_waitlisted: true
      create(:signed_up_team, team_id: team_id_existing, is_waitlisted: true)
      expect(described_class.topic_id_by_team_id(team_id_existing)).to be_nil
    end

    it 'returns nil when team has not signed up' do
      expect(described_class.topic_id_by_team_id(team_id_non_existing)).to be_nil
    end
end

Screenshot of RSpec Test Cases Running Successfully

Demo Video Link

Team

Mentor
  • Ed Gehringer
Members
  • Dinesh Pasupuleti <dpasupu@ncsu.edu>
  • Prateek Singamsetty <pksingam@ncsu.edu>
  • Rushabh Shah <rshah32@ncsu.edu>

Pull Request

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2023_-_E2375._Refactor_classes_relating_to_signing_up_for_topics&oldid=152706"