Topic: Clean code.

Hi!
Im trying to make my code pretty and easy to read but one part is a major pain in the ass.

Let's say I want to get a certain user with an id.

def show
    @user = User.find(:first, :conditions => ["id = ?", params[:id]])
end

Ok, so is that the 'best' way?

But that's not it, let's say the user tries to find localhost/show/asdasdasd or just localhost/show/

This is how i protect myself from that.

  def show
    @user = User.find(:first, :conditions => ["id = ?", params[:id]])

    redirect_to :action => "show", :id => 1 unless @user != nil
    params[:id] = 1 unless params[:id] =~ /\A[0-9]+\Z/
  end


All this just doesn't feel rubyish enough. I suppose I should use rescue instead, anyone wanna push me in the right direction?

Thanks /emil.

Re: Clean code.

At the moment I'm doing it this way:

def show
  # Defaults to :first if params[:id] is nil
  id = (params[:id] or :first)
  @user = User.find id
rescue
  flash[:notice] = 'Error selecting record, redirecting to default.'
  redirect_to :action => :show
end

Ofcourse this doesn't tell you whether a record wasn't found because the id number doesn't exist or someone tried entering a weird string rather than an id.

Taking a line from your code you could do this instead:

def show
  # Use params[:id] if it conforms to the regex, otherwise use :first
  params[:id] =~ /\A[0-9]+\Z/ ? id = params[:id] : id = :first
  @user = User.find id
end

Or even this, since .to_i converts nil as well as non-numeric strings to 0. This one doesn't feel too reliable to me though.
def show
  params[:id].to_i > 0 ? id = params[:id] : id = :first
  @user = User.find id
end

Re: Clean code.

Thanks alot!

Re: Clean code.

I'm not exactly sure what the requirements are of what you want. Perhaps this will do it?

@user = User.find_by_id(params[:id]) || User.find(:first)

Does this not satisfy the requirements? If you could specify why not, perhaps I can help further.

Railscasts - Free Ruby on Rails Screencasts

Re: Clean code.

ryanb wrote:

I'm not exactly sure what the requirements are of what you want. Perhaps this will do it?

@user = User.find_by_id(params[:id]) || User.find(:first)

Does this not satisfy the requirements? If you could specify why not, perhaps I can help further.

Hi Ryan. Thanks for your answer, that rocks! One question though, that is safe against sql-injections, aye?

Thanks! //emil

Re: Clean code.

Yep, as long as you aren't directly inserting user input into an SQL string (say in the find :conditions option) then it is safe against SQL injection. Rails will automatically escape the input varaibles.

Railscasts - Free Ruby on Rails Screencasts

Re: Clean code.

def find_user
begin
  @user = User.find(params[:id])
rescue ActiveRecord::RecordNotFound
  flash[:error] = "Record Not Found"
  redirect_to :action => :somewhere
else
  # do something
end

I usually use begin...end ... think it's cleaner this way

Re: Clean code.

I've always done:

@user = User.find_by_id(params[:user])

if !@user
  redirect_to :action => "whatever"
  return
end


But I think the approach with rescue looks a lot cleaner.

Re: Clean code.

Just a quick tip. Instead of this:

def find_user
  begin
    @user = User.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    flash[:error] = "Record Not Found"
    redirect_to :action => :somewhere
  else
    # do something
  end
end

You can do this:

def find_user
  @user = User.find(params[:id])
rescue ActiveRecord::RecordNotFound
  flash[:error] = "Record Not Found"
  redirect_to :action => :somewhere
else
  # do something
end

In this case you are calling rescue on the entire method. Of course this won't work if you need to do something outside of this rescue block.

Railscasts - Free Ruby on Rails Screencasts

Re: Clean code.

Hey, nice one Ryan.  I didn't know you could treat an entire method like begin/rescue block.