ruby-on-railscontroller

How to refactor controller method with several different failure messages

I have a controller "destroy" method that checks for various dependent objects and fails with corresponding alert messages. The deletion is also prevented on the database side with has_many :invoices, dependent: :restrict_with_error but that fails silently for the user.

Rubocop thinks "Assignment Branch Condition size is too high" and I would like to refactor this also because it looks kind of repetitive. However, I struggle with the returns. When I move checks into a private function the return resturns from the function but I also need it to end the "destroy" method. I kind of need a double return.

def destroy
    unless Current.user.default_entity.owner == Current.user ||
           Current.user.default_entity.user_privileged?('manage_contacts')

      redirect_back(fallback_location: contacts_path, alert: t('errors.no_access_rights')) and return
    end

    unless @contact.invoices.count.zero?
      redirect_back(fallback_location: contacts_path,
                    alert: t('errors.cant_delete_contact_with_invoices')) and return
    end

    unless @contact.offers.count.zero?
      redirect_back(fallback_location: contacts_path,
                    alert: t('errors.cant_delete_contact_with_offers')) and return
    end

    @contact.destroy

    redirect_to contacts_path, status: :see_other
  end

I have similar methods in other controllers throughout my application. Is there maybe a pattern to refactoring such types of methods?

Solution

  • Update: The accepted answer does much better job than mine. I didn't properly consider what the code does, only did some moving around. I still think it might be valuable too see some steps you can take, but some of my desicions were not good (moving authentication code to model), and some are only part there (validations). So take this more as a brainstorm, not a solution.

    There's always (especially with ruby) many ways to structure code. Here's my approach and what I consider to be improvements, but your opinions and preferences may be different. Also - you can decide to ignore Rubocop. This warning doesn't say it's wrong code, just that maybe it's possible to write it better.

    Original code

    def destroy
  unless Current.user.default_entity.owner == Current.user ||
          Current.user.default_entity.user_privileged?('manage_contacts')

    redirect_back(fallback_location: contacts_path, alert: t('errors.no_access_rights')) and return
  end

  unless @contact.invoices.count.zero?
    redirect_back(fallback_location: contacts_path,
                  alert: t('errors.cant_delete_contact_with_invoices')) and return
  end

  unless @contact.offers.count.zero?
    redirect_back(fallback_location: contacts_path,
                  alert: t('errors.cant_delete_contact_with_offers')) and return
  end

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

    Variation 1

    Let's get rid of duplication - we can set alert when there's a problem, and if we set it, we return and redirect.

    def destroy
  alert = nil
  unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    alert = t('errors.no_access_rights')
  end

  unless @contact.invoices.count.zero?
    alert = t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    alert = t('errors.cant_delete_contact_with_offers')
  end

  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

    We didn't really make the code shorter or less branched, but we got some insight - there's logic for finding possible problems, and there are two ways this method end - redirect_back with error, or destruction of contact.

    Variation 2

    Let's move setting alert to separate private method, making action cleaner

    def destroy
  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

private

def alert
  unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    return t('errors.no_access_rights')
  end

  unless @contact.invoices.count.zero?
    return t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    return t('errors.cant_delete_contact_with_offers')
  end
end

    The naming here is possibly not great, but it was just fast brainstorm.

    This again enhanced the readability of the core - destroy method, and makes it simpler to move the details around, which leads to:

    Variation 3

    Why are we checking for whether model can be deleted? Controller doesn't need to know that, this knowledge possibly lies in model. Also, ActiveRecord models have a great native way of managing validations and errors, so that will help us make more standard Rails endpoint. Here for reference - Rails docs and Rails guide

    # app/models/contact.rb
class Contact < ApplicationRecord
  ...

  before_destroy :validate_destruction, prepend: true


  private

  def validate_destruction
    unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
      errors.add t('errors.no_access_rights')
      return false
    end

    unless invoices.count.zero?
      errors.add t('errors.cant_delete_contact_with_invoices')
      return false
    end

    unless offers.count.zero?
      errors.add t('errors.cant_delete_contact_with_offers')
      return false
    end
  end
end

# app/controllers/contracts_controller.rb

def destroy
  if  @contact.destroy
    redirect_to contacts_path, status: :see_other
  else
    redirect_back(fallback_location: contacts_path, alert:) if alert
  end
end

    Nice and clean.

    I didn't really test all the code, so I don't guarantee it works exactly this way, I'm just trying to point direction

    Some general thoughts: