Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: QoL improvement, optionally apply helpers like negated or abs with fluent API #84

Open
kylemilloy opened this issue Feb 3, 2025 · 1 comment · May be fixed by #85
Open

Comments

@kylemilloy
Copy link

kylemilloy commented Feb 3, 2025

Use case

In some cases where using the current syntax I'd like to optionally apply negation of absolute values to something without having to break out of the current builder syntax. For example:

return $number->multipliedBy(3)
  ->dividedBy(2)
  ->negated(fn (BigNumber $number) => $number->toInt() % 2 === 0)
  ->add(5)
  ->toInt();

The alternative to this is a little more cumbersome with:

$number = $number->multipliedBy(3)
  ->dividedBy(2);

if ($number->toInt() % 2 === 0) {
  $number = $number->negated();
}

return $number->add(5)->toInt();

The idea here is that I only sometimes want to negate the value (in my arbitrary example, if it's even). In addition to closures, it would be nice if this could take bools as well if higher up the stack we are doing a bit of logic that will decide if it should be negated. This would also be useful in abs() as well where sometimes we don't necessarily want to return the absolute value.

My goal for this would be to bring this into brick/money after this gets approved as I find myself most often requiring these helpers when dealing with transforming dual-entry accounting that our finance system runs in into something more user friendly when showing a balance.

Approach

I'll write a PR for this. My approach would be to create a helper to satisfy if the passed value should resolve. In quick pseudo-code this would look like:

class BigInteger
{
    public function abs(bool|Closure $abs = true): BigInteger
    {
        if ($this->resolveOptionalClosure($abs)) {
            return $this->isNegative() ? $this->negated() : $this;
        }

        return $this;
    }

    public function negated(bool|Closure $negated = true): BigInteger
    {
        if ($this->resolveOptionalClosure($negated)) {
            return new BigInteger(Calculator::get()->neg($this->value));
        }

        return $this;
    }

    protected resolveOptionalClosure(bool|Closure $cb): bool
    {
        return $cb instanceof Closure ? (bool) $cb($this) : $cb;
    }
}

And this would be applied out to each class...Decimal, Rational, etc etc. If we're happy with this feature I will write a PR here and for the brick/money library as well

@BenMorel
Copy link
Member

BenMorel commented Feb 7, 2025

Hi @kylemilloy!

Unfortunately I don't think this feature is a good fit for brick/math in its current form: IMO it complicates the API and increases the cognitive load for other users, for a use case that I would consider niche. Plus, if we were to implement this, I think it would be inconsistent to only apply it to abs() and negated(): it could very well be applied to all operations, including plus(), minus() etc.

I do understand where you're coming from though, and in your specific use case, this API does make the code a bit more readable.

One solution could be to create your own BigInteger class that wraps brick/math's BigInteger, and add your own methods there.

Another solution would be to add a single generic method, like apply(), that would take a Closure which accepts and returns a BigNumber, allowing to modify the current number without leaving the fluent API:

return $number->multipliedBy(3)
  ->dividedBy(2)
  ->apply(fn (BigInteger $number) => $number->toInt() % 2 === 0 ? $number->abs() : $number)
  ->apply(fn (BigInteger $number) => $shouldNegate ? $number->negated() : $number)
  ->add(5)
  ->toInt();

Its implementation would just be:

public function apply(Closure $closure): BigNumber
{
    return $closure();
}

I'm not totally convinced about it, but as I understand it, it would solve your problem. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants