Barking Iguana

My main concern about returning explicitly is readability. It's a very subjective thing, but in general whenever I see an unnecessary return statement my internal WTF counter increments.

Less subjective though, it has been pointed out that returning explicitly is slower.

Benchmarking in Ruby is easy. Here's how:

require 'benchmark'

def explicit
  return "TEST"
end

def implicit
  "TEST"
end

n = 100_000_000
Benchmark.bmbm do |x|
  x.report("Explicit return") { n.times { explicit } }
  x.report("Implicit return") { n.times { implicit } }
end

And here are the results of this particular benchmark:

Rehearsal ---------------------------------------------------
Explicit return  50.380000   0.210000  50.590000 ( 51.000510)
Implicit return  36.200000   0.100000  36.300000 ( 36.454038)
----------------------------------------- total: 86.890000sec

                      user     system      total        real
Explicit return  47.650000   0.070000  47.720000 ( 47.744167)
Implicit return  35.900000   0.070000  35.970000 ( 35.985493)

This shows that while returning explicitly is slower, like the to_proc hack it's not slow enough to matter. You need to return a huge number of times to see any significant difference.

Does this change my mind? No. Returning explicitly is still ugly.

Update: The above benchmark was run on Ruby 1.8.6. Tom Ward has provided similar benchmarks for Ruby 1.8.7, 1.9 and jRuby 1.1.6 (using n = 10,000,000) which show that the cost of explicitly returning on these platforms in negligible. Still ugly though.

The stack trace is one of the most valuable pieces of information you can have when trying to debug a problem. It tells you what line of code was being run when an error was thrown and gives you an idea of the execution path that lead to that line of code being run.

Quick plea then. Please don't do this:

def foo
  do_something
rescue => e
  puts "Problem: #{e}"
  raise e
end

This will start a new stack trace at raise e. If I rescue this further up the stack there's no indication of where the problem was originally encountered - I just get pointed at your error handling code. Precious information, gone.

Do this instead:

def foo
  do_something
rescue => e
  puts "Problem: #{e}"
  raise
end

Note the lack of argument in the call to raise. This tells Ruby to re-raise the last exception. The stack trace remains intact and debugging can continue unhindered. Glory be!

Not just for Ruby this time. This applies to pretty much every programming language under the sun.

Don't use unnecessary control statements to determine whether you want to return true or false.

def foo
  if some_boolean && other_boolean
    return true
  else
    return false
  end
end

You should do this instead:

def foo
  return some_boolean && other_boolean
end

It's very very rare that I ever have to return an explicit true or false. This should be a warning sign.

Of course, in Ruby you don't need to return explicitly so you should do this:

def foo
  some_boolean && other_boolean
end

Ruby returns the value of the last line executed.

Don't do this:

def foo
  value = Foo.first(:conditions => { :label => "bar" })
  return value
end

Do this instead:

def foo
  Foo.first(:conditions => { :label => "bar" })
end

Working with arrays in Ruby. Don't do this:

index = 0
for item in array
  index += 1
  puts "Item #{index}: #{item.inspect}"
end

Do this instead:

array.each_with_index do |item, index|
  puts "Item #{index}: #{item.inspect}"
end

There are a bunch of handy methods like this. Read the Enumerable documentation and make your code that little bit more readable.

It's common knowledge that using the to_proc hack is slower than not. Just how much slower is it? I decided to put together a few benchmarks to find out.

Environment

These tests were run on Ruby 1.8.6-pl111 and Rails 2.1.

Benchmarking

Say there's a database of 1,000 items that for some reason you want to iterate over. Let's forget that if you're showing 1,000 items you probably have usability issues and just roll with it.

1_000.times { |n| Bar.create :name => "bar-#{n}" }
bars = Bar.find(:all)

Here's how much slower it is over a dataset of 1,000 ActiveRecord instances.

Benchmark.measure { bars.map(&:name) }.real
#=> 0.00645709037780762

Benchmark.measure { bars.map { |b| b.name } }.real
#=> 0.00141692161560059

Now that's a horrific increase: it takes more than 350% longer to run the to_proc hack than the plain block... but let's be realistic here, over 1,000 records it's taken 0.0065 seconds. Big woop. Who cares?

How about over 1,000,000 rows? We already have 1,000 rows, let's top that up.

