← Back to Upcase

How can I improve this code?


(Cyle Hunter) #1

I’m working on a tournament application where users can register for competitions as individuals, or as part of a team depending on the format of the competition.

I have a Registration model which polymorphically represents either a User or a Team registering for the Competition, the type requires is determined by a registration_type column on the Competition model. A User can have and belong to many Teams, and it’s ID will appear in a list of ids when calling the Team objects user_ids methods. Here’s what my code looks like currently to facilitate this:

class Competition < ActiveRecord::Base
  has_many :registrations
  
  def registered?(user)
    case registration_type
    when "User"
      registrations.where(registerable: user).any?
    when "Team"
      ids = registrations.map {|r| r.registerable.user_ids }.flatten
      ids.include?(user.id)
    end
  end
end

class Registration < ActiveRecord::Base
  belongs_to :competition
  belongs_to :registerable, :polymorphic => true
end

class Team < ActiveRecord::Base
  has_and_belongs_to_many :users
  has_many :registrations, :as => :registerable
end

class User < ActiveRecord::Base
  has_and_belongs_to_many :teams
  has_many :registrations, :as => :registerable
end

And here’s a pretty ASCII diagram if it helps :smile: :

       .
      +---------------------------+    
      |         Competition       |    
      +-------------+-------------+    
                    |                  
                    |                  
        +-----------+-------------+    
        |      Registration       |    
        +----------+--------------+    
                   |                   
                   |                   
      +------------+-------------+     
      |                          |         
+-----v-----+             +------v----+
|    User   +-------------+    Team   |
+-----------+             +-----------+

The problem I’m facing is with the registered? method in the Competition model. Given a User instance, the Competition model will determine if the user is registered. This seemed like a logical decision to me because the Competition model has access to it’s collection of Registration objects, where as any given Registration instance will only know about it’s self.There’s two problems that I see with this approach:

  1. Registration logic is inside of the Competition model.
  2. There’s two different ways of determining whether any given User is registered based on the Competition object’s registration_type, i.e., his ID will appear directly in the registerable_id field if the registration_type is set to “User”, or else he’ll appear as one of the Team object’s user ids.

Is there an intelligent way to refactor this code? Other areas of my code were designed around Registration objects being single inclusive entities, so I’m hesitant about seperating it’s logic into different classes just for this one use case unless it really makes sense to do so.