Advanced calculations amongst Money objects casts them to string

[madsem]

Hey Ben,

first off I wanna thank you for creating a lot of amazing packages, which I am using a lot! :) Very helpful to me.

Now, I'm unsure if this is by design or considered a bug/improvement. But I tried doing advanced calculations between multiple AbstractMoney objects, and it results in them being cast to string which ofc breaks the entire calculation.

Here is an example that calculates the return on ad spend for advertising campaigns:

// $this->revenue & $this->spend are both Money objects.

/**
     * Calculate Return on Ad Spend as Percentage Value
     *
     *
     * @return int
     */
    private function returnOnAdSpend(): int
    {
        $revenue = $this->revenue->toRational();

        // prevent division by zero
        if ($revenue->isNegativeOrZero() && $this->spend->isNegativeOrZero()) {
            return 0;
        }

        $returnOnAdSpend = $revenue->dividedBy($this->spend)->multipliedBy(100);
dd($returnOnAdSpend); // App\Exceptions\MoneyException : The given value "USD 285.00" does not represent a valid number.
        return $returnOnAdSpend->getAmount()->toInt();
    }

Now if I change the $returnOnAdSpend calculation to this:

$returnOnAdSpend = $revenue->dividedBy($this->spend->getMinorAmount()->toInt())->multipliedBy(100);
dd($returnOnAdSpend);

I get this:

Brick\Money\RationalMoney {#2581
  -amount: Brick\Math\BigRational {#2550
    -numerator: Brick\Math\BigInteger {#2574
      -value: "1900000"
    }
    -denominator: Brick\Math\BigInteger {#2602
      -value: "330000"
    }
  }
  -currency: Brick\Money\Currency {#2588
    -currencyCode: "USD"
    -numericCode: 840
    -name: "US Dollar"
    -defaultFractionDigits: 2
  }
}

Am I using it wrong? What is going on? :)

edit: Thought it would be good to mention that I'm using Brick/Money in a Laravel 7 app, and also have created a custom cast to cast values to/from Money:

<?php

namespace App\Casts;

use Brick\Money\Money;
use Brick\Money\AbstractMoney;
use Illuminate\Contracts\Database\Eloquent\CastsAttributes;

class MoneyCast implements CastsAttributes
{

    /**
     * Cast the given value.
     *
     * @param  \Illuminate\Database\Eloquent\Model  $model
     * @param  string  $key
     * @param  int  $value
     * @param  array  $attributes
     *
     * @return Money
     * @throws \Brick\Money\Exception\UnknownCurrencyException
     */
    public function get($model, $key, $value, $attributes)
    {
        return Money::ofMinor($value, $this->loadCurrency());
    }

    /**
     * Prepare the given value for storage.
     *
     * @param  \Illuminate\Database\Eloquent\Model  $model
     * @param  string  $key
     * @param  Money|float  $value
     * @param  array  $attributes
     *
     * @return int
     */
    public function set($model, $key, $value, $attributes)
    {
        if ($value instanceof AbstractMoney) {

            return $value->getMinorAmount()->toInt();

        }

        return Money::of($value, $this->loadCurrency())->getMinorAmount()->toInt();
    }

    private function loadCurrency()
    {
        return $model->account->currency ?? config('settings.currency');
    }

}

[madsem]

Added a tinker test, without any MoneyCast in-between, but same result:

>>> use Brick\Money\Money;
>>> $rev = Money::of(1000, 'USD'):
PHP Parse error: Syntax error, unexpected ':' on line 1
>>> $rev = Money::of(1000, 'USD');
=> Brick\Money\Money {#3413}
>>> $spend = Money::of(500, 'USD');
=> Brick\Money\Money {#3417}
>>> $returnOnAdSpend = $rev->dividedBy($spend)->multipliedBy(100);
Brick/Math/Exception/NumberFormatException with message 'The given value "USD 500.00" does not represent a valid number.'

[BenMorel]

Hi 👋

The issue is that you can't (by design) divide a Money by a Money. Even if you could (assuming they're in the same currency), you wouldn't get a Money back, you'd get a number!

Does that make sense?

[madsem]

Hey @BenMorel

sorry for late reply, so busy atm. Yes, that makes perfect sense now that you explained it.

I guess I was expecting it to accept other Money instances because the comparison etc is also possible amongst instances. But without thinking it through... :)

But on the other hand it would make sense if this would be doable, calculations could create a new Money because that is essentially what happens.

Thoughts?

[BenMorel]

Definitely not a Money as a result: dividing 10 USD by 2 USD gives you 5, not 5 USD.

I could add a method to divide a Money by a Money, but then the question is: what do you expect as a result? A BigDecimal? Which scale? What about rounding? A BigRational? Why not, but this might be unexpected to some people.

To summarize, I would advise to get the amount and do the division yourself, at least you can specify the scale & rounding mode, or convert to a BigRational if necessary:

use Brick\Math\RoundingMode;

$revenue->getAmount()->dividedBy($spend->getAmount(), 2, RoundingMode::DOWN);

[madsem]

Hey,

the more I thought about it, the more I realize it's really the best way it works now.

It's not given that calculations expect a monetary result, it could be a percentage or anything really. So yeah, a Money instance would definitely be wrong.

Sorry for the confusion, it was just not obvious to me and I got a little confused haha. Thanks again for the package, it's really fun working with it.