Question on the weekly iteration for avoiding nil

Ruby on Rails
1

I have this method

def create
    @user = user_from_params
    if params['Group Code'] && group.nil?
        failed_signup('Group Not Found')
    elsif save_user(@user,group)
        sign_in_with_redirect(@user)
    else
      failed_signup('User Validation Error')
    end
  end

def save_user(user,group = nil)
    if user.save
      user.register
      user.connect(user.password)
      flash[:attacher] = attacher(user,group)
      user.follow(group) if group
      return true
    else
      return false
    end
  end

 def group
    @group ||= Group.find_by(group_code: params['Group Code'])
  end

If there is a group object available I pass in that otherwise I pass in a nil and do operations based on that. I saw half of that video but then felt lost.

Can anyone help me how I can avoid using nil here. @benorenstein

2

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 Group or 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:

def create
  if group_code.present?
    signup_with_group
  else
    signup_without_group
  end
end

def signup_with_group
  if group.present?
    signup_without_group
    flash[:attacher] = attacher(@user, group)
    @user.follow(group)
  else
    failed_signup('Group Not Found')
  end
end

def signup_without_group
  if save_user(@user)
    sign_in_with_redirect(@user)
  else
    failed_signup('User Validation Error')
  end
end

def save_user(user)
  if user.save
    user.register
    user.connect(user.password)
    return true
  else
    return false
  end
end

def group_code
  params['Group Code']
end

def group
  @group ||= @group ||= Group.find_by(group_code: group_code)
end

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 attacher returned nil when group was nil.

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.

3

Cool. Thanks.