Ruby I don't like #2 - catch(:wtf) { throw :wtf } 8

Posted by pratik
on Tuesday, August 04

The 1960s and 1970s saw computer scientists move away from GOTO statements in favor of the structured programming programming paradigm. Some programming style coding standards prohibit use of GOTO statements. – Wikipedia

Ruby takes the whole GOTO nonsense to an entirely new heights. Ruby’s version of GOTO/LABEL is called throw/catch. The lunacy goes further as Ruby’s throw is equivalent to GOTO with a return value.

1
2
3
4
5
6

def hello
  throw :done, "wtf"
end

catch(:done) { hello }
=> "wtf"

Not only it makes the flow control hard to follow, it also shows your lack of fundamental programming skills. I’d love to see a case where you use throw/catch because there’s no other way. Only place I’ve ever used throw/catch is in my evil middleware Rack::Evil. And the name says it all.

Let’s take a real example from Rails :

1
2
3
4
5
6
7
8

def find_with_associations(options = {})
  catch :invalid_query do
    join_dependency = JoinDependency.new(self, merge_includes(scope(:find, :include), options[:include]), options[:joins])
    rows = select_all_rows(options, join_dependency)
    return join_dependency.instantiate(rows)
  end
  []
end

Just by looking at this method, you’ll have absolutely no idea who’s gonna be throwing :invalid_query. It could be any method subsequently called while the block is being executed. Only way to know is by doing a global search for throw :invalid_query.

Rails uses throw/catch here because it wants to return an empty array when something somewhere goes wrong. And the thing that can possibly go wrong is so deep down inside, throw/catch provides an easy way out without much refactoring. However, easy is not always the best way or the proper way.

If we look at the relevant code from the involved methods :

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27

def select_all_rows(options, join_dependency)
  connection.select_all(
    construct_finder_sql_with_included_associations(options, join_dependency),
    "#{name} Load Including Associations"
  )
end

def construct_finder_sql_with_included_associations(options, join_dependency)
  scope = scope(:find)
  sql = "SELECT #{column_aliases(join_dependency)} FROM #{(scope && scope[:from]) || options[:from] || quoted_table_name} "

  ....
  if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
    add_limited_ids_condition!(sql, options, join_dependency)
  end
  ....

  sanitize_sql(sql)
end

def add_limited_ids_condition!(sql, options, join_dependency)
  unless (id_list = select_limited_ids_list(options, join_dependency)).empty?
    sql << "#{condition_word(sql)} #{connection.quote_table_name table_name}.#{primary_key} IN (#{id_list}) "
  else
    throw :invalid_query
  end
end

This doesn’t seem that bad on the first look. But think again. Apart from the control flow retardness, the method add_limited_ids_condition adds an extra responsibility to the caller – catching invalid_query. And this is very easy to miss too – as seen with the very same method in question here – calculations.rb. Add a few of more throw/catch and you get a proper spaghetti code.

I think the better way to write the above code is :

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

def find_with_associations(options = {})
  join_dependency = JoinDependency.new(self, merge_includes(scope(:find, :include), options[:include]), options[:joins])
  rows = select_all_rows(options, join_dependency)
  rows ? join_dependency.instantiate(rows) : []
end

def select_all_rows(options, join_dependency)
  finder_sql = construct_finder_sql_with_included_associations(options, join_dependency)
  connection.select_all(finder_sql, "#{name} Load Including Associations") if finder_sql
end

def construct_finder_sql_with_included_associations(options, join_dependency)
  ....
  limitable = !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])

  unless limitable && add_limited_ids_condition!(sql, options, join_dependency).blank?
    ....
    sanitize_sql(sql)
  end
end

def add_limited_ids_condition!(sql, options, join_dependency)
  id_list = select_limited_ids_list(options, join_dependency)
  sql << "#{condition_word(sql)} #{connection.quote_table_name table_name}.#{primary_key} IN (#{id_list}) " if id_list.present?
end

I’d normally say that you should be flexible about following such rules about using a pattern or not using some. But this is an exception. Using throw/catch is just fucking wrong. Plain and simple.

Comments

