In your example, the
group parameter to
save_user is acting as a Control Couple. The
group parameter is only used if it evaluates to a true-ish value. Instead of expecting either an instance of
nil and introspecting on the parameter, you can split the method into two, and only invoke the group-related behavior if a group is present to begin with.
Here’s a first pass at refactoring this action:
flash[:attacher] = attacher(@user, group)
failed_signup('Group Not Found')
failed_signup('User Validation Error')
@group ||= @group ||= Group.find_by(group_code: group_code)
Here’s a summary of what I did:
- I changed the decision of which branch to take very early on, to avoid repeating the decision later.
- I created a base case signup method (
signup_without_group) for when no group is present, which is totally unaware of groups.
- I created a new method for the case when a group is present (
signup_with_group) and composed the base case to avoid duplication.
I think the only behavioral differences between your version and mine are:
- Mine will take the non-group path if
params['Group Code'] is an empty string. I changed it for readability, but I doubt it changes the actual behavior.
- Mine won’t assign
flash[:attacher] if there isn’t a group code. I took a shot in the dark and guessed that
The refactored version no longer uses
nil at all. The cyclomatic complexity of each method is also reduced, and I’m guessing you can remove a conditional from
attacher as well. Let me know if you have any questions about my changes.