Shopify/active_shipping

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.

CC: @kmcphillips @thegedge

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.