Leave a response

  1. raggi – August 04, 2009 @ 06:54 PM

    I completely agree here. I only use catch + throw when there’s really no other good option, and for whatever reason, I want to intentionally blow the stack.

    In reality, the only production code I have that does this is async_sinatra, and it’s a workaround for the weakness of rack for doing async without stack frames.

    At the same time, I provide a non-throw API, which is preferred, as with all of the ideas at the moment, is not supported by most middleware.

    It does suggest an idea that throw/catch is only really needed when there’s a lack of proper design, as you also suggest here.

    Good article.

  2. Sheldon Hearn – August 04, 2009 @ 07:32 PM

    Like many tools, I think this one is easily abused but has its place.

    Judicious use of throw and catch allows lower layers to send up non-exceptional notifications to higher layers, without layers between them having to know about the implementation at either end.

    So throw/catch allows for a useful form of encapsulation. The metaphor is very much like the exception metaphor, but doesn’t imply the “disastrous bailout” that an exception should imply.

    In addition, layers in between them can deal with the notifications themselves, when they think they best know how to handle them. This allows me to produce extensible stacks. This, however, is not as strong a case as abstraction.

    The construct is powerful, and can easily be used to produce vertical goto spaghetti. But if we removed from ruby, every feature matching that description, we’d end up with Java.

  3. José Valim – August 04, 2009 @ 08:05 PM

    I think that we should have a goto equivalent, as throw/catch, since goto will be back available on PHP 5. #kidding #blackhumor

  4. Michael Jurewitz – August 04, 2009 @ 08:16 PM

    @sheldon — Exceptions are not a metaphor for encapsulation, they’re an often-abused nuisance that places a significant burden on higher level consumers of API. Cocoa on Mac OS X mostly gets exceptions right: They should be used for exceptional circumstances only, usually from operations that are obvious programmer error and that a program may not be able to recover from.

    If you’re looking to use exceptions or throw/catch as a method of communication then you’re going down the wrong path. Instead, try delegation, errors as a return value from API, or framework infrastructure like a notification center, where objects can post notifications for any other object to pick up if it is so interested.

    Exceptions have their place, but it should be minimal at most.

  5. Sam Smoot – August 04, 2009 @ 08:46 PM

    The specific example is good, but the spirit is too dogmatic.

    Most of the “Pro” camp for Exceptions as flow-control (which seems to be the majority of Rubyists :-p) should be using throw/catch instead.

    Check it: http://github.com/wiecklabs/harbor/blob/b7865329d0639984c66474c953bf9418bb953811/lib/harbor/application.rb#L58

    ASP.NET (and presumably most other frameworks/languages) uses an actual Exception for a redirect! (redirect now, jump out of current execution). IIRC it’s something like AbortRequestException. Though a redirect is always equivalent to a redirect-now in ASP.NET I think? It’s been awhile.

    Anyways, there’s nothing wrong with this, you don’t care about the stack-trace, so don’t spend time building it. Exceptions are definitely not the right way to handle this (if you have an alternative anyways).

    Yes, as a generality you’re on-point. But there are exceptions to every rule and I’d hate to see less experienced Rubyists make this one their religion.

  6. Stefan Kanev – August 04, 2009 @ 09:51 PM

    However, I don’t agree with (1) that catch/throw is even remotely close to GOTO in any conceivable way and that (2) the try/catch mechanism is actually bad, even in the example you are giving.

    First, the GOTO thing. The bad thing with GOTO is that it allows you to jump in loops and conditionals. That is really not the case here. Try/Catches are functionally equivalent to exceptions. They basically allow you to go up the stack. The only difference is the return value, but hey, you get the same with the exception — only it is the exception object and not the return value.

    Second, try/catch gives you control flow — plain and simple. Your rewrite is valid only because you have only one method that can throw :invalid_query. Now, suppose that add_limited_ids_condition! is a reusable method and it is called from a number of query constructing methods. Suppose a query constructor (select_all_rows_and_extra_stuff) is invoking it. It is also invoking a few other methods (like add_limited_ids_condition!) that might break if the query is invalid. Hence, if any of those doesn’t work as expected, you need to halt and return some other value. You could of course, go and return nils if the query is invalid, then check the nils if they are invalid and then return a nil up…

    …which is exactly what exception are there to avoid — have your success scenario straightforward and your exceptional case jumping the stack.

    So, I would argue that there is a very vague difference between exceptions and try/catch in ruby, but you can certainly make sense out of them if you adhere to some convention. Say, use exceptions to notify client code (RecordNotFound, lost connection to database, etc.) and use try/catch for specific control flow.

  7. alan – August 05, 2009 @ 08:34 AM

    Whoops, there should probably be a global find/replace on “{primary_key}” and “{table_name}”, to have a connection.quote_column_name and connection.quote_table_name applied. If set_table_name and set_primary_key are left in AR, it should be expected they could allow for mixed case names, which are broken in previous examples. My patch attempts for mixed case support have been ignored (even with included tests), but maybe you could make these (mostly trivial) changes, pratik.

    I’d have no problem making the patch for these cases, but if no one is going to look at them, it probably wouldn’t be worth it for me to do.

  8. Pratik – August 05, 2009 @ 12:06 PM

    @alan – If you could just catch me on IRC ( lifo ) or drop me an email, I’ll commit them.

    Thanks.

Comment