10
\$\begingroup\$

Is it bad to do return render? I am doing this to NOT also allow a redirect.

def create
    @patient = Patient.new(params[:patient])
    if  !@patient.save
      @patient.user.password = ''
      @patient.user.password_confirmation = ''
      return render is_admin? ? 'new_admin' : 'new'
    else
      #Fire off an e-mail
      PatientMailer.welcome_email(@patient).deliver
    end

    if current_user == @patient
      sign_in @patient
    else
      redirect_to patients_path
    end
  end
\$\endgroup\$
8
\$\begingroup\$

Writing it like this makes it look the render returns a meaningful value that is then returned by create and used by the code that calls create, which is not the case. So instead I would write:

render is_admin? ? 'new_admin' : 'new'
return

This makes it clear that render is solely used for its side effects and create does not return a value (other than nil).

\$\endgroup\$
1
\$\begingroup\$

There are two sensible ways to write that function.

The first way, with the early return, treats the special case more like an error condition:

def create
  @patient = Patient.new(params[:patient])
  if  !@patient.save
    @patient.user.password = ''
    @patient.user.password_confirmation = ''
    render is_admin? ? 'new_admin' : 'new'
    return
  end

  # Fire off an e-mail
  PatientMailer.welcome_email(@patient).deliver

  if current_user == @patient
    sign_in @patient
  else
    redirect_to patients_path
  end
end

The other way outlines all the possible outcomes:

def create
  @patient = Patient.new(params[:patient])
  if  !@patient.save
    @patient.user.password = ''
    @patient.user.password_confirmation = ''
    render is_admin? ? 'new_admin' : 'new'
  else
    # Fire off an e-mail
    PatientMailer.welcome_email(@patient).deliver
    if current_user == @patient
      sign_in @patient
    else
      redirect_to patients_path
    end
  end
end

I personally have a slight preference for the first way, since the failure to save is more like an error. Your original code, which is a hybrid of the two, feels awkward.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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