5
\$\begingroup\$

The ElectricalEngine class responds to the horsepower message. Because efficiency is calculated in percent a programmer can mistakenly initialize it with an integer instead of a float.

class ElectricalEngine
  attr_reader :volts, :current, :efficiency

  def initialize(volts, current, efficiency)
    @volts, @current, @efficiency = volts, current, efficiency
  end

  HP_IN_WATTS = 746
  def horsepower
    (volts * current * efficiency) / HP_IN_WATTS
  end
end

puts ElectricalEngine.new(240, 90, .6).horsepower # correct
puts ElectricalEngine.new(240, 90, 60).horsepower # buggy

How would you handle this scenario?

  1. Do nothing. It's the programmers responsibility to know the right datatype.
  2. Rename efficiency to efficiency_as_float to make it clearer.
  3. Rename efficiency to efficiency_as_percent and adjust horsepower's calculation.
  4. Write a custom efficiency method to check the datatype and convert it accordingly.
  5. Check efficiency type and raise an error if it's not a float.
  6. Other

Solution four might look like this. Of course this conversion can happen in the initialize method too, but I think this is cleaner.

def horsepower
  (volts * current * efficiency_as_float) / HP_IN_WATTS
end

def efficiency_as_float
  if efficiency.is_a?(Integer) # what if 1 is passed in instead of 1.0?
    efficiency / 100.to_f
  else
    efficiency
  end
end

Solution 5 would look something like this:

def initialize(volts, current, efficiency)
  raise "Efficiency must be a float" unless efficiency.is_a?(Float)
  @volts, @current, @efficiency = volts, current, efficiency
end

Should ElectricalEngine own the responsibility of converting incorrect datatypes?

\$\endgroup\$
  • \$\begingroup\$ srp is a meta tag and I have removed it. If you are concerned about violating the single responsibility principle, please mention that concern in your question. \$\endgroup\$ – RubberDuck Nov 18 '14 at 13:18
  • 1
    \$\begingroup\$ On a related note: I have rejected your proposed tag-wiki edit. The reasoning is the same as provided by RubberDuck already. For more information, see the relevant meta-discussion \$\endgroup\$ – Vogel612 Nov 18 '14 at 13:28
  • 1
    \$\begingroup\$ Thanks for the notes. I've just wasted 5 minutes trying to figure out what "burninate" means :D \$\endgroup\$ – Mohamad Nov 18 '14 at 13:33
  • \$\begingroup\$ Can't you just check if efficiency is a number between 0 and 1.0? \$\endgroup\$ – Kristof Claes Nov 18 '14 at 14:08
  • \$\begingroup\$ @KristofClaes I could, yes. But I classify that under the "raise an error" solution. The implementation details are not that important at this stage. I'm interested more in "the spirit" of the approach. \$\endgroup\$ – Mohamad Nov 18 '14 at 14:12
6
\$\begingroup\$

I'd say pick #1: Do nothing (except, as m_x says, use to_f). But, if you really, really want to do something, pick #5: Raise an error. Specifically, I'd recommend raising a RangeError with a helpful message.

raise RangeError, "efficiency must be between 0 and 1" unless (0..1).cover?(efficiency.to_f)

As for this:

Because efficiency is calculated in percent a programmer can mistakenly initialize it with an integer instead of a float.

