2

Здравствуйте. Столкнулся с незнакомой мне ситуацией, в разработке новичок, нуждаюсь в совете.

Имеется контроллер с экшеном 'update'

def update
    @arrival = find_arrival
    @details = @arrival.arrival_details

    if check_conditions(@arrival)
      flash[:notice] = 'Документ прихода отредактирован'
    else
      flash[:error] = 'Возникла ошибка. Проверьте правильность заполнения формы'
    end
    redirect_to edit_admin_arrival_path(@arrival)
  end

а так же следующие private-методы:

def check_conditions(arrival)
  new_status = arrival_params[:status]

  case @arrival.status
  when 'draft'
    return unless check_dependencies
    recalculate_balance if new_status == 'accrued'
    @arrival.update(arrival_params)
  when 'canceled'
    return unless new_status == 'draft'
    @arrival.update(status: arrival_params[:status])
  when 'accrued'
    return if new_status == 'draft'
    recalculate_balance if new_status == 'canceled'
    @arrival.update(new_status)
  end
end

def recalculate_balance
  puts '[PRY] recalculated'
end

def check_dependencies
  Provider.exists?(arrival_params[:provider_id]) &&
  Warehouse.exists?(arrival_params[:warehouse_id])
end

Интересует следующее - стоит ли выносить данное условие в отдельный класс или какой-нибудь там Service Object? Не думаю, что такому огромному условию место в контроллере. Что можете посоветовать?

1 ответ 1

1

Сам update вполне себе нормальный.

Разве что выкинул бы оттуда @details, т. к. он всё равно доступен в @arrival. Но это мелочь. Если так удобно, пользуйтесь.
Ещё я бы переименовал ассоциацию :arrival_details в просто :details, чтобы снизить избыточность и необходимость в @details.

Но секция private перегружена, да. У вас туда утекла бизнес-логика. А контроллер не должен содержать бизнес-логики вообще, по-хорошему. Он должен лишь предоставлять к ней доступ.

Куда вы это вынесете, зависит от кучи факторов. В разных условиях решениями могут быть:

  • В модель Arrival (по @arrival, так же она называется?)
  • В Service Object
    • если модель и так слишком толстая
    • если действие затрагивает и другие модели тоже
  • В модель с обобщением части логики наружу для повторного использования
    • если используемая в модели логика может быть хорошо обобщена
  • ...?

Скажем, что вижу в вашем конкретном случае я:

  • В check_conditions у вас вышла самодельная машина состояний. Это достаточно популярная штука, у неё есть реализации вроде AASM, позволяющие оформить код компактнее и читаемее.
  • check_conditions не только проверяет условия, но и обновление выполняет. И вообще он настолько часто ссылается на @arrival, что возникает здравый вопрос: а не должно ли это быть его методом?
    • Статический анализатор Reek выполняет анализ "не ссылается ли метод на что-то больше, чем на self" и выводит предупреждение, если что-то подходит. Но не относитесь к Reek слишком серьёзно, думайте своей головой в первую очередь.
  • check_dependencies должен скорее быть в модели, чем-то вроде:

    validates :provider, :warehouse, presence: true
    

    Да, тупо валидацией! Но стоит её дополнительно подкрепить внешним ключом с NOT NULL на уровне БД, чтобы никакие баги в приложении не могли привести к существованию Arrival с несуществующими provider_id и warehouse_id.

1
  • Благодарю Вас за подробный и объемный ответ 4 апр 2017 в 6:28

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge you have read our privacy policy.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.