5
\$\begingroup\$

I'm writing a function to reverse a number in Ruby. IE, 314159.reverse should return 951413.

Here's what I have, which works:

class Numeric
  def reverse
    str = self.to_s.reverse

    if self.is_a?(Float)
      return str.to_f
    elsif self.is_a?(Integer)
      return str.to_i
    end
  end
end

I don't like the enumeration of different number types. Is there a way to dynamically pass in the type?

Something like:

return str.to_type(self.type)

Any other thoughts on simplifying this?

See also: My related Stack Overflow question on some other approaches I tried.

\$\endgroup\$
  • 1
    \$\begingroup\$ What does it even mean to reverse a Float? An extra digit of precision would make a huge difference in the result. \$\endgroup\$ – 200_success Jun 26 '15 at 18:31
  • \$\begingroup\$ @200_success For example, if self is 4042.840399100887, it should return 788001993048.2404. (Which my code does without precision issues.) \$\endgroup\$ – Aaron Brager Jun 27 '15 at 2:39
6
+50
\$\begingroup\$

I am going to refactor this in small steps so that you can follow my thought process and see where you agree and you disagree.

First, let's start with some style. In Ruby, self is the implied receiver of a message send if you don't supply one, it is generally not necessary to state it explicitly:

class Numeric
  def reverse
    str = to_s.reverse

    if is_a?(Float)
      return str.to_f
    elsif is_a?(Integer)
      return str.to_i
    end
  end
end

Conditionals are expressions in Ruby, not statements. In fact, everything is an expression in Ruby. So, conditionals evaluate to a value, which means that instead of returning a value from each of the branches, we can return the value of the entire conditional expression instead:

class Numeric
  def reverse
    str = to_s.reverse

    return if is_a?(Float)
      str.to_f
    elsif is_a?(Integer)
      str.to_i
    end
  end
end

Being an expression-oriented language, the value of a method body (and also a block body, module body, class body, any block of code, really) is the value of the last expression evaluated inside that body. There is no need to explicitly return (or next in case of a block) it:

class Numeric
  def reverse
    str = to_s.reverse

    if is_a?(Float)
      str.to_f
    elsif is_a?(Integer)
      str.to_i
    end
  end
end

For multi-way conditionals like this, it is usually more legible to use a case expression, instead of a series of if and elsifs:

class Numeric
  def reverse
    str = to_s.reverse

    case
    when is_a?(Float)
      str.to_f
    when is_a?(Integer)
      str.to_i
    end
  end
end

Now we can start doing some real interesting changes. Module#=== checks whether its argument is an instance of self. Object#is_a? checks whether self is an instance of its argument. IOW: the two methods are mirror images of each other, you can usually exchange o.is_a?(m) with m === o and vice-versa. This allows us to simplify our case expression even more:

class Numeric
  def reverse
    str = to_s.reverse

    case self
    when Float
      str.to_f
    when Integer
      str.to_i
    end
  end
end

Now, on to the big thing!

In an object-oriented language, it is always possible to replace a conditional with runtime polymorphic message dispatch. Message dispatch is more powerful than conditionals. Smalltalk is the existence proof of that, it doesn't even have conditionals built into the language, its conditionals are implemented in the library, using message dispatch, kind of like this:

class TrueClass
  def if_then_else(then_block, else_block)
    then_block.()
  end
end

class FalseClass
  def if_then_else(then_block, else_block)
    else_block.()
  end
end

(2 > 3).if_then_else(-> { puts 'True' }, -> { puts 'False' })
# False

This gives rise to the Replace Conditional with Polymorphism Refactoring. (Here is an example of it in Ruby.)

In your case, you are not just switching behavior based on some abstract notion of "type" of the object, you are literally switching behavior based on the class. Even worse: you are switching based on the class of self.

That is a big honking red flag: self always knows what class it is. It should never ever have to check for its own class!

What you are basically doing is re-implementing message dispatch:

foo.bar

will run different code, depending on the class of foo. That's already built into Ruby, you don't have to re-implement it yourself.

So, we let Ruby take care of making the decision whether to invoke the Float or the Integer version of our code:

class Integer
  def reverse
    str = to_s.reverse

    str.to_i
  end
end

class Float
  def reverse
    str = to_s.reverse

    str.to_f
  end
end

