Rewrite allocate/split

[antstorm]

This PR introduces a more effective and uniform solution to the #allocate and #split methods. These are now synonyms accepting either array for allocations or a number for even splits. The solution does not differ between the two approaches (allocate(3) == allocate([1, 1, 1])).

It is somewhat similar to the former #allocate_from_splits method, however for every part that is being processed we measure it's ratio against the sum of the remaining parts (not the whole thing). This will always give us the ratio of 1 for the last element.

It also adds a couple of tests to make sure it works as expected, since there were some inconsistencies with the former solution.

Splitting BigDecimals into repeating decimals (as in fact any other use case) now adds back up to the original amount.

This change should be backwards compatible apart from the fixed cases of allocating big decimals (assuming it wasn't being used much since no issues were reported).

[coveralls]

Coverage decreased (-0.6%) to 99.27% when pulling d08d6a4 on antstorm:rewrite-allocate into 1f1c8c7 on RubyMoney:master.

[coveralls]

Coverage decreased (-0.03%) to 99.891% when pulling 3a5bb8b on antstorm:rewrite-allocate into 1f1c8c7 on RubyMoney:master.

[coveralls]

Coverage decreased (-0.6%) to 99.27% when pulling d08d6a4 on antstorm:rewrite-allocate into 1f1c8c7 on RubyMoney:master.

[antstorm]

@printercu would love your opinion on this one if you don't mind

[ct-clearhaus]

until remaining_parts.empty? do perhaps?

[printercu]

while x.any??

[antstorm]

FYI, .any? is an Enumerable method that loops over the collection, avoiding

[ct-clearhaus]

I think the strategy is brilliant! 🏆

I tried to come up with a "less imperative" version, but could only golf it as far as this (which I don't really like because of the indexing mess):

    def generate
      remaining_amount = amount

      parts.reverse.map.with_index do |part, i|
        current_split = remaining_amount * part / parts[0..(-i-1)].inject(0, :+)
        current_split = current_split.truncate if whole_amounts

        remaining_amount -= current_split

        current_split
      end.reverse
    end

[printercu]

Should .ceil instead of .truncate allow to avoid double reverse?

[ct-clearhaus]

Not bad thinking! However, the reverse is there to leave the "extra pennies" (that are left "in the end" of the map) to the first parts.

For instance, if we are to divide amount = 10 into parts = [1,1,2] (spec in line 83); the map "loops" would have this before subtracting from remaining_amount:

[part, i, remaining_amount.to_s, current_split.to_s]
[2   , 0, "10"                 , "5"]
[1   , 1, "5"                  , "2"]
[1   , 2, "3"                  , "3"]

Or, slightly more intricate, if we divide amount = BigDecimal(10) into parts = [1,1,1] with whole_amounts = false (spec in line 105):

[part, i, remaining_amount.to_s      , current_split.to_s]
[1   , 0, "0.1E3"                    , "0.33333333333333333333E2"]
[1   , 1, "0.66666666666666666667E2" , "0.333333333333333333335E2"]
[1   , 2, "0.333333333333333333335E2", "0.333333333333333333335E2"]

By removing both reverses and replacing truncate with ceil (and replacing parts[0..(-i-1)] with parts[i..-1] and #to_f-ing the division because integer division by default truncates) we get

[1, 0, "10", "2"]
[1, 1, "8", "2"]
[2, 2, "6", "6"]

which is wrong (according to the spec that the extra pennies to go to the first parts) and

[1, 0, "0.1E3"                    , "0.33333333333333333333E2"]
[1, 1, "0.66666666666666666667E2" , "0.333333333333333333335E2"]
[1, 2, "0.333333333333333333335E2", "0.333333333333333333335E2"]

which is also wrong because it's not afterwards reversed; thus, the last two parts receives the extra.

In other words, if the spec was to send the extra pennies to the last, you'd be right.

[printercu]

Please take a look: https://gist.github.com/printercu/f9dbde27172ca4a57533d1dcdd95f2a3

It works the same way: greater values appear in the begining. But it gives different results, and it's not clear for me which is better. For example, split 10 into 1, 1, 2: [3, 2, 5] or [3, 3, 4].

[antstorm]

To be honest with you, I'm not sure what's the best approach here. Unless you guys have a strong preference to change the leftover penny distribution, I think it makes sense to stick to existing spec and keep the old tests — this allows us to put this in a minor release.

Also, note:

split 9.0, [1, 1, 2]
=> [3, 2, 4]
split2 9.0, [1, 1, 2]
=> [3, 2, 4]

So it seems like the decision here is to either favour a distribution of the result closer to the given parts or closer parts / amount ratio. And it kinda makes sense to have this behaviour selectable by user.

[antstorm]

One more note:

split 10.0, [1, 1, 2]
=> [3, 2, 5]
split2 10.0, [1, 1, 2]
=> [3, 3, 4]

split 10.0, [1, 2, 1]
=> [3, 5, 2]
split2 10.0, [1, 2, 1]
=> [3, 5, 2]

split 10.0, [2, 1, 1]
=> [6, 2, 2]
split2 10.0, [2, 1, 1]
=> [5, 3, 2]

I think this suggests that .truncate is more consistent, always adding the leftover penny to the first party.

[ct-clearhaus]

(...) and it's not clear for me which is better. (...)

Me neither. I expect it's possible to find cases where the cumulative relative error will favour any of the two. If that's correct, then it's just an arbitrary choice between the two, and in that case, I agree that the current spec should be favoured.

I guess an alternative strategy to the double reverse could be "Work your way from below and mirror around zero", say:

• "From below": remaining_amount = -amount and += (rather than -=; for negative numbers, ceil is the same as truncate so choose either)
• "Mirror": .reverse.map{|x| x+amount}

I suspect the performance is quite alike.

Speaking about performance, as long as the complexity is in terms of parts.length I wouldn't care much. If users have "too long parts" then they may optimise and contribute 😉

I wonder if anyone is actually using this split method? I notice that https://github.com/carlospalol/money/ does not have such a thing 🙂

[antstorm]

No idea if anyone is actually using this method :) I can definitely see how it can be extremely useful in certain situations, but that's definitely not your typical scenario.

[printercu]

.lazy before first .reverse may help avoiding actual reverses. But I don't know if it's available in all rubies supported by gem.

[antstorm]

I've modified it so it now doesn't have any .reverse calls

[printercu]

While this is so simple I would inline it with parts = Array.new(parts, 1) if parts.is_a?(Numeric) in initialize.

[printercu]

WDYT about extracting this methods to Money::Allocate and including it into Money? With @ct-clearhaus 's suggestions generate becomes clean and straightforward, so it can be used without instantiating an object and be implemented as private method or class method of new module?

[antstorm]

I see this as inheritance vs composition decision. While there's a certain appeal in including this directly into Money, I don't think there's much benefit in doing so — object instantiation is cheap in ruby (plus it's just 1 object) and it's easier to add more complex behaviour (like switchable distribution) to a separate class.

Happy to discuss this further if you feel like I'm missing something here.

[printercu]

Here is what I suggest:

class Money
  module Allocation
    class << self
     def split(amount, parts, ...)
    end

    def allocate(...)
      Allocate.split(...)
    end

    alias_method :split, :allocate
  end
end

# in lib/money.rb
class Money
  include Allocation
end

This way allocation methods are in separate module, because it's like an extended. Similar to Arithmetics. If you want you can keep a class Money::Allocation::Generator, but for now I see no pros in using it vs a static method.

[antstorm]

Don't you think this is creating a tight coupling between the two? As of the code in the PR — Money::Allocation does not depend on Money in any way. Some obvious benefits I see are:

• Being able to easier test it in isolation (no need to instantiate all the Money objects to check results)
• Allocation is not concerned with aliasing the method for backwards-compatibility (which I think it shouldn't)
• The public method is defined on the Money directly, making it easier to find (less indirection)
• Any changes to how infinite_precision or initialize work will not affect this class in any way

Correct me if I'm wrong, but what you are suggesting is essentially splitting the code between different files without adding a class boundary. Why do you think it's a better approach?

[printercu]

1. It's even easier, because there is no need to instantiate Allocation as it's now. Just Money::Allocation.split, similar to testing my example from gist.
   2, 3) This is just collecting methods together by functionality. Why there is separate Arithmetics module? Then let's move all methods to Money.

I just want to keep different functionality separate from other. Money should include only the basics. Allocations are not essential for most of users, so related methods must be kept separately. I use this test: what should i do to remove functionality? With module I need just remove include and delete file, without - look for all related methods and files and remove them one by one.

[ct-clearhaus]

+1 for "minimality".

[antstorm]

@printercu while it's less flexible, I see the reason for moving to class methods for now, I'll make this change.

As for grouping methods by functionality — yes, sometimes there is a need to do so and Arithmetics is a good example, but it also indicates that the public interface for this class is too big. In case you're familiar with SOLID design principles, it's the violation of the Interface Segregation principle — you're making collaborators depend on a bunch of stuff they don't need/use.

Normally OO design is judged by how easy it is to change things. In your suggestion the Money depends on Allocation, as well as Allocation depends on Money — adding more reasons for both to change.

I suggest we agree to disagree here. However I would appreciate some input on things that matter — see the comment at the bottom of this PR.

[ct-clearhaus]

Adding this would make the spec fail:

expect(described_class.new(10, [1, 1, 2], false).generate).to eq([2.5, 2.5, 5])

@antstorm That is expected, right?

Should we add a spec like

expect(described_class.new(10, [1, 1, 2], false).generate).to eq([3, 2, 5])

somewhere?

[antstorm]

Since it's hooked into Money, the whole_amounts is coupled to the type of the amount. I'll add a spec that you've suggested to make it intentional, but if you think like there's a point in treating the amount like a float when whole_amounts is false, I can make it work as well.

[ct-clearhaus]

I think what you made is perfectly fine! I just stumbled upon the corner case — which is kinda even an "outside (out of scope) corner" 😉

[printercu]

Storing remaining_parts_sum and subbing current part from it reduces O(n) from n^2 to n.

[printercu]

Update gist from https://github.com/RubyMoney/money/pull/772/files#r188010498 in this way.

[antstorm]

Great suggestion, thank you!

[antstorm]

@printercu @ct-clearhaus thank you both for such great comments! 💯

Here's the result I ended up with:

def generate
  remaining_amount = amount
  remaining_parts_sum = parts.inject(0, :+)

  parts.reverse.map do |part|
    current_split = remaining_amount * part / remaining_parts_sum
    current_split = current_split.truncate if whole_amounts

    remaining_amount -= current_split
    remaining_parts_sum -= part

    current_split
  end.reverse
end

However it fails the tests because in ruby 1.0 - 0.7 = 0.30000000000000004. This can probably be solved with using BigDecimal, but in my mind it's an overkill compared to a mutable array. WDYT?

P.S. I guess we'll have the same problem with summing part (the current solution), since 0.1 + 0.2 = 0.30000000000000004 :/ Can probably be solved with forcing parts to be integers, but this would also need to be a major release.