1
\$\begingroup\$

I have this update method which decides whether to update or recreate(same as update but resets the approval_status and create_date) of the building_approval object. I have the on_progress? method on building_approval class to check whether it needs to be updated or recreated so no flags are being passed from the view, the problem is I use this on_progress? method on view to change some minor UI in the form, so it is being called twice (once in the view for UI and once in the update method in the controller for deciding whether to update or recreate). I have considered passing to_update param from view to the controller so I wouldn't have to run on_progress? method twice but it makes my code look ugly

tldr:

def update
    return render :edit unless @building_approval.update(param_building_approvals)

    if(@building_approval.on_progress?)
      @building_approval.reapply
      @building_approval.send_reapply_mail(current_member)
      notice = @building_approval.has_contract? ? 'messages.warning.building_approval_has_contract' : 'messages.info.approval_reapplied'
    else
      @building_approval.after_save_by_apply()
      @building_approval.send_apply_mail(current_member)
      notice = 'messages.info.updated'
    end
    redirect_to prospective_customer_building_approval_path(@customer.id, @building_approval.id),
                notice: t(notice, :name => BuildingApproval.model_name.human)
end

or

def update
    return render :edit unless @building_approval.update(param_building_approvals)

    if(params[:to_update] == "false")
      @building_approval.reapply
      @building_approval.send_reapply_mail(current_member)
      notice = @building_approval.has_contract? ? 'messages.warning.building_approval_has_contract' : 'messages.info.approval_reapplied'
    else
      @building_approval.after_save_by_apply()
      @building_approval.send_apply_mail(current_member)
      notice = 'messages.info.updated'
    end
    redirect_to prospective_customer_building_approval_path(@customer.id, @building_approval.id),
                notice: t(notice, :name => BuildingApproval.model_name.human)
end

which one is better ? the first one's cons is it uses on_progress? method twice(in view and in controller) , the cons in the second one is it uses on_progress? once but I need to pass hidden_field to_update params to controller and make ugly if statements

\$\endgroup\$
  • \$\begingroup\$ Any reason not to call that method twice? is it time consuming? you could save the result on a variable if you don't want to call it twice but if it makes sense to call the method twice then I don't see the problem. Adding a hidden field looks like a hack for a problem you don't actually have. \$\endgroup\$ – arieljuod May 9 at 3:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.