3
\$\begingroup\$

I have a method in my rails project that initializes a new user by copying some properties of another. The method should also retain entered parameters if saving the new user is rejected. Rubocop complains that Assignment Branch Condition Size for this method is 42.02/15. How can I improve this?

def setup_user_clone
    member_id = (params[:id] || params[:member_id])
    @membership = Membership.find(member_id)
    accounts = @membership.user.accounts.map(&:id) | [@account.id]
    cloned_user = User.new @membership.user.attributes.slice('permissions')
    cloned_user.game_plan_admin = @membership.user.game_plan_admin
    if user_edit_params
      cloned_user.email = invite_email
      cloned_user.permissions = user_edit_params[:permissions]
      cloned_user.accounts = Account.find(invite_app_access.map(&:to_i))
      cloned_user.level = user_edit_params[:user_level]
    end
    profile = SettingsProfile.new @account, cloned_user
    accounts |= [*cloned_user.accounts.map(&:id).to_set]
    @user_edit = UserEdit.new current_user, profile, @membership.user.admin?, accounts, @membership.user.shared_credentials.map(&:id)
    @method = :post
  end
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Here's my notes

  • First don't use #map and use #pluck instead (for database calls), all over your code.

    bad:

    @membership.user.accounts.map(&:id)
    

    better:

    @membership.user.accounts.pluck(:id)
    
  • Use eager loading, to reduce queries

    Bad:

    Membership.find(member_id)
    

    Better:

    Membership.includes(user: :accounts).find(member_id)
    

    Because you are using users and accounts in the loops after.

  • This whole block could be reduced to a single statement

    if user_edit_params
      cloned_user.email = invite_email
      cloned_user.permissions = user_edit_params[:permissions]
      cloned_user.accounts = Account.find(invite_app_access.map(&:to_i))
      cloned_user.level = user_edit_params[:user_level]
    end
    

    To

    if user_edit_params
      cloned_user.assign_attributes(
        email: invite_email
        permissions: user_edit_params[:permissions]
        accounts: Account.find(invite_app_access.map(&:to_i))
        level: user_edit_params[:user_level]
      )
    end
    
  • No idea what this does

    @membership.user.attributes.slice('permissions')
    

    but wouldn't this do the same ?

    @membership.user.permissions
    
  • Law of Demeter

  • And basically just group common stuff into single methods and call the methods inside this method, isntead of having one huge method like this

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.