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

Change Package interface for ActiveShipping 2.0 #469

Closed
wants to merge 2 commits into from
Closed

Change Package interface for ActiveShipping 2.0 #469

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ef5c00
Change Package interface for ActiveShipping 2.0
Jan 12, 2017
7dcc362
Code review changes:
Jan 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 34 additions & 125 deletions lib/active_shipping/package.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,118 +1,62 @@
module ActiveShipping #:nodoc:
class Package
cattr_accessor :default_options
attr_reader :options, :value, :currency
cattr_accessor :default_options do {} end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer:

cattr_accessor: default_options
self.default_options = {}

attr_reader :weight, :length, :width, :height, :options, :value, :currency

# Package.new(100, [10, 20, 30], :units => :metric)
# Package.new(Mass.new(100, :grams), [10, 20, 30].map {|m| Length.new(m, :centimetres)})
# Package.new(100.grams, [10, 20, 30].map(&:centimetres))
def initialize(grams_or_ounces, dimensions, options = {})
options = @@default_options.update(options) if @@default_options
options.symbolize_keys!
@options = options
alias_attribute :mass, :weight

@dimensions = [dimensions].flatten.reject(&:nil?)
def initialize(weight:, length:, width:, height: nil, options: {})
@options = @@default_options.merge(options.symbolize_keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @@default_options can be changed by a client of this library then I'd recommend @@default_options.merge(options).symbolize_keys to guarantee all keys are symbols.


imperial = (options[:units] == :imperial) ||
([grams_or_ounces, *dimensions].all? { |m| m.respond_to?(:unit) && m.unit.to_sym == :imperial })
raise ArgumentError, "Weight needs to be a Measured::Measurable object" unless weight.is_a?(Measured::Measurable)
raise ArgumentError, "Length needs to be a Measured::Measurable object" unless length.is_a?(Measured::Measurable)
raise ArgumentError, "Width needs to be a Measured::Measurable object" unless width.is_a?(Measured::Measurable)
raise ArgumentError, "Height needs to be a Measured::Measurable object" unless height.nil? || length.is_a?(Measured::Measurable)

weight_imperial = dimensions_imperial = imperial if options.include?(:units)
@weight = weight

if options.include?(:weight_units)
weight_imperial = (options[:weight_units] == :imperial) ||
(grams_or_ounces.respond_to?(:unit) && m.unit.to_sym == :imperial)
end

if options.include?(:dim_units)
dimensions_imperial = (options[:dim_units] == :imperial) ||
(dimensions && dimensions.all? { |m| m.respond_to?(:unit) && m.unit.to_sym == :imperial })
end

@weight_unit_system = weight_imperial ? :imperial : :metric
@dimensions_unit_system = dimensions_imperial ? :imperial : :metric

@weight = attribute_from_metric_or_imperial(grams_or_ounces, Measured::Weight, @weight_unit_system, :grams, :ounces)
@length = length
@width = width
@height = height || ( cylinder? ? width : Measured::Length.new(0, length.unit) )

if @dimensions.blank?
zero_length = Measured::Length.new(0, (dimensions_imperial ? :inches : :centimetres))
@dimensions = [zero_length] * 3
else
process_dimensions
end
@value = self.class.cents_from(options[:value])
@currency = options[:currency] || options[:value].try(:currency)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I pass in the following Hash for options:

{
  value: MoneyWithCurrency.new(10, currency: :usd),
  currency: :cad
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'd question why the input is mismatched internally.

It's a matter of prioritizing an explicit value given for the options hash, vs. prioritizing the currency defined with the value object that might respond to currency.

I can see the value in either, but I favoured the input that was explicitly given.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would actually trust the currency attached to value more than a given currency, but in the example I gave there's an inconsistency, so I think it would be fair to raise ArgumentError.

end

@value = Package.cents_from(options[:value])
@currency = options[:currency] || (options[:value].currency if options[:value].respond_to?(:currency))
@cylinder = (options[:cylinder] || options[:tube]) ? true : false
@gift = options[:gift] ? true : false
@oversized = options[:oversized] ? true : false
@unpackaged = options[:unpackaged] ? true : false
def dimensions
[@length, @width, @height]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be memoized too, if this object is correctly immutable.

end

def unpackaged?
@unpackaged
def girth
@girth ||= cylinder ? width.scale(Math::PI) : (width + height).scale(2)
end
alias_method :circumference, :girth
alias_method :around, :girth

def oversized?
@oversized
def volume
@volume ||= if cylinder?
Math::PI * width.scale(0.5).value * height.scale(0.5).value * length.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 dat-scale

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, you're switching to value here. And that makes the incorrect assumption that unit is the same on all of these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

First, would the measured gem value from a function to coerce a list of values into the same unit? For example, this would be written as:

Measured.coerce_unit(width.scale(0.5), height.scale(0.5), length).reduce(&:*)

We could also add a function to do the inner product of the values until we support multiplying these together.

Second, I would recommend parentheses around the three terms on the right, because the first multiply will coerce things into a float and then you'll lose some precision gained from :value being Rational or BigDecimal. The other option would be to create a Rational or BigDecimal PI constant in this class and use that instead (recommended). The nice thing about that is that it will reduce the number of return value types.

else
length.value * width.value * height.value
end
end

def cylinder?
@cylinder
@cylinder ||= @options[:cylinder].present?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is then saying cylinder: "false" means that it is a cylinder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily wrong, but as long as we're ok with that. And document it. And are consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't feel strong about this but I think that's an odd behaviour if someone is verbose about their options. I don't like using options hash where we largely leverage key/value but in some cases rely on the solely the key as a flag.

I'd recommend we should promote something other usage like:

options = {
  ...
  shape: :cylinder
}

Then this becomes:

def cylinder?
  @cylinder ||= @options[:shape] == :cylinder
end

Maybe shape isn't the best word, maybe package_type or type or mail_piece_shape. It also means we avoid where causes like

options: {
  cylinder: 'false',
  envelope: 'false',
}

Also are we better to talk about box and tube which are pretty common packing types versus cylinder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great insight. I'm okay with shape, a Package generally has a shape - box, tube, or envelope. I won't enforce the enumeration of the shape, but I'll keep the boolean methods for common shapes e.g. box/tube/envelope.

end
alias_method :tube?, :cylinder?

def gift?
@gift
@gift ||= @options[:gift].present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I think this could be expressed as a type of reason for export. There are a few like sale, gift, etc.. see image below. Therefore this could become

def gift?
  @gift ||= @options[:export_reason] == :gift
end

screen shot 2017-01-13 at 10 56 52 am

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export_reason is a little strangely worded to me, mostly for the export included. Maybe mail_reason? ship_reason?

Actually, since this is a customs concern, export is perfectly natural. I'll go with export_reason unless somebody has arguments for otherwise.

end

def ounces(options = {})
weight(options).convert_to(:oz).value
end
alias_method :oz, :ounces

def grams(options = {})
weight(options).convert_to(:g).value
end
alias_method :g, :grams

def pounds(options = {})
weight(options).convert_to(:lb).value
end
alias_method :lb, :pounds
alias_method :lbs, :pounds

def kilograms(options = {})
weight(options).convert_to(:kg).value
end
alias_method :kg, :kilograms
alias_method :kgs, :kilograms

def inches(measurement = nil)
@inches ||= @dimensions.map { |m| m.convert_to(:in).value }
measurement.nil? ? @inches : measure(measurement, @inches)
end
alias_method :in, :inches

def centimetres(measurement = nil)
@centimetres ||= @dimensions.map { |m| m.convert_to(:cm).value }
measurement.nil? ? @centimetres : measure(measurement, @centimetres)
def oversized?
@oversized ||= @options[:oversized].present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I think we should look at what this is describing. It's a carrier's view of physical rating characteristics of a package.

e.g. for Canada Post it might be

options = {
  rating_characteristics: [:oversized]
}

Where as for USPS it might be

options = {
  rating_characteristics: [:oversized, :cubic]

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit wordy, but best describes it since characteristics is too vague.

I'm okay with rating_classifications as well.

end
alias_method :cm, :centimetres

def weight(options = {})
case options[:type]
when nil, :actual
@weight
when :volumetric, :dimensional
@volumetric_weight ||= begin
m = Measured::Weight.new((centimetres(:box_volume) / 6.0), :grams)
@weight_unit_system == :imperial ? m.convert_to(:oz) : m
end
when :billable
[weight, weight(:type => :volumetric)].max
end
def unpackaged?
@unpackaged ||= @options[:unpackaged].present?
end
alias_method :mass, :weight

def self.cents_from(money)
return nil if money.nil?
Expand All @@ -129,40 +73,5 @@ def self.cents_from(money)
end
end
end

private

def attribute_from_metric_or_imperial(obj, klass, unit_system, metric_unit, imperial_unit)
if obj.is_a?(klass)
return obj
else
return klass.new(obj, (unit_system == :imperial ? imperial_unit : metric_unit))
end
end

def measure(measurement, ary)
case measurement
when Integer then ary[measurement]
when :x, :max, :length, :long then ary[2]
when :y, :mid, :width, :wide then ary[1]
when :z, :min, :height, :depth, :high, :deep then ary[0]
when :girth, :around, :circumference
self.cylinder? ? (Math::PI * (ary[0] + ary[1]) / 2) : (2 * ary[0]) + (2 * ary[1])
when :volume then self.cylinder? ? (Math::PI * (ary[0] + ary[1]) / 4)**2 * ary[2] : measure(:box_volume, ary)
when :box_volume then ary[0] * ary[1] * ary[2]
end
end

def process_dimensions
@dimensions = @dimensions.map do |l|
attribute_from_metric_or_imperial(l, Measured::Length, @dimensions_unit_system, :centimetres, :inches)
end.sort
# [1,2] => [1,1,2]
# [5] => [5,5,5]
# etc..
2.downto(@dimensions.length) do |_n|
@dimensions.unshift(@dimensions[0])
end
end
end
end
Loading