CODEWITHSUNDEEP
ruby-on-rails
GMorse19
GMorse19

Reputation: 61

Is there a better way to implement my business logic in Rails?

I have a WorkSpace model that has_many Reviews. The Reviews model has a list of attributes, each with their own average rating calculated. I'd like to allow users to find WorkSpaces with the highest rated attribute of their choice.

I have been able to achieve this using scopes and keeping the logic inside the WorkSpace model.

This is my first attempt to do any kind of logic in Rails and I'm wondering if this logic would be better in the controller instead. It works well, but the info it generates is attached to every WorkSpace and I'm thinking this a little redundant as the only time a user would need to access this data is when they're using the filtering system not every time they click on a WorkSpace.

WorkSpace Model(The half with the logic I'm discussing)

class WorkSpace < ApplicationRecord
  belongs_to :user
  has_many :reviews, dependent: :delete_all

  scope :max_rating, ->(rating) { joins(:reviews)
    .group('work_spaces.id')
    .order('AVG(reviews.rating) desc')
    .having('AVG(reviews.rating) > ?', rating) if rating }

  scope :max_bathroom, ->(bathroom) { joins(:reviews)
    .group('work_spaces.id')
    .order('AVG(reviews.bathroom) desc')
    .having('AVG(reviews.bathroom) > ?', bathroom) if bathroom }

  scope :max_noise, ->(noise) { joins(:reviews)
    .group('work_spaces.id')
    .order('AVG(reviews.noise) desc')
    .having('AVG(reviews.noise) > ?', noise) if noise }

  scope :max_wifi, ->(wifi) { joins(:reviews)
    .group('work_spaces.id')
    .order('AVG(reviews.wifi) desc')
    .having('AVG(reviews.wifi) > ?', wifi) if wifi }

  scope :max_seating, ->(seating) { joins(:reviews)
    .group('work_spaces.id')
    .order('AVG(reviews.seating) desc')
    .having('AVG(reviews.seating) > ?', seating) if seating }

  def top_avg_rating
    WorkSpace.max_rating(2).limit(5)
  end

  def top_avg_bathroom
    WorkSpace.max_bathroom(2).limit(5)
  end

  def top_avg_noise
    WorkSpace.max_noise(2).limit(5)
  end

  def top_avg_wifi
    WorkSpace.max_wifi(2).limit(5)
  end

  def top_avg_seating
    WorkSpace.max_seating(2).limit(5)
  end

Review Model

# frozen_string_literal: true

class Review < ApplicationRecord
  belongs_to :work_space
  belongs_to :user
end

WorkSpace Serializer

class WorkSpaceSerializer < ActiveModel::Serializer
  attributes :id,
             :place_id,
             :lat,
             :lng,
             :name,
             :address,
             :photo,
             :reviews,
             :user,
             :count_reviews,
             :avg_rating,
             :avg_noise,
             :avg_wifi,
             :avg_bathroom,
             :avg_food,
             :avg_coffee,
             :avg_seating,
             :avg_outlet,
             :bool_outlet,
             :bool_seating,
             :bool_coffee,
             :bool_food,
             :bool_bathroom,
             :bool_wifi,
             :top_avg_rating,
             :top_avg_bathroom,
             :top_avg_noise,
             :top_avg_wifi,
             :top_avg_seating
  has_one :user
  has_many :reviews
end

Could this or should this type of logic be done in the WorkSpace controller instead? And only accessed when an Axios GET request is made? Or... am I way off base and I should just give up now?

Update

So far I was able to Dry up the scopes with this bit of code.

scope :by_average_for, ->(column) {
    joins(:reviews)
      .group('work_spaces.id')
      .order("AVG(reviews.#{column}) desc")
      .having("AVG(reviews.#{column}) > 4", column) if column
  }

Thank you, https://stackoverflow.com/users/14660/schwern.

I'm working on implementing the class method next. Can't seem to get that working...

Upvotes: 3

Views: 332

Answers (1)

Schwern
Schwern

Reputation: 165456

Generally, controllers are "skinny". They should only contain logic which connects models to views and that's it. Database logic goes into models. Display logic goes into decorators. Talking to APIs and services goes into service objects.

Your logic can be DRY'd up. Your scopes can be turned into a single scope that takes an argument.

  scope :by_average_for, ->(column) {
    joins(:reviews)
      .group('work_spaces.id')
      .order("AVG(reviews.#{column})", :desc)
  }

Similarly, a single class method can replace all the top_foo methods. These can be made more flexible by taking arguments with defaults.

  class << self
    def top_averages_for(column, greater_than: 2, limit: 5)
      by_average_for(column)
        .having("AVG(reviews.#{column}) > ?", greater_than)
        .limit(5)
    end
  end

If you need individual instance methods for serialization, they can also be DRY'd up by defining the methods dynamically using define_method.

TOP_AVG_COLUMNS = [
  :rating,
  :seating,
  ...
].freeze

TOP_AVG_COLUMNS.each do |column|
  define_method(:"top_avg_#{column}") do
    top_averages_for(column)
  end
end

If these are only for serialization, they may be more appropriate in a decorator.

You can use TOP_AVG_COLUMNS and similar constants to DRY up your list of attributes to serialize.

# In WorkSpace
TOP_AVG_ATTRIBUTES = TOP_AVG_COLUMNS.map { |col| 
  :"top_avg_#{col}"
}.freeze

# In WorkSpaceSerializer
ATTRIBUTE_COLUMNS = [
  :id,
  :place_id,
  :lat,
  :lng,
  :name,
  :address,
  :photo,
  :reviews,
  :user
].freeze

attributes(ATTRIBUTE_COLUMNS + WorkSpace::TOP_AVG_ATTRIBUTES)

If this business logic got more complicated your model might get fat. Then you'd move it into a WorkSpaceManager, an ActiveModel::Model, whose purpose is to perform business logic on WorkSpaces.

Upvotes: 2

Related Questions