HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Reversing a number in Ruby

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberreversingruby

Problem

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.

Solution

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, ac

Code Snippets

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
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
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
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
class Numeric
  def reverse
    str = to_s.reverse

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

Context

StackExchange Code Review Q#94786, answer score: 6

Revisions (0)

No revisions yet.