Refactor 2 iteration

[KELiON]

В этом PR опишу, как бы я порефакторил текущую версию приложения.

Классно, что вынес логику в service-объект – стало удобнее! По коду такие комменты:

• Всё ещё сложновато читать. Я бы это назвал преждевременной оптимизацией) Когда код разбит на методы, вроде бы для структуризации, но при этом в каждом методе по одной строчке и эти методы вызываются по 1 разу, из-за чего глазами приходится постоянно бегать по файлу (ниже приведу пример, как я бы это всё поменял)
• В использовании многих методов (attribute_color, attribute_price, experiment_params) я бы предложил использовать command-query separation принцип (https://ru.wikipedia.org/wiki/CQRS), когда у любого метода есть одна из двух функций: "команда", типа "сделай что-то" или "запрос", типа "дай мне что-то". Сейчас же получается смешение - имя как будто query, а логика – как будто command. То есть должно быть либо params = experiment_params, либо set_experiment_params внутри которого выставляем инстанс-переменную @params (у command в имени всегда есть глагол)
• https://github.com/Froz10/experiments-api/blob/master/app/services/api/experiment_service.rb#L11 и https://github.com/Froz10/experiments-api/blob/master/app/services/api/experiment_service.rb#L12 – тут 2 раза используется одно и тоже постфиксное условие query_to_db_valid?, внутри которого мы делаем запрос в базу. Из-за этого получается, что мы делаем лишний запрос – можно всё объединить в одно условие:

if query_to_db_valid?
  attribute_color
  attribute_price
end

• https://github.com/Froz10/experiments-api/blob/master/app/services/api/experiment_service.rb#L13 – тут мы вызываем метод experiment_params, где инициализируем хеш и записываем в переменную @params, а потом на следующей строчке делаем это ещё раз https://github.com/Froz10/experiments-api/blob/master/app/services/api/experiment_service.rb#L14 – получается можно 13 строчку удалить и ничего не изменится
• В методе call присваиваются instance-переменные, которые в итоге не используются (@experiment, @experiments) – лучше в таком случае использовать локальные переменные (а @experiment вообще можно в переменную не записывать)

В итоге сервис получится вот таким:

module Api
  class ExperimentService < ApplicationService
    def initialize(token) # rubocop:disable Lint/MissingSuper: Call super to initialize state of the parent class.
      @token = token
    end

    def call
      Experiment.colors
      Experiment.prices
      if !Experiment.exists?(token: @token)
        button_color = Experiment.color 
        price = Experiment.price
      end
      Experiment.create(token: @token, button_color: button_color, price: price)
      Experiment.select(:button_color, :price).where(token: @token)
    end
  end
end

После такого рефакторинга видно, что в начале метода снова нарушается CQRS (Experiment.colors, Experiment.prices). Для этого надо ещё порефакторить модель Experiment (тут комменты, которые я в прошлый раз что-то не отметил):

• Методы colors и prices на самом деле нам нужны только для того, чтобы инициализировать переменные @colors и @prices. Получается, что мы можем вместо явного вызова этих методов в методах color и price вызывать методы, а не инстанс-переменные. Вот так:

  def self.color
    colors.distribution
  end

  def self.price
    prices.distribution
  end

• После такого изменения первые 2 строки в методе call можем вообще убрать и всё будет так же работать.
• Едем дальше. Сейчас мы в предпоследней строчке каждый раз пытаемся создать experiment, даже если он уже есть в базе (в таком случае он просто не создаётся из-за валидации). Но зачем нам это делать, если мы до этого и так делаем проверку на наличие эксперимента? Вместо этого можно получить эксперимент по его токену и в случае неудачи создать. Для этого можно использовать метод find_or_create_by. Получается вообще весь сервис – это один вызов метода с блоком из 2 строк внутри:

def call
  Experiment.find_or_create_by(token: @token) do |experiment|
    experiment.button_color = Experiment.color
    experiment.price = Experiment.price
  end
end

• Мы реквайрим файл distribution из lib, а внутри этого файла декларируем 2 класса – Color и Price. Это можно назвать антипаттерном в rails, т.к. по стандартным соглашениям по имени класса должно быть понятно, где он лежит. Тут есть 2 варианта:
• Разбить на 2 файла (color.rb и price.rb), либо, например, добавить namespace, чтобы классы назывались Distribution::Color и Distribution::Price. Мне лично больше нравится второй вариант:

class Distribution
  class Color
    ...
  end

  class Price
    ...
  end
end

• Далее, можно убрать зависимость модели Experiment от Distribution – пусть они связываются только через наш сервис-объект. Для этого уберем метод color/colors и price/prices из модели, а сервис изменим следующим образом:

module Api
  class ExperimentService < ApplicationService
    def initialize(token) # rubocop:disable Lint/MissingSuper: Call super to initialize state of the parent class.
      @token = token
      @color_distribution = Distribution::Color.new
      @price_distribution = Distribution::Price.new
    end

    def call
      Experiment.find_or_create_by(token: @token) do |experiment|
        experiment.button_color = color_distribution.distribution
        experiment.price = price_distribution.distribution
      end
    end
  end
end

По названию метода distribution непонятно, что он делает. Опять же, либо мы делаем command "сделай что-то" (в таком случае будет distrubute), либо "дай мне что-то", например next_value. Мне наверное больше нравится второй вариант – переименуем метод distribution в next_value

Теперь перейдём к контроллеру. Тут можно немного упростить логику метода index. Тут снова нарушение cqrs – у нас есть метод device_header, который назван как query, а выполняет функцию command (т.к. выставляет значение instance-переменной). Вместо этого я бы сделал его как query (при этом добавив memoization и убрав лишние "сущности" и назвав instance переменную так же, как и метод – не token, а device_token):

def device_header
  @device_header ||= request.headers['Device-Token']
end

А теперь можно добавить стандартный подход к аутентификации в rails и сделать её в before_action:

before_action :validate_header!

def validate_header!
  render(json: {}, status: :forbidden) unless device_header
end

А метод index, тем самым разгрузим и оставим только основнвую логику:

def index
  @experiments = Api::ExperimentService.call(device_header)
end

Кстати, тут у нас немного путанница: на самом деле пользователь может попасть только в один эксперимент (именно поэтому я в сервисе использовал не where, который возвращает коллекцию, а find_by, который возвращает конкретную сущность). А в названии у нас множественное число – что не верно. Поэтому переименуем)