Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am writing this code in the controller. I want to know is this the right way to write it or not. Because I don't want to write If else conditions and then either redirect or render the pages.

Here is my sample code :

def create
    @stud = Student.new params[:student]
    @stud.save!
    redirect_to students_path(@stud)
  rescue
    render 'new'
end
share|improve this question

2 Answers

I know you wanted to avoid if / else, but I think i would probably write it like this:

def create
  @stud = Student.new params[:student]

  if @stud.save
    redirect_to @stud
  else
    render :action => :new
  end
end

Which is the same amount of code but has two advantages:

  • it is more readable by a human. Rescue is not a human concept in logic, whereas if / else is clearly understood.

  • when you rescue here, you rescue from anything. if @stud.save is specific, if that fails it won't throw an exception, but will return false. So you are responding to the appropriate condition.

One other note: if you want to stick with the rescue pattern, you could shorten the function by a single line by writing

@stud = Student.create! params[:student]

which is the same as writing:

@stud = Student.new params[:student]
@stud.save!

ian.

share|improve this answer

save actually returns false only if before filters or validation cancel the operation. It throws database exceptions. Therefore it is, in my opinion, a good practice. I suggest not to rescue an exception that you do not really deal with. It may make the user crazy if the database index is broken but the application constantly says there is a validation error.

You can rescue ActiveRecord::RecordInvalid if you 'really' do not want to use if statements. But actually that is the only thing that save does.

You can use ActiveController::Responder. According to the Rails Guide it does the same thing as if-else statement (and looks beautiful):

def create
  @stud = Student.new(params[:student])
  flash[:notice] = 'Student was successfully created.' if @stud.save
  respond_with(@stud)
end
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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