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

Prices and percentages in entities #217

Open
krmarien opened this issue Sep 14, 2014 · 10 comments
Open

Prices and percentages in entities #217

krmarien opened this issue Sep 14, 2014 · 10 comments

Comments

@krmarien
Copy link
Member

See comment dc5550f

@bgotink
Copy link
Member

bgotink commented Sep 15, 2014

My prosal can be found in the commit comments.

We should take into account that amounts are stored in different ways now:
Most are stored x100 in the database, some are stored in config x10,000.
(I might have missed other configurations)

These will have to be fixed in an update script.

@krmarien
Copy link
Member Author

How are you going to fix this? Store everything x10,000?

@bgotink
Copy link
Member

bgotink commented Sep 15, 2014

Either that, or make it configurable. The latter is perhaps the best option, as it means we don't need any schema updates or update script for the database:

class Amount
{
    private $amount;

    public function getFloat($times = 4)
    {
        return $this->amount / pow(10, $times);
    }

    public function setFloat($value, $times = 4)
    {
        $this->amount = (int) ($value * pow(10, $times));

        return $this;
    }
}

// usage:

class MyEntity
{
    private $myValue;

    public function getMyValue()
    {
        return $this->myValue->getFloat(2);
    }

    public function setMyValue($value)
    {
        $this->myValue->setFloat($value, 2);

        return $this;
    }
}

@krmarien
Copy link
Member Author

krmarien commented Oct 4, 2014

I implemented a first version of this with an example (963db5b)

But I'm not yet convinced of the functions in the entity itself. Would it be better to return the amount object itself? Now I have a return {float value} and a return {integer value} function.

@bgotink
Copy link
Member

bgotink commented Oct 4, 2014

I would hide the fact that we're using the Amount class, but that's personal preference ;)

@krmarien
Copy link
Member Author

krmarien commented Oct 4, 2014

So like it is now but with the update of the $times from the comment in the commit?

@bgotink
Copy link
Member

bgotink commented Oct 4, 2014

I'll take a more detailed look tomorrow before deciding whether I'm completely happy with the current implementation 😉

@krmarien
Copy link
Member Author

krmarien commented Oct 5, 2014

The implementation with $times is not that easy, because the scale is configurable.

@bgotink
Copy link
Member

bgotink commented Oct 5, 2014

I've made some changes to the proposed code.
These changes allow us to e.g. set the price of an event as an integer to value 2700, which is 100 times the value. The caller of the method doesn't need to know what multiplier is used to store the amount, so this multiplier can be changed without other code needing a refactor.

@krmarien
Copy link
Member Author

krmarien commented Oct 5, 2014

I will work further on this with the latest changes of dev-form (otherwise we would get a merge headache)

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

No branches or pull requests

2 participants