How do you assign a variable with the result of a if..else block?

Question

I had an argument with a colleague about the best way to assign a variable in an if..else block. His orignal code was :

@products = if params[:category]
  Category.find(params[:category]).products
else
  Product.all
end

I rewrote it this way :

if params[:category]
  @products = Category.find(params[:category]).products
else
  @products = Product.all
end

This could also be rewritten with a one-liner using a ternery operator (? :) but let's pretend that product assignment was longer than a 100 character and couldn't fit in one line.

Which of the two is clearer to you? The first solution takes a little less space but I thought that declaring a variable and assigning it three lines after can be more error prone. I also like to see my if and else aligned, makes it easier for my brain to parse it!

Answer 1

Seems to me that the second one would be more readable to the typical programmer. I'm not a ruby guy so I didn't realize that an if/else returns a value.... So, to take me as an example (and yes, that's my perspective :D), the second one seems like a good choice.

Answer 2

First if using ternary, second if not.

The first one is near impossible to read.

Answer 3

I'm not a Ruby person either, but Alarm Bells are instantly ringing for scope of the second command, will that variable even be available after your if block ends?

Answer 4

As a Ruby programmer, I find the first clearer. It makes it clear that the whole expression is an assignment with the thing assigned being determined based on some logic, and it reduces duplication. It will look weird to people who aren't used to languages where everything is an expression, but writing your code for people who don't know the language is not that important a goal IMO unless they're specifically your target users. Otherwise people should be expected to have a passing familiarity with it.

I also agree with bp's suggestion that you could make it read more clearly by indenting the whole if-expression so that it is all visually to the right of the assignment. It's totally aesthetic, but I think that makes it more easily skimmable and should be clearer even to someone unfamiliar with the language.

Just as an aside: This sort of if is not at all unique to Ruby. It exists in all the Lisps (Common Lisp, Scheme, Clojure, etc.), Scala, all the MLs (F#, OCaml, SML), Haskell, Erlang and even Ruby's direct predecessor, Smalltalk. It just isn't common in languages based on C (C++, Java, C#, Objective-C), which is what most people use.

Answer 5

I would say that second version is more readable for people non familiar with that structure in ruby. So + for it! On the other side, first contruction is more DRY.

As I look on it a little bit longer, I find first solution more attractive. I'm a ruby programmer, but I didn't use it earlier. For sure I'll start to!

Answer 6

encapsulation...

@products = get_products

def get_products
  if params[:category]
    Category.find(params[:category]).products
  else
    Product.all
  end
end

Answer 7

Assuming your models look like this:

class Category < ActiveRecord::Base
  has_many :products
end
class Product < ActiveRecord::Base
  belongs_to :category
end

you could do something even more crazy, like this:

#assuming params[:category] is an id
@products = Product.all( params[:category] ? {:conditions => { :category_id => params[:category]}} : {})

Or, you could use the sexy, lazily loaded named_scope functionality:

class Product < ActiveRecord::Base
  ...

  #again assuming category_id exists
  named_scope :all_by_category, lambda do |cat_id|
    if cat_id
      {:conditions => {:category_id => cat_id}}
    end
  end

  #if params[:category] is a name, and there is a has and belongs to many
  named_scope :all_by_category, lambda do |cat_name|
    if cat_name
      {:joins => :categories, :conditions => ["categories.name = ?",cat_name]}
    end
  end
  ...
end

used like

@products = Product.all_by_category params[:category]

Answer 8

I usually find the second one cleaner. When face with small methods that essentially do this, I usually leave the extra variable out by Replacing Nested Conditional with Guard Clauses, which leaves something like this:

def retrieve_products(params)
  if params[:category]
    return Category.find(params[:category]).products
  end
  Product.all
end

In ruby I usually go a little extra mile to make it two lines, if it fits nicely:

def retrieve_products(params)
  return Category.find(params[:category]).products if params[:category]
  Product.all
end

In this case, as the conditional case is much more verbose, I'd reverse it into:

def retrieve_products(params)
  return Product.all unless params[:category]
  Category.find(params[:category]).products
end

Answer 9

@products =
if params[:category]
  Category.find(params[:category]).products
else
  Product.all
end

Is another option, it both avoids repeating @products and keeps the if aligned with else.

Answer 10

Just another approach:

category = Category.find(params[:category]) if params[:category]
@products = category ? category.products : Product.all

Answer 11

If one is browsing through the code, I'd say the second code block (yours), is definitely the one I find easiest to quickly comprehend.

Your buddy's code is fine, but indentation, as bp pointed out, makes a world of difference in this sense.