Rubocop complains about Metrics/AbcSize-CodePudding

So I'm trying to take the right path here and understand how to solve this cop, this looks like a small piece of code IMO, why is it complaining?

moving the nested if-else doesn't change anything, any suggestions on how to solve this cop?

 class WebPush::Register

  include Interactor

  # rubocop:disable Metrics/AbcSize
  def call
    user = Learner.find_by(id: context.user_id)

    # return if existing
    if user.web_push_subscription
      context.subscription = user.web_push_subscription
    else
      subscription = WebPushSubscription.new(
        endpoint:   context.push_params[:endpoint],
        auth_key:   context.push_params[:keys][:auth],
        p256dh_key: context.push_params[:keys][:p256dh],
        learner:    user
      )

      if subscription.save
        context.subscription = subscription
      else
        context.error = subscription.errors.full_messages
        context.fail!
      end
    end
  end
  # rubocop:enable Metrics/AbcSize

end

CodePudding user response：

First, you need to understand how ABC is calculated. Nested conditionals do not affect ABC. RuboCop's warning output shows you the calculation results:

Assignment Branch Condition size for call is too high. [<5, 28, 4> 28.72/17]

The <5, 28, 4> is your <Assignments, Branches, Conditionals>, as described in this article.

The overall score is calculated like this: sqr(5^2 28^2 4^2) = 28.72

The default maximum score for the cop is 17.

I've annotated your code below with the ABC scores for each line. Take note that every time you reference context, you add a B-point. This is because context is not a local variable to call, so the ABC metric assumes it's a method call every time.

def call
  user = Learner.find_by(id: context.user_id) # <1, 3, 0>

  if user.web_push_subscription # <0, 1, 1>
    context.subscription = user.web_push_subscription # <1, 3, 0>
  else # <0, 0, 1>
    # this constructor call is the most expensive part
    subscription = WebPushSubscription.new( # <1, 1, 0>
      endpoint:   context.push_params[:endpoint], # <0, 3, 0>
      auth_key:   context.push_params[:keys][:auth], # <0, 4, 0>
      p256dh_key: context.push_params[:keys][:p256dh], # <0, 4, 0>
      learner:    user # <0, 0, 0>
    )

    if subscription.save # <0, 1, 1>
      context.subscription = subscription # <1, 2, 0>
    else # <0, 0, 1>
      context.error = subscription.errors.full_messages # <1, 4, 0>
      context.fail! # <0, 2, 0>
    end
  end
end

if you set the cop option: CountRepeatedAttributes: false (which I recommend), it will bring your score down to 19.1.

In lieu of that, you can bring your score down by extracting the creation of WebPushExtraction into its own method like so:

  def call
    user = Learner.find_by(id: context.user_id)

    if user.web_push_subscription
      context.subscription = user.web_push_subscription
    else
      create_subscription(user)
    end
  end

  private

  def create_subscription(user)
    push_params = context.push_params
    subscription = WebPushSubscription.new(
      endpoint:   push_params[:endpoint],
      auth_key:   push_params.dig(:key, :auth),
      p256dh_key: push_params.dig(:key, :p256dh),
      learner:    user
    )

    if subscription.save
      context.subscription = subscription
    else
      context.error = subscription.errors.full_messages
      context.fail!
    end
  end

This will split the score up between the two methods. Note some additional ABC-saving strategies in create_subscription, like assigning push_params to a variable, and using dig for the nested hash accessors. Final score for create_subscription is between 12 and 16 depending on the cop options you use, and call is between 6 and 8.

Generally all that's required to bring down ABC score is to refactor to smaller methods.

CodePudding user response：

ABC stands for Assignment, Branch, Condition. ABC = sqrt(A^2 B^2 C^2)

Let's count them.

  def call
    user = Learner.find_by(id: context.user_id)  // assignment, branches

    # return if existing
    if user.web_push_subscription // branch, condition
      context.subscription = user.web_push_subscription  // assignment, branches
    else  // condition
      subscription = WebPushSubscription.new(  // assignment, branch
        endpoint:   context.push_params[:endpoint],  // branches
        auth_key:   context.push_params[:keys][:auth],  // branches
        p256dh_key: context.push_params[:keys][:p256dh],  // branches
        learner:    user
      )

      if subscription.save // branch, condition
        context.subscription = subscription  // assignment
      else  // condition
        context.error = subscription.errors.full_messages  // assignment, branches
        context.fail!  // branch
      end
    end
  end

A = 5
B = 17 (at least)
C = 4

This gives 18.2. By default, 17 is the maximum Rubcop considers satisfactory.

We can handle this with a few simple extract methods.

  private def user
    @user ||= Learner.find_by(id: context.user_id)
  end

  private def create_web_push_subscription_from_context
    subscription = WebPushSubscription.new(
      endpoint:   context.push_params[:endpoint],
      auth_key:   context.push_params[:keys][:auth],
      p256dh_key: context.push_params[:keys][:p256dh],
      learner:    user
    )

    if subscription.save
      context.subscription = subscription
    else
      context.error = subscription.errors.full_messages
      context.fail!
    end
  end

  def call
    if user.web_push_subscription
      context.subscription = user.web_push_subscription
    else
      create_web_push_subscription_from_context
    end
  end

After this refactoring the code is simpler, each method does one thing. That's what the ABC metric encourages.