CODEWITHSUNDEEP
rubycoding-style
Pierre Olivier Martel
Pierre Olivier Martel

Reputation: 3194

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

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!

Upvotes: 48

Views: 56656

Answers (16)

tanius
tanius

Reputation: 16759

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

Let's pretend not – because in many cases, it pays off for readability to optimize variable names and condition complexity for length so that the assignment incl. condition fits into one line.

But instead of using the ternary operator (which is unnecessary in Ruby and hard to read), use if … then … else. The then (or a ;) is necessary when putting it all into one line. Like this:

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

For readability and self-documenting code, I love it when my programs read like English sentences. Ruby is the best language I found for that, so far.

Upvotes: 0

Julien 'JS'
Julien 'JS'

Reputation: 149

I think the best code is :

@products = Category.find(params[:category])&.products.presence || Product.all

The "&" after de find ensure that the method "products" will not evaluate if category is nil.

Upvotes: 1

Slipp D. Thompson
Slipp D. Thompson

Reputation: 34903

I don't like your use of parentheses in your first block. Yes, I'm a LISP user, but I believe I make a fair point when I say the first might look confusing in the middle of other code, maybe around other if blocks.

How about...

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

(^ Tongue in cheek, to exacerbate the issues with @badp's answer)

Upvotes: 0

Tombart
Tombart

Reputation: 32388

Yet another approach would be using a block to wrap it up.

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

which solves the assignment issue. Though, it's too many lines for such "complex" code. This approach would be useful in case that we would want to initialize the variable just once:

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

This is something you can't do with the rewritten code and it is correctly aligned.

Upvotes: 6

antinome
antinome

Reputation: 3458

As an alternative to the syntax in badp's answer, I'd like to propose:

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

I claim this has two advantages:

  1. Uniform indentation: each level of logical nesting is indented by exactly two spaces (OK, maybe this is just a matter of taste)
  2. Horizontal compactness: A longer variable name will not push the indented code past the 80 (or whatever) column mark

It does take an extra line of code, which I would normally dislike, but in this case it seems worthwhile to trade vertical minimalism for horizontal minimalism.

Disclaimer: this is my own idiosyncratic approach and I don't know to what extent it is used elsewhere in the Ruby community.

Edit: I should mention that matsadler's answer is also similar to this one. I do think having some indentation is helpful. I hope that's enough to justify making this a separate answer.

Upvotes: 69

Andres
Andres

Reputation: 495

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.

Upvotes: 0

Matías Flores
Matías Flores

Reputation: 69

Just another approach:

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

Upvotes: 6

Chuck
Chuck

Reputation: 237010

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.

Upvotes: 22

BaroqueBobcat
BaroqueBobcat

Reputation: 10150

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]

Upvotes: 2

matsadler
matsadler

Reputation: 189

@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.

Upvotes: 2

badp
badp

Reputation: 11813

I don't like your use of whitespace in your first block. Yes, I'm a Pythonista, but I believe I make a fair point when I say the first might look confusing in the middle of other code, maybe around other if blocks.

How about...

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


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

You could also try...

@products = Product.all #unless a category is specified:
@products = Category.find(params[:category]).products if params[:category]

...but that's a bad idea if Product.all actually is a function-like which could then be needlessly evaluated.

Upvotes: 17

Derick Bailey
Derick Bailey

Reputation: 72858

encapsulation...

@products = get_products

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

Upvotes: 16

klew
klew

Reputation: 14967

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!

Upvotes: 1

blissapp
blissapp

Reputation: 1370

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?

Upvotes: 1

Nate Noonen
Nate Noonen

Reputation: 1371

First if using ternary, second if not.

The first one is near impossible to read.

Upvotes: 1

joeslice
joeslice

Reputation: 3454

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.

Upvotes: 0

Related Questions