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

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.

Methods to be refactored

• 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:

Sign Up Sheet

• Methods like signup_team, confirmTopic, create_SignUpTeam and slotAvailable? lack clear indication of their purpose or follow confusing naming conventions.

Signup Topic

• Confusing Methods: Methods like import, find_slots_filled, and find_slots_waitlisted lack clarity in their purpose or could be misleading.
• Reasonable Methods: slotAvailable? could be an instance method, aligning better with object-oriented principles.
• Potential Redundancy: Some methods seem redundant or have been reimplemented elsewhere, such as reassign_topic.

Signed Up Team

• Naming and Redundancy: Methods like find_team_participants could be named more intuitively, while others like topic_id and topic_id_by_team_id suggest a Law of Demeter violation.
• Opportunity for Improvement: Methods here could be refactored to utilize existing methods in other classes, like participants from the Team class.

Overview

• Naming and Clarity: Many methods lack clear and descriptive names, making it challenging to discern their purpose without diving into their implementation.
• Redundancy and Reimplementation: There are instances of redundant functionalities and methods reimplemented in different classes, indicating potential code consolidation opportunities.
• Encapsulation and Structure: Reassigning methods to classes where they logically belong and adhering to encapsulation principles could significantly enhance code readability and maintainability.

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.

Design in sign_up_sheet.rb file

(1) self.signup_team

Existing code:

 def self.signup_team(assignment_id, user_id, topic_id = nil)
    users_team = SignedUpTeam.find_team_users(assignment_id, user_id)
    if users_team.empty?
      # if team is not yet created, create new team.
      # create Team and TeamNode
      team = AssignmentTeam.create_team_with_users(assignment_id, [user_id])
      # create SignedUpTeam
      confirmationStatus = SignUpSheet.confirmTopic(user_id, team.id, topic_id, assignment_id) if topic_id
    else
      confirmationStatus = SignUpSheet.confirmTopic(user_id, users_team[0].t_id, topic_id, assignment_id) if topic_id
    end
    ExpertizaLogger.info "The signup topic save status:#{confirmationStatus} for assignment #{assignment_id} by #{user_id}"
    confirmationStatus
  end

Code smells:

• Conditional Complexity: There's conditional complexity within the method based on whether users_team is empty or not, leading to slightly repetitive code.
• 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.

What to improve

• Error Handling: Add error handling for potential failures during team creation or sign-up processes.

(2) self.confirmTopic

Code:

 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

Code smells:

• 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

Code:

 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

Issues in the above method

• 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:

• Refactor for Clarity: Separate the logic for creating sign-up sheets and managing waitlist status into distinct methods to improve readability and maintainability.
• Variable Naming: Use more descriptive variable names to improve clarity and avoid reassigning values to parameters.
• Use Constants: Replace hardcoded values like 0 and nil with constants or meaningful representations for better code readability.

Issues with the Return Statement:

• 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:

• 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:

SignUpResult = Struct.new(:team_id, :topic_id)
return SignUpResult.new(team_id, topic_id)

(4) self.slotAvailable?

Code:

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

Issues:

• 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.

Team

Mentor

• Ed Gehringer

Members

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