Improve clarity/usage of Package dimensions
jonathankwok opened this issue ยท 4 comments
Problem
If you weren't aware of the implementation, what do you think are the details of this package?
ActiveShipping::Package.new(20, [13, 14, 15])
What is the 20
?
What is the dimensional ordering of 13x14x15
? Hint: It's not LWH.
Even if you add units: :imperial
, it doesn't explain what units you're using, just the system the units would be in. Is it inches? Feet? Furlongs? Fathoms?
Also, retrieving the length is not as easy as you would think.
irb(main):001:0> package = ActiveShipping::Package.new(20, [13, 14, 15])
=> #<ActiveShipping::Package:0x007f957e15ecd8 @options={}, @dimensions=[#<Quantified::Length: 13 centimetres>, #<Quantified::Length: 14 centimetres>, #<Quantified::Length: 15 centimetres>], @weight_unit_system=:metric, @dimensions_unit_system=:metric, @weight=#<Quantified::Mass: 20 grams>, @value=nil, @currency=nil, @cylinder=false, @gift=false, @oversized=false, @unpackaged=false>
irb(main):002:0> package.length
NoMethodError: undefined method `length' for #<ActiveShipping::Package:0x007f957e15ecd8>
No, you would have to do this:
irb(main):009:0> package.centimetres[2]
=> 15
Or is it [0]
? Who can remember. Why isn't it package.length
? Why does it return a Fixnum
and not a Quantified
? Why are we using Quantified?
Possible solution
Now, what if we changed it to be more verbose?
ActiveShipping::Package.new(
weight: Measured::Weight.new("20", "g"),
length: Measured::Length.new("15", "cm"),
width: Measured::Length.new("14", "cm"),
height: Measured::Length.new("13", "cm"),
)
Zero chance of confusion. In addition, uses the measured
gem, which is what we want in #427. It's more lines/typing, but it eliminates all areas of uncertainty. And when you're shipping something, certainty is key.
I think the use of measured is great here. And, if desired, I'm going to add an extension module to number classes (opt-in) that will make this far less verbose (e.g., 15.grams
).
Absolutely, I'm on board with a less verbose version, but not before having something clear. An express way of implementing the above could be:
package = ActiveShipping::Package.new(20.grams, [15, 14, 13].cm)
package.length # => #<Measured::Length: 15.0 cm>
Extending Array
to have some kind of units would be pretty neat I think.
We're talking in measured
about .grams
. and .cm
, but the problem with that is that it opens up and adds types to Array
and Fixnum
. Keep in mind it's not like hours
/minutes
but there are 24 measurements and their aliases in Measured::Length
. Multiply by weight and each other system. Starts to be too many additions to base types, which eventually will conflict.
That being said, new(20, [13, 14, 15])
is an unacceptable signature. We can and should be opinionated about Package
and PackageItem
and their signatures.
We should 100% remove Quantified
for all the reasons that measured
exists to improve it. But we can figure out some shorthands and put them into measured
globally.
@jonathankwok @kmcphillips Removing this from the 2.0 milestone. We're using Measured everywhere now, but leaving these signatures the same for 2.0.