Ruby static method refactoring

static_methods.rb

# original implementation refactored after extract method (but still using static methods)
# criticism for this approach is: 
# - dependencies are passed between method calls, rather than referenced via instance methods
# - cannot declare static methods as private (this is not valid)
# - there are multiple responsibilities (I'd argue this is not valid)
# - can be difficult to test


class SyncToAnalyticsService
  ConnectionFailure = Class.new(StandardError)

  def self.perform(data)
    data                = data.symbolize_keys
    account             = Account.find(data[:account_id])
    analytics_client    = Analytics::Client.new(CC.config[:analytics_api_key])

    sync_users(analytics_client, account)
  end

  def self.sync_users(analytics_client, account)
    account_attributes  = account_attributes(account)

    account.users.each do |user|
      sync_user(analytics_client, account_attributes, user)
    end
  end

  def self.sync_user(analytics_client, account_attributes, user)
    create_or_update_user(analytics_client, account_attributes, user)
  rescue SocketError => ex
    raise ConnectionFailure.new(ex.message)
  end

  def self.create_or_update_user(analytics_client, account_attributes, user)
    attributes = user_attributes(user, account).merge(account_attributes)
    analytics_client.create_or_update(attributes)
  end

  def self.user_attributes(user, account)
    {
      id:             user.id,
      email:          user.email,
      account_admin:  account.administered_by?(user)
    }
  end

  def self.account_attributes(account)
    {
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }
  end
end

# alternative suggestion pushing all dependencies to the top level, and passing them in rather than encapsulating them as part of the class
# this in turn helped to clarify the single responsibility of this class - with an updated name to match (and since this functionality is related specifically to the analytics, inclusion under the `Analytics` module is more communicative)
# previous distinction between user and account attributes is actually not necessary, and is artificial from the perspective of the analytics client
# because the analytics client, and the account, are both passed in as dependencies, testing is much easier to control (allowing the use of mocks, stubs or fakes)
# previous implementation passed in data for the sole sake of query for the account. assume the caller can do that, and pass in only what the class requires (an account, and an analytics client)

Analytics::SyncUsersForAccount.perform(Account.find(data[:account_id], Analytics::Client.new(CC.config[:analytics_api_key])))


class Analytics::SyncUsersForAccount
  ConnectionFailure = Class.new(StandardError)
  
  def self.perform(account, analytics_client)
    account.users.each do |user|
      begin
        analytics_client.create_or_update(attributes_from(user, account))
      rescue SocketError => e
        raise ConnectionFailure.new(e.message)
      end
    end
  end
  
  def self.attributes_from(user, account)
    {
      id:                 user.id,
      email:              user.email,
      account_admin:      account.administered_by?(user),
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }
  end
  
  private_class_method :attributes_from
end