(1_000_000 - 1_000).times { Bar.create :name => Time.now.to_f.to_s }
bars = Bar.find(:all)

That makes it 1,000,000 rows in the table. By this stage your database is probably thinking you hate it. I'm pretty confident that presenting 1,000,000 rows to the person using your application is a bit of an edge case, but hey, here's how long it takes.

Benchmark.measure { bars.map(&:name) }.real
#=> 6.25304508209229

Benchmark.measure { bars.map { |b| b.name } }.real
#=> 1.38965106010437

Almost 5 seconds extra over a million rows. Okay so 5 seconds is a pretty big hit, but how long will your application be running before you hit a million rows in one of your tables and you need to iterate over all million rows?

Don't optimise your code prematurely. By the time to_proc becomes an issue you'll already have hit many, many other problems.

Benchmark.measure { Bar.find(:all) }.real
#=> 406.738657951355

Worry about those first.

A few people near me have heard the start of my rumblings about optimising the number of HTTP requests required for each page. There are a variety of reasons that you might want to do this, but that discussion is for another post. For now simply assume that I don't like unnecessary HTTP requests. Extrapolating from that you would reasonably assume that I really don't like wasted HTTP requests such as might happen when the url in a CSS @import directive 404's. If you assumed that, you'd be right.

I got fed up of tracking down the source of these errors in a few applications that I maintain so I fired up a TextMate session and scraped together this nasty little snippet of Ruby to do my dirty work for me.

#! /usr/bin/env ruby

css_root = File.expand_path(`pwd`.strip)
css_files = Dir[File.join(css_root, "**", "*.css")]

missing_imports = Hash.new([])
css_files.each_with_index do |css_file, index|
  imports = File.read(css_file).split(/\n|\r/).grep(/\@import url\((.*)\)/)
  imports.each do |import|
    desired_path = import.scan(/url\((["'\ ])?(.*)\1\)/).to_a.first.to_a.last
    desired_root = desired_path[0,1] == "/" ? css_root : File.dirname(css_file)
    filesystem_path = File.expand_path(File.join(desired_root, desired_path))
    if !File.exists?(filesystem_path)
      missing_imports[css_file] += [{ :path => filesystem_path, :directive => import }]
    end
  end
end

if missing_imports.any?
  puts "Missing files declared as imports in CSS:\n\n"

  missing_imports.keys.each do |origin|
    puts "Origin:               #{origin}"
    missing_imports[origin].each do |import|
      puts "Missing @import file: #{import[:path]}"
      puts "Directive:            #{import[:directive]}"
    end
    puts ""
  end
else
  puts "No imported files are missing. Well done."
end

Run it in the directory which serves your document root - for Rails applications this would be RAILS_ROOT/public/ - and it'll either spit out a list of missing files you've tried to @import or tell you that you've done well.

Missing files declared as imports in CSS:

Origin:               /Users/craig/projects/1.8/public/stylesheets/.../find_by_service.css
Missing @import file: /Users/craig/projects/1.8/public/stylesheets/.../a_to_z.css
Directive:            @import url('.../a_to_z.css');

Now, just to be clear, I don't like @import directives. I'd prefer they were completely removed from the CSS. They are popular with a lot of people though so I'll compromise for the moment: if you must use them, please make sure they're going to work.

A lot of languages encourage talking to yourself. Lots of OO PHP code is sprinkled with code that looks like $this->foo_method();. In some languages it's necessary. Ruby isn't one of them.

class Foo
  def bar
    # Why are you talking to yourself?!
    @thingy = self.foo
  end

  def foo
    "QUUX!"
  end
end

The code above could be written without self at all.

class Foo
  def bar
    @thingy = foo
  end

  def foo
    "QUUX!"
  end
end

While this is a (very) trivial example, it makes a huge difference on larger code-bases. Give it a try: if you don't talk to yourself your code will look less crazy.

Only one caveat: when you're doing assignment you'll need to talk to yourself unless you're doing a local assignment.

class Foo
  attr_accessor :thingy

  def bar
    # This will assign to a local variable.
    thingy = foo
  end

  def foo
    "QUUX!"
  end
end

class Foo
  attr_accessor :thingy

  def bar
    # This will call Foo#thingy=
    self.thingy = foo
  end

  def foo
    "QUUX!"
  end
end