patternrubyrailsMinor
Initializing a new user
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
endSolution
Here's my notes
-
First don't use
bad:
better:
-
Use eager loading, to reduce queries
Bad:
Better:
Because you are using users and accounts in the loops after.
-
This whole block could be reduced to a single statement
To
-
No idea what this does
but wouldn't this do the same ?
-
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
-
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]
endTo
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]
endContext
StackExchange Code Review Q#86912, answer score: 3
Revisions (0)
No revisions yet.