Okay. So, the last bit we have here is a little bit of code duplication, which we can get rid of by using the Extract Method Refactoring followed by the Pull Up Method Refactoring.

Extract:

class Integer
  def reverse
    reverse_to_s.to_i
  end

  private def reverse_to_s
    to_s.reverse
  end
end

class Float
  def reverse
    reverse_to_s.to_f
  end

  private def reverse_to_s
    to_s.reverse
  end
end

Pull Up:

class Numeric
  private def reverse_to_s
    to_s.reverse
  end
end

class Integer
  def reverse
    reverse_to_s.to_i
  end
end

class Float
  def reverse
    reverse_to_s.to_f
  end
end

Now we're done. Our code is nice and clean, all our methods do exactly one thing, there are no IFs or other conditionals.

The only slightly smelly bit left is the monkey-patching we are doing. Refinements to the rescue!

module ReversibleNumeric
  refine Numeric do
    private def reverse_to_s
      to_s.reverse
    end
  end

  refine Integer do
    def reverse
      reverse_to_s.to_i
    end
  end

  refine Float do
    def reverse
      reverse_to_s.to_f
    end
  end
end

314159.reverse
# NoMethodError

using ReversibleNumeric

314159.reverse
# => 951413

[Well, actually, Refinements are still in a state of flux, and the above code may or may not work, depending on your exact version and implementation of Ruby.]

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

How about using a case statement.

def reverse(n)
  as_string = n.to_s.reverse
  case n
  when Float
    Float(as_string)
  when Integer
    Integer(as_string)
  end
end

reverse(1234.5678) # => 8765.4321 A Float
reverse(12343) # => 34321 which is an Integer
reverse(832987979497908) # => 809794979789238 A Bignum
reverse(0371) # => 942 An Octal
reverse(0x1f) # => 13 A Hexadecimal
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I like that this doesn't use to_f, etc. But is there a way to go a step further - something like n.type(as_string) - which would let me remove the case statement entirely? \$\endgroup\$ – Aaron Brager Jun 29 '15 at 7:48
  • \$\begingroup\$ You can, of course, choose not to use the case statement for another branch statement. I think it would simply trade complexities. Even your statement suggests that you are converting a number to a type from a string, which hints that you would get the same type as the object referred to as 'n', as the method name should reflect the return of the method. \$\endgroup\$ – vgoff Jun 29 '15 at 23:13
2
\$\begingroup\$

Modifying the internals of a class is usually frowned upon, also, I feel like reversing a float is not going to be useful, so you can greatly simplify when caring only about integers:

def number_reverse(n)
  n.to_s.reverse.to_i
end

p number_reverse(12345) #-> 12345
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Reversing a float is a requirement. \$\endgroup\$ – Aaron Brager Jun 26 '15 at 18:30
  • \$\begingroup\$ The article you linked to makes a good case for not overriding existing methods, but I'm not doing that here. Unless future versions of Ruby add a reverse method to Numeric, what's the harm? \$\endgroup\$ – Aaron Brager Jun 26 '15 at 18:31
  • 1
    \$\begingroup\$ @AaronBrager "What's the harm?" The harm is if everyone does the same. Ruby might not add this or that method, but maybe someone else's code will ("what's the harm?" they said). Or maybe you're overriding something that some other code added and relies on, and again things break. Hence the advice to just not do it. Otherwise you risk a slippery slope. Not saying it will happen, but if there's a another solution, why risk it? As you yourself implied, there's really no practical reason to add a reverse method to Numeric, so why would you do it? \$\endgroup\$ – Flambino Jun 26 '15 at 19:48
  • \$\begingroup\$ @Flambino Ah, interesting. In Swift, such a conflict would cause a compile-time error, making it immediately apparent and easily diagnosable. \$\endgroup\$ – Aaron Brager Jun 27 '15 at 2:41
  • 1
    \$\begingroup\$ @AaronBrager No (separate) compile step in Ruby. It's just a completely different beast. More flexibility, though - just act responsibly. E.g. ActiveSupport from Rails includes many additions to core stuff, but that its point. The presumption is that you use it with open eyes. And it strives to only add widely-applicable stuff with well-known behavior. Your reverse method is pretty narrow and its behavior isn't quite implicit (e.g. what does it mean to reverse a float? Flipping some bits?) so be careful with mixins. It's OK to use for exercises or small isolated scripts, though. \$\endgroup\$ – Flambino Jun 27 '15 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.