
Small Refactorings in Ruby - caiusdurling
http://caiustheory.com/some-small-refactorings-in-ruby
======
stephth
I enjoyed reading that, thanks.

One thing: replacing

    
    
        def my_method
          @foo == :bar ? :foo_matches : nil
        end
    

with

    
    
        def my_method
          :foo_matches if @foo == :bar
        end
    

doesn't look like an improvement to me. The first style is direct and
readable, and lowers the barrier of entry to understand it. The second one
requires to be familiar with the subtleties of Ruby, which adds difficulty
when you're working with multiple languages daily or getting started with
Ruby. I'm not saying you should avoid pushing Ruby to its best capabilities so
that it's readable by any programmer, but in this case, winning 4 characters
doesn't seem worth the compromise.

~~~
FuzzyDunlop
I think the example for that is bad, when you look at what it's trying to
explain.

    
    
       def foo_matches?
         @foo == :bar
       end
    

It returns true if it does, and false if it doesn't, as opposed to true or
nil. Which is miles better than any of those alternatives.

It also failed to consider that the naming of the method and the usage of Ruby
naming conventions is just as important as what goes on in the body.

~~~
stephth
Agreed, this is an example where returning true or false would be more useful.
I presumed the point was assuming a situation where a method returning either
a value or nil would be desirable.

------
LargeWu
I am just not a fan of that pattern of testing the return value of an
assignment statement:

    
    
      if (( info = details[:info] ))
        ...do something
    

instead of

    
    
      info == details[:info]
      if info
        ... do something
    

It always looks like somebody typed = instead of ==. Or at the very least, I
have to stop and think about what's going on. Yes, I get it, it lets you do
something in one line of code, but I think it sacrifices readability and can
make the intent ambiguous.

~~~
xentronium
Usually there is a convention to put parens around if-assignment clause, so it
is detectable.

~~~
LargeWu
That's interesting, I've never seen that convention in use before. Or at least
never picked up on it.

~~~
caiusdurling
Someone on twitter asked me about that convention too - the best explanation I
could find was that it's from C, where the compiler will warn you if you have
a plain `if (foo = "bar")` in case you typo'd it. But putting double parens
around it makes the compiler stop warning you, as you've intended the code to
be that way by using a double parens. -
[http://stackoverflow.com/questions/3323257/double-
parenthese...](http://stackoverflow.com/questions/3323257/double-parentheses-
in-sample-code/3323400#3323400) was the explanation I linked on twitter about
it.

------
danso
I like using the multiple-equality-comparison technique mentioned as the top
example by the OP:

    
    
           foo == "bar" || foo == "baz" || foo == :sed || foo == 5
           ["bar", "baz", :sed, 5].include?(foo)
    
    

But that just _has_ to be several times slower than the boolean check with the
array usage, enough that you wouldn't want this refactoring in a mega-loop,
right?

~~~
steveklabnik
> that just has to be several times slower

This is why Matz gave us benchmark:

    
    
        steve at thoth in ~/tmp
        $ cat benchmark.rb 
        require 'benchmark'
    
        n = 500000
        Benchmark.bm do |x|
          x.report("==") do
            n.times do 
              foo = "lol"
              foo == "bar" || foo == "baz" || foo == :sed || foo == 5
            end
          end
    
          x.report("include?") do
            n.times do
              foo = "lol"
              ["bar", "baz", :sed, 5].include?(foo)
            end
          end
        end
    
        steve at thoth in ~/tmp
        $ ruby benchmark.rb
               user     system      total        real
        ==  0.370000   0.000000   0.370000 (  0.367943)
        include?  0.550000   0.000000   0.550000 (  0.551897)

~~~
cheald
Now do it enough to trigger a couple of GC passes, and the #include? version
looks even worse.

------
cheald
Example #1 is actually a bad thing to do. This creates a new array every time
you want to do the comparison. I've fixed multiple open-source libraries who
abused this exact method, refactored them into the chained "and" statements,
and seen a hefty performance increase as a result (MongoMapper and Haml come
to mind)

Example #3 suffers from unbalanced returns and is less clear in its intent
than the original (albeit difficult to read) example. Instead, I'd propose:

    
    
        def my_method
          @blah == foo && :foo_matches || :no_match
        end
    

This eliminates the visual problem of adjacent semicolons, while still being
clear and concise.

I'd argue that Example #4 is bad because it hides the expectation that nil
should be returned in the negative case.

------
jrochkind1
> name = details[:info] && details[:info][:name]

If you have ActiveSupport, then:

> name = details[:info].try {|hash| hash[:name]}

~~~
caiusdurling
Object#try should work in plain 1.9 too, although I'm not sure I'd have
reached for it in this case. This was more about showing the DRYing up a
(visual) pattern across two lines into a different construct though, so
perhaps I would've used try if it was just one line somewhere in my code.

That said, I didn't know try could take a block instead of a symbol, so thanks
for sharing :-)

