What would be a better way to code this if then else clause?

Question

I have the following code. I'm still a newbie in Ruby on Rails. As you can see I'm repeating myself 4 times.

I tried something like this:

if @property.nil? || @property.status_id == 144 || (@property.status_id <= 16 && current_user.nil?) || (@property.status_id <= 16 && current_user.id != @property.user_id)

But it gives me lots of errors in case @property is nil. Because then @property.status_id cannnot be called since @property is nil.

Anyway, I think an experienced Ruby on Rails coder gets the idea.

  def show
    @property = Property.find(params[:id]) rescue nil
    if @property.nil?
      flash[:error]=t("The_property_was_not_found")
      redirect_to root_path
      return
    end
    if @property.status_id == 144
      flash[:error]=t("The_property_was_not_found")
      redirect_to root_path
      return
    end
    if @property.status_id <= 16 && current_user.nil?
      flash[:error]=t("The_property_was_not_found")
      redirect_to root_path
      return
    end
    if @property.status_id <= 16 && current_user.id != @property.user_id
      flash[:error]=t("The_property_was_not_found")
      redirect_to root_path
      return
    end
    @images = Image.find(:all, :conditions =>{:property_id => params[:id]})
  end

root

Answer 1

def show
    @property = Property.find(params[:id]) rescue nil
    if @property.nil? || @property.status_id == 144 || (@property.status_id <= 16 && (current_user.nil? || current_user.id != @property.user_id))
      flash[:error]=t("The_property_was_not_found")
      redirect_to root_path
    else
      @images = Image.find(:all, :conditions =>{:property_id => params[:id]})
    end
  end

I am not familiar with Ruby syntax so this might not really compile, but you get the idea.

Answer 2

You could try this:

def show
begin 
  @property = Property.find(params[:id])
  if [144,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1,0].include?(@property.status_id)
    flash[:error]=t("The_property_was_not_found")
    if current_user && (current_user.id != @property.user_id)
      redirect_to myimmonatie_path 
    else
      redirect_to root_path 
    end
rescue
  flash[:error]=t("The_property_was_not_found")
  redirect_to root_path
end
@images = Image.find(:all, :conditions =>{:property_id => params[:id]})

end

Answer 3

Is that really the exact code? || short-circuits and a nil value shouldn't be a problem.

@property=nil
if @property.nil? || @property.status_id == 144
   puts @property.class.to_s
end

Outputs NilClass

Answer 4

I think you should approach this by defining the "can show" logic into a straightforward helper method you can call to make a determination rather than cluttering up your show method with all kinds of branches that ultimately make the same action occur.

def can_show_property?(property)
  return false unless (property)

  return false if (property.status_id == 144 or property.status_id > 16)

  return false unless (current_user && current_user.id == property.user_id)

  true
end

def show
  @property = Property.find(params[:id]) rescue nil

  unless (can_show_property?(@property))
    flash[:error]=t("The_property_was_not_found")
    redirect_to root_path
    return
  end

  @images = Image.find(:all, :conditions =>{ :property_id => params[:id] })
end

Having "magic" numbers in your code like 144 does lead to asking why they aren't assigned constants. It's usually a lot easier to understand when clearly labelled MyApp::PROPERTY_LOCKED.