Yes, it can happen (I've made such mistakes myself), but using a 0..1 float is the more common approach (in my experience). It's usually only spreadsheets that deal with percentages as 0..100; in most programming (and math) contexts, "percent" means some 0..1 number. So calling it efficiency_as_percent could cause the opposite effect: People passing a 0..1 float where an int is required.

Either way, the efficiency isn't specifically percentage (although you might render it as such); it's just a ratio. A factor. Hence floats make more sense, as they allow you set a more precise value than 0..100.

Of course, you have to be a bit pragmatic about all this, so you don't end up implementing a strict type in a dynamically-typed language. For instance, you could also check volts and current - e.g. they probably shouldn't be negative. But then it quickly becomes a huge headache.

You might also ask, "well, what if I want to calculate the horsepower of an over-unity engine?". Well, you can't, if efficiency can't go above 1.0. But in a sense, it's just algebra. You've got a formula, and you can plug whatever you want into it. Whether it makes sense to plug certain values in doesn't change the math. From a practical standpoint, a 200% efficiency engine is of course impossible, but you can still do the math just fine. Heck, even a perfect, 100% efficiency engine is impossible. Sooo should you only allow 0...1 (half-open range)? If you don't allow 1.0 itself, then how close a value do you allow? 0.9? 0.99999?

Similarly, a zero-efficiency engine sounds like a mistake, so should you point that out too? And so on and so on...

Anyway, I'd be ok with checking efficiency, but I wouldn't bother with it myself. Leave it to the programmer to do things right or suffer the consequences. GIGO: Garbage In, Garbage Out.

\$\endgroup\$
  • \$\begingroup\$ I see where you are going with this. It makes perfect sense. If I want to check for this, then why not check other things too? It seems like an arbitrary choice to focus on efficiency because I believe it's easier to make an mistake. Thanks. \$\endgroup\$ – Mohamad Nov 18 '14 at 14:15
  • \$\begingroup\$ "You've got a formula, and you can plug whatever you want into it" >> I think every math graduate reading that will choke to death :D. Anyway, good point here, and I'll add : if you really care about type semantics, just create an Efficiency class that responds to to_f and raises a range error when initialized with a value out of bound (or silently constrains the value inside accepted range). This way there's two distinct steps : checking if the values are meaningful, and combining values within a formula. \$\endgroup\$ – m_x Nov 18 '14 at 14:23
  • 1
    \$\begingroup\$ @m_x Well, in that case: I accept no responsibility for any choking-related issues experienced by math grads :) But really, the calculation will produce a result - of some kind - for any combination of (real) numbers, but perhaps physics grads will choke (again: not responsible!) if you start talking about >100% efficiency :) Anyway, good point there, about making it a more explicit 2-step process. \$\endgroup\$ – Flambino Nov 18 '14 at 15:00
6
\$\begingroup\$

my two cents : use duck-typing.

@efficiency = efficiency.to_f

... if it quacks like a float, then it is a float. This allows to leverage ruby's awesomeness with things like :

class EfficiencyProfile 
  def initialize(some_data, value)
    @data = some_data
    @efficiency_value = some_value 
  end

  def to_f 
    @efficiency_value.to_f
  end 
end 

e = EfficiencyProfile.new(some_big_chunk_of_data_about_engine_performance, 42)
ElectricalEngine.new(1,2,e) # would work without a complain

Dynamic typing sure is dangerous, but you have to embrace it if you want to benefit from it.

Checking the type explicitly is an half-arsed solution, because :

  • it is cumbersome, and you will never be as good at it than, say, a C compiler is
  • it will completely remove the sole benefit from dynamic typing : flexibility.
\$\endgroup\$
  • \$\begingroup\$ I'm not clear on the benefits derived from the example you provided. To me it looks like extracting the efficiency into its own class and using dependency injection. Am I correct? \$\endgroup\$ – Mohamad Nov 18 '14 at 14:17
  • \$\begingroup\$ not exactly. My example is not really clear : what i'm trying to say is that you only need to care that what you get responds to to_f. That way, if you want to use anything other than a Float, it is still possible. Duck typing is better explained with non primitive types : one good example is the core StringIO class, that mimics a "real" IO object - you can use it with any lib that demands an IO object (responds to #read, #write, #close, etc.) because those libs just assume that if what you pass to them quacks like an IO, then it is an IO. \$\endgroup\$ – m_x Nov 18 '14 at 14:30
  • \$\begingroup\$ It doesn't matter what type efficiency is, all that matters is that efficiency is between 0 and 1. This is also the message that the user wants to see: not that they used the wrong type, but that their input was out of bounds. This would also protect against someone accidentally passing 80.5, thinking they were passing 80.5%. \$\endgroup\$ – raptortech97 Nov 18 '14 at 17:47

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.