Remove type switch from Measurable#initialize

[thegedge]

The type switching in Measurable#initialize means that constructing a Measurable will always incur unnecessary cost. Clients generally know what they have, so have factory functions on the class (e.g., from_decimal) that clients use to construct Measurables

[kmcphillips]

Want to provide some pseudo-code to explain this? I don't fully follow.

[thegedge]

Sure thing. Instead of Length.new(10, :cm) we'd do Length.from_numeric(10, :cm). The code would look like something this:

class Measured::Measurable
  class << self
    def from_numeric(value, unit)
      new(value, unit)
    end

    def from_string(value, unit)
      new(BigDecimal(value), unit)
    end

    private :new
  end

  def initialize(value, unit)
    raise TypeError, "Expected a numeric value" unless value.is_a?(Numeric)
    raise Measured::UnitError, "Unit cannot be blank" if unit.blank?
    raise Measured::UnitError, "Unit #{ unit } does not exist" unless self.class.conversion.unit_or_alias?(unit)

    @value = value
    @unit = conversion.to_unit_name(unit)
  end

[thegedge]

It's a slightly more verbose to write, but avoids having to go through a switch on class, which I generally like to avoid. It is more explicit though, which is something I like because implicit things can often hinder "understandability".

[kmcphillips]

I don't know. Is the case that expensive? We can provide those entry points from various types, but having a simple .new removes the burden from the caller from having to think about types each time.

[kmcphillips]

Then build it as:

  def initialize
    when Float
      from_float(value)
    when BigDecimal
      from_decimal(value)
    # etc

[thegedge]

I'm very much against switching on class since it goes against duck typing and OOP (liskov substitution principle), and it makes for a more confusing interface since you have a single method with a broad interface for an argument. Also, in a performance world, you should not pay a cost for things you don't need, so by having explicit factory functions the client gets to decide what cost they're willing to pay.

That's mostly opinion, so here are some benchmarks since I'm aiming for performance (benchmark script at the bottom of my comment):

Warming up --------------------------------------
                hash   132.160k i/100ms
                case   197.864k i/100ms
              nocase   241.405k i/100ms
Calculating -------------------------------------
                hash      2.248M (± 3.5%) i/s -     11.234M in   5.003500s
                case      4.866M (± 4.8%) i/s -     24.337M in   5.013206s
              nocase      7.769M (± 4.7%) i/s -     38.866M in   5.013716s

Comparison:
              nocase:  7769440.4 i/s
                case:  4866392.1 i/s - 1.60x  slower
                hash:  2247993.5 i/s - 3.46x  slower

case will continue to get slower as you add more branches, whereas the other two should likely stay around the same. Adding a few more whens makes it 3.1x slower.

Benchmark script

require 'benchmark/ips'
require 'bigdecimal'

MAPPERS = {
  String => proc { 'String' },
  Float => proc { 'Float' },
  BigDecimal => proc { 'BigDecimal' },
}

def hash_test(v)
  MAPPERS[v.class].call
end

def case_test(v)
  case v
  when String ; 'String'
  when Float ; 'Float'
  when BigDecimal ; 'BigDecimal'
  end
end

def nocase_test(v)
  'Float'
end

Benchmark.ips do |x|
  VALUE = 1.23
  x.report("hash") { hash_test(VALUE) }
  x.report("case") { case_test(VALUE) }
  x.report("nocase") { nocase_test(VALUE) }
  x.compare!
end