Skip to content
This repository has been archived by the owner on Jun 13, 2018. It is now read-only.

Improve clarity/usage of Package dimensions #434

Open
jonathankwok opened this issue Dec 5, 2016 · 4 comments
Open

Improve clarity/usage of Package dimensions #434

jonathankwok opened this issue Dec 5, 2016 · 4 comments

Comments

@jonathankwok
Copy link
Contributor

jonathankwok commented Dec 5, 2016

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

@thegedge
Copy link
Contributor

thegedge commented Dec 5, 2016

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).

@jonathankwok
Copy link
Contributor Author

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.

@kmcphillips kmcphillips added this to the Version 2.0 milestone Dec 6, 2016
@kmcphillips
Copy link
Member

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.

@garethson
Copy link
Contributor

@jonathankwok @kmcphillips Removing this from the 2.0 milestone. We're using Measured everywhere now, but leaving these signatures the same for 2.0.

@garethson garethson removed this from the Version 2.0 milestone May 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants