dimanche 28 mars 2021

Difficult extracting shared logic in two Rails classes adding/updating records

I have an app that aims to helps people to meet up. I have a database table that stores time windows of availability for a particular user:

AvailabilityWindow (user_id, start_time, end_time, can_travel, travel_radius, target_user, will_travel, state) (state can be 'active' or 'removed')

User availability can be open to all other users, or targeting a specific user (by using the target_user_id field in AvailabilityWindow). To make creating untargeted/targeted availability easier, I have two classes:

class UserOpenAvailability
   class << self
     def for(user)
       new(user, user.availability_windows.where(target_user_id: nil))
     end
   end

   def initialize(user, availability: [])
     @user = user
     @availabilities = availability
   end

   def add(start_time, end_time, can_travel: false, travel_radius: 0)
      @availabilities << AvailabilityWindow.new(user: user, start_time: start_time, end_time: end_time, can_travel: can_travel, travel_radus: travel_radius)
   end

   def existing_availability
     self.class.for(@user)
   end

   def save
     # complicated save logic
   end

end
class TargetedUserAvailability
   class << self
     def for(user, target_user)
       new(user, target_user, user.availability_windows.where(target_user: target_user))
     end
   end

   def initialize(user, target_user, availability: [])
      @user = user
      @target_user = target_user
      @availabilities = availability
   end

   def add(start_time, end_time, can_travel, travel_radius, will_travel: true)
      @availabilities << AvailabilityWindow.new(user: user, target_user: @target_user, start_time: start_time, end_time: end_time, can_travel: can_travel, travel_radus: travel_radius, will_travel: will_travel)
   end

   def existing_availability
     self.class.for(@user, @target_user)
   end

   def save
     #...
   end
end

These can be used as follows to either add to existing availability:

availability = UserOpenAvailability.for(user)
availability.add(...)
availability.save

or save a fresh set of availability:

availability = UserOpenAvailability.new(user, [])
availability.add(...)
availability.save

The problem I'm having is that the logic in the save method is extensive, and very similar in both classes.

When a user updates their availability, I need to add/update records in the AvailabilityWindow table.

  • any new availability needs to be added as a new record
  • any previous availability that has been deselected needs to be 'removed' by changing the state to removed
  • any changes to existing availability (e.g. end_time brought forward) requires the existing availability to be removed and a new, reduced time slot added.
  • any unchanged availability needs to be untouched

Implementing the above logic is fairly complicated. It needs to be wrapped in a transaction, it needs to compare existing availability with new availability to work out what needs to be saved/removed, it needs to save all the updates and report if any changes ended up being made.

At the moment it looks kind of like this:

def save
  @user.with_lock do
    ActiveRecord::Base.transaction do 
      added_availabilities.each do |availability|
        raise ActiveRecord::Rollback unless availability.save
      end
      removed_availabilities.each(&:remove!)

      @availability_changed = (added_avaialbilities + removed_availabilities).any?
      return true
    end
    false
  end
end

def availability_changed?
  !!@availability_changed
end

def added_availabilities
  @availabilities.select do |new_availability|
    existing_availabilities.none? do |existing|
       existing.same_availability?(new_availability)
    end
  end
end

def removed_availabilities
  existing_availabilities.select do |existing|
    @availabilities.none? do |new_availability|
       new_availability.same_availability?(existing)
    end
  end
end

This code is currently sitting in both classes and I'm having trouble refactoring it out. I don't think creating a base class that both my classes extend from really works because the #add methods have different arguments.

I could move the logic into a concern/module, but then there's going to be bidirectional dependencies as the concern expects there to be both a @availabilites array and existing_availabilities method. That seems like I'm just hiding the problem.

I can only think of adding a class such as AvailabilityWindowUpdater that takes a set of existing availability windows and a desired set of availability windows, and performs all the above save logic. But having a class called ..Updater is usually not a good sign.

Can anyone help with some suggestions?

Aucun commentaire:

Enregistrer un commentaire