HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Initializing a new user

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
usernewinitializing

Problem

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

Solution

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

Code Snippets

@membership.user.accounts.map(&:id)
@membership.user.accounts.pluck(:id)
Membership.find(member_id)
Membership.includes(user: :accounts).find(member_id)
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

Context

StackExchange Code Review Q#86912, answer score: 3

Revisions (0)

No revisions yet.