Unspecified behavior of Money::allocate()

[Sharom]

What is correct behavior for each of these examples?

/* Negative amount */
(new Money(-5, new Currency('USD')))->allocate([1, 1]); // Now returns [-2, -3]
(new Money(-5, new Currency('USD')))->allocate([3, 7]); // Now returns [-1, -4]
(new Money(-5, new Currency('USD')))->allocate([7, 3]); // Now returns [-3, -2]

/* Negative ratio */
(new Money(5, new Currency('USD')))->allocate([-3, 7]); // Now returns [-3, 8]
(new Money(5, new Currency('USD')))->allocate([-3, -7]); // Now returns [2, 3]

I guess that ratios mustn't contain negative value, and we should throw an exception if it does.

What about negative amount of money?

[frederikbosch]

You are right. Negative ratio should throw an exception. Good question on the the negative money, I think the generated values are incorrect.

(new Money(-5, new Currency('USD')))->allocate([3, 7]); // Now returns [-1, -4]
(new Money(-5, new Currency('USD')))->allocate([3, 7]); // Should returns [-4, -1]

Right? We could use the recently added method absolute to fix this I guess.

[Sharom]

We could use the recently added method absolute to fix this I guess.

@frederikbosch Current behavior subordinate to mathematical logic. But I am not sure that this behavior is correct from financial side.

[frederikbosch]

@Sharom See PR #260. It fixes both issues. Could you have a look and see if this correct behaviour now?

[frederikbosch]

I did not use absolute by the way. That was not helpful in this case.

[tabennett]

I know this is an old issue, but shouldn't the first example of a negative ratio be allowed? As long as there's always a single positive ratio, it shouldn't matter if the other ratios are negative. This should really be up to the specific domain implementation to guard against these types of invariants. In our case, we have valid reasons to allow for ratios to be negative (as long as at least one of them is positive). In our domain, we have logic that prevents nonsensical situations where all ratios would be negative, however we can't use this object for the valid cases because of the InvalidArgumentException being thrown.

[frederikbosch]

@tabennett I am willing consider your idea. Just to understand why the library should not be the guard against negative allocations, can you at least give me one example in which you would consider this possible or even required?

[tabennett]

We've built an app that processes utility bills at scale. Part of this involves simplifying the bills for our clients as well as catching billing mistakes on their invoices. We break bills up into service charges and fees. Service charges are charges for base services provided (e.g a trash service) whereas fees are extra line items that the service provider often put on the invoice (e.g installation, fuel, etc).

We use allocation for the following:

• Roll the fees (mentioned above) into the service charge so that the client has a simplified view.
• Validate fee and service charge amounts against a contract with have with the provider (this can be a percentage as well as a unit-based amount).

Both of those operations require us to allocate fees onto service charges. This is done as follows:

// Calculate the total sum of all service charges ($serviceChargeTotal will be a new money object).
$serviceChargeTotal = $invoice->serviceCharges()->sumMoney('amount');

// 2. For each service charge, generate a ratio that contains the percentage of that charge to the service charge total.
$ratios = $invoice->serviceCharges()->map(fn($serviceCharge) => $serviceCharge->getAmount() / $serviceChargeTotal()->getAmount());

// 3 For each fee on the invoice, allocate the portion of the fee that was charged for each service:
$feeAllocations= $invoice->fees()->map(fn($fee) => $fee->allocate($ratios->all()));

Once we have those fee allocations we can add them back into each individual service charge. This let's us know the true cost of the service (for validation purposes) and greatly simplifies the invoice because the fee line items can be removed.

The problem we have now is that we will often have service charge credits (negative line item amounts) on an invoice (due to us catching billing errors from previous invoices) and we need to be able to treat these just as we would a positive service charge (i.e we roll fees back into them so that we can validate the invoice and remove fees from it). Right now we can't currently do this because it would required being able to allocate with negative ratios.

We're currently using a fork of this library in order to allow us to process invoices with credits. Here's what our fork has in the allocate method. This (or something similar) is the change I'm proposing:

       $nonNegativeRatios = array_filter($ratios, function($ratio) {
            return $ratio >= 0;
        });

        if (count($nonNegativeRatios) === 0) {
            throw new \InvalidArgumentException('Cannot allocate to none, ratios must contain at least one non-negative value');
        }

        foreach ($ratios as $key => $ratio) {
            $share = $this->getCalculator()->share($this->amount, $ratio, $total);
            $results[$key] = $this->newInstance($share);
            $remainder = $this->getCalculator()->subtract($remainder, $share);
        }

Here we're allowing negative ratios to be allocated as long as there is at least one non-negative value in the ratios array. In our Domain, we will never have a vendor invoice that doesn't contain at least one positive service charge (our invoice entity can't even be instantiated). So this check really isn't necessary for us.

This is what I'm referring to when I propose that it's not the Money object's responsibility to guard such invariants; It really should be left up to the specific domain. If someone is allocating and all of their ratios are negative then they're probably going to have other problems with their code (all negative ratios is nonsensical in that it's no longer an allocation operation at all, mathematically speaking). I would even go so far as to propose that this library not worry about this kind of thing at all; I'm not away of an implementation of this pattern in any other programming language that sees it as the Money object's responsibility to guard these kind of invariants.

If you would like a real world example (with real line items and amounts) I'd be more than happy to provide one. I think I should be able to dig through some of our older invoice records and provide something succinct to prove my case.

[frederikbosch]

@tabennett Thanks for the explanation. On second hand, I agree that we should not guard for negative amounts at all. Are you able to create a PR for this?

[tabennett]

I've created a PR for this: #623