Hello. Faced with a situation unfamiliar to me, in the development of a novice, I need advice.

There is a controller with the action '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

as well as the following private methods:

 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

Interests the following - whether it is worth making this condition in a separate class or any Service Object? I do not think that such a huge condition is a place in the controller. What can you advise?

    1 answer 1

    The update itself is quite normal.

    Unless would have thrown out @details , @details it is still available in @arrival . But it is a trifle. If so convenient, use.
    I would also rename the association :arrival_details to simple :details , to reduce redundancy and the need for @details .

    But the private section is overloaded, yes. You have business logic leaked there. A controller should not contain business logic at all, in an amicable way. It should only provide access to it.

    Where you take it depends on a bunch of factors. In different conditions, the solutions can be:

    • In the Arrival model (by @arrival , is it also called?)
    • In Service Object
      • if the model is already too thick
      • if the action affects other models too
    • In model with generalization of part of logic outside for reuse
      • if the logic used in the model can be well generalized
    • ...?

    Let's say what I see in your particular case:

    • In check_conditions , you have a homemade state machine . This is quite a popular thing, it has implementations like AASM that allow you to format the code more compactly and more readable.
    • check_conditions not only checks the conditions, but also performs the update. And in general, he so often refers to @arrival , that a sensible question arises: should this not be his method?
      • The static Reek analyzer performs an analysis of “whether the method refers to something more than self ” and displays a warning if something is appropriate. But do not take Reek too seriously, think with your head first.

    • check_dependencies should rather be in a model , something like:

       validates :provider, :warehouse, presence: true

      Yes, stupidly validation! But it is worth it to additionally support it with a foreign key with NOT NULL at the database level, so that no bugs in the application could lead to the existence of Arrival with non-existent provider_id and warehouse_id .

    • Thank you for the detailed and lengthy response - Vadim Ryazantsev