-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
There are equals method? #17
Comments
Very good point. This method throws an exception because there's no way to tell whether 2 amounts in different currencies are equal, without an exchange rate; but your use case is valid as well, one might want to check whether amount and currency are equal. We have at least 3 options here:
@solodkiy Can you tell me a bit more about your use case: in what situation do you need to check that both the amount and currency are equal? @jiripudil @jkuchar @VasekPurchart Do you have an opinion here? |
I write system to analyse transactions from the bank. For it I need to compare transactions with each other. Transactions are equals if their amount, and other data equals too. I used moneyphp/money before. This lib use first option: equals always return bool, compare can throw exception. https://github.com/moneyphp/money/blob/master/src/Money.php#L128
In Money object you don't need to think about exchange rate at all. You have special class MoneyComparator fort this task. |
Small research about naming: "function equals(" "function isequal(" |
I know So my worry with At first glance I'd say that adding an Still, I'm a bit worried about having two |
I was thinking (but did not manage to reply yet) the same - two methods with similar names and vastly different behaviours would be really confusing. And I think that I came up with |
Another acceptable name would be
Bonus question: should this method check that the contexts are identical as well? I would say no, but then the naming is, once again, confusing. |
I actually thought about a name that might be more suitable: Is everyone happy with this name, before I tag a release? |
I still think it can confuse people. But it solve my problem. |
To be honest I would not ever expect $money->isEqualTo() to do anything with currency conversion. When I would do that I would like to use special comparator which would compare money with more knowledge it has. When I write it on paper it what feels natural to me:
The thing here is that when comparing different currencies, environment (outside world) is stepping in and it is up to model how precise they want to compare them (you have have realtime forex provider, than it does not make much sense), they will be equal one time and few milliseconds later they will not.
I would think of equality as logical equality and identity as it works generally with value objects (identical when all fields are the same) |
@jkuchar Are you suggesting that I'm not really against the idea, but I have two remarks:
Unless someone has a valid use case, I currently don't see the point of adding a method that compares everything, including the context. This is not what's been asked here. |
No feedback here? I'd like to move forward with this issue. |
Me too, but I have only yet used this library in a single-currency environment, in which the current and proposed behaviour is essentially the same. Hence my view might be skewed, but I'm in favour of:
But then again, I lack first-hand experience with multiple currencies. I'd love to hear from someone who's been using this library in a multi-currency setup. |
I'm quite busy right now, I will get back to this this weekend. |
I'm not experienced but will throw my 2 cents. AFAICS, I see two ways:
I favor the second option as it will not break compatibility and will make things easier and explicit. As to the wording, I think |
I am for:
|
@jkuchar Looking forward to your feedback ;) |
@BenMorel what is the progress so far? |
Ok. I think all have been said and all I have to do is wait too...! |
To add some personal feedback:
|
I agree that is makes sense, at least by default, to keep
It would be very surprising to see It would be equally surprising to see It's also comforting to see that Java's MonetaryAmount.isEqualTo() does the same:
So let's summarize, once again, our options: Option 1: add a boolean parameter to
|
@BenMorel Thanks for nice summary. I'm really happy that his issue is still open, despite my long weekend (joke). I would like to add my two cents that come up to my mind while I was reading this whole thread again. Is comparison and equality the same thing? Therefore should they behave the same?Let's dive into definitions a little. Interesting is that part which says
(I would consider Money object as pair of currency and decimal value) This means - if we start with all values This implies that However if we are comparing values (greater then, lower then, ...) we need to have both values coming from the same class of equivalence, otherwise SummaryAs there is a fundamentail difference between equality and inequality operations, so it makes sense to me to respect nature of these operations so:
One more
|
Interesting point that equality and comparison is not the same, though I'm not sure most people would intuitively understand it that way without reading the docs. It would be acceptable to make isEqualTo only return true/false, and the others throw an exception. I know this thread related to "equals" specifically, but this morning I started thinking that if
Agreed. Option two would make the most sense, in addition to other comparisons methods that do market price conversions (if you believe that is within the scope of this project--not sure you want to go that route.) |
@jkuchar Thanks for your reply, I hope you enjoyed your weekend! 🙂 You definitely pushed the discussion to the next level here.
If I understand you correctly, the interesting part in the quote from Wikipedia is actually "It does not say that one is greater than the other, or even that they can be compared in size" (emphasis mine). So your reasoning is that 1 USD and 1 EUR cannot be compared in size, but can still be compared for equality (not equal). This reasoning definitely makes sense, however my worries from my previous post still hold; the most prominent one being that if you stop throwing exceptions on different currencies in Also, when I think about the way I use Money in my own applications, most of the time I do want an exception if I accidentally compare two monies with different currencies for equality! So even though your suggestion would solve the op's use case, it would leave me without a method for my use case, so back to square one, we'd still need another method or a boolean parameter! In summary, I don't think changing the behaviour of
You're most probably right about exception management. An unchecked exception would make sense when comparing monies with different currencies, as this likely represents an error in your program, and less likely represents an exception you'd catch in a normal program flow. This is a topic of its own though, especially with the lack of definition of checked and unchecked exceptions in PHP, therefore I encourage you to open another issue if you want to discuss this further.
Fully agree that we shouldn't be comparing the context; I had to ask the question though. @eblanshey I appreciate your feedback as well!
I don't think someone could argue that for inequality methods, as you have no way to say which one is greather than the other, without currency conversion, which is outside the scope of the Money object itself.
I still believe that indeed, even though @jkuchar's comment made me think about it for a little while.
It's definitely in the scope of this project, and is already there, see MoneyComparator! |
The more I think about it, the more I like the original suggestion of
Sigh... |
|
Maybe something like |
Also I think that the best decision is have one equals method, without exeption. Agree with @jkuchar. |
As I said above, my own expectation in my apps is to get an exception if I accidentally compare two monies with different currencies (my use case is probably very different from yours), so we need a method for that, too. What would you suggest then? |
Can you tell more about cases when you need this compare? Do you catch this exceptions? What do you do in catch block? What will happen with your app if method reurn false without exeption? Maybe assertion will cover your case? |
Sorry, I misunderstood the issue here. In addition to the isEqualTo method comparing currencies I thought OP also wanted to be able to compare the values using exchange rates ¯_(ツ)_/¯ Don't ask how I derived that. Considering just comparing if currency and value are the same, I'm personally indifferent as long as it is well documented.. Everyone else has their own intuitive idea of how it should work and you can't please everyone :) |
I never catch these exceptions. When I do
I wanted to suggest the same thing for your use case: if ($money->isSameCurrencyAs($other) && $money->isEqualTo($other)) { ... This would also solve your problem, but I didn't suggest it because I thought that calling two methods when one could do would make the code unnecessarily verbose. So the same goes for your suggestion: I don't want to pollute my code with additional
I don't like this idea at all: when your app mostly deals with monies in a single currency; this adds a level of complexity that you really don't need! |
Sure, as long as it's documented it's fine. It looks like the only way to please everyone here, naming conventions aside, is to create two methods. Let's just find a name that is meaningful enough to keep confusion to a minimum? |
I do not like this idea as you are bringing your domain-specific usage into general-purpose library. So I’m still for strictly distinguishing between comparision and equality. (see general equals method later in this post) Values from different classes of equivalence as simply un-equal-able without addition knowledge (currency convertor). So I would be strict on Money object and provide whole new API for manipulating Money objects multi currency environment. (but do not want to discuss it here as it can bring lot of complexity here as converting currency pairs in different directions does not have to lead into the same result as spread exists in the market and it can depend on amount size and hevily on time and this breaks consistency rule of java equals).
It is descriptive, but I do not like this as it just describes implementation not the domain concept. It is qualitatively the same thing as $100czk->getAmount()->equals($5eur->getAmount()) && $100czk->getCurrency()->equals($5eur->getCurrency())
I still do not get what kind of usage that is. Please, could you elaborate on this more? (maybe I have partly answered this question later) API overviewAs there is still not clear attitude what outcome should be - I’m going to evaluate on Money API more. I will work with there two objects: $czk100 = Money::of(100, Currency::ofCountry('czk'));
$eur5 = Money::of(5, Currency::ofCountry('czk')); These methods does not allow cross-currency operations: $czk100->plus($eur5); // : ❌ usage exception
$czk100->minus($eur5); // : ❌ usage exception
$czk100->multipliedBy($eur5); // : ❌ usage exception
$czk100->dividedBy($eur5); // : ❌ usage exception
$czk100->isGreaterThan($eur5); // : ❌ usage exception
$czk100->isLessThan($eur5); // : ❌ usage exception
$czk100->isGreaterThanOrEqualTo($eur5); // : ❌ usage exception
$czk100->isLessThanOrEqualTo($eur5); // : ❌ usage exception Equals methodAnd equals is different: $czk100->equals($czk100); // true
$czk100->equals($eur5); // false
$czk100->equals('hello!'); // false
$czk100->equals(new \DateTimeImmutable()); // false from Java docs
According to Java docs: There is no possibility of throwing exception as all objects are sort-of equal-able. For example Date(2018-11-27) is not equal to Money(100, CZK). And I definitely cannot say that Date(2018-11-27) > Money(100, CZK) Problem we are solving: Is
|
@jkuchar Thanks for your detailed post, lots of interesting insight, in particular Java's definition of
That's not my intention. I was attempting to explain that, even though the current behaviour of
That's exactly what the library does at the moment: throwing exception when comparing different currencies, whether this is for inequality or equality.
Did you see above that we already have a MoneyComparator implementation?
Simple: some parts of my code deal with multi-currencies, and some other parts deal with the assumption that all monies are of the same currency. If this assumption is broken by a defect in my code, leading to a leak of a Money with a different currency, I'd like to get an exception when I perform an
A similar suggestion has been made in brick/date-time#9, with no feedback from the OP so far. I do not quite like this approach. In Java this makes sense, as this is a language requirement that all objects must be comparable to each other. In a specific PHP library however, I don't see any value in providing a generic I like to build libraries around use cases: what's a legitimate use case for such a comparison? If there is none, then this "feature" should not be built into the library IMO.
Exactly, and we have so far 2 equally valid use cases, that only differ in how they handle equality on 2 monies with different currencies:
I still think that
I would not break this consistency, but I'd rather add a new method for the OP's use case, method for which we don't seem to agree on a good name. What do you suggest, exactly, @jkuchar? |
I haven't been suggesting anything in particular. I have just been trying to put all arguments that I had on my mind. (yep, sometime contradicting each other 😃) I just ended with that there are really two use-cases.
Yep, it totaly makes sense to make these copariosons external as they are more conserned with coverting then comparing values. After thinking again about this I agree that:
For me it would makes sense to implement |
Implementing comparisons for collections is yet-another-use-case I think, as now it's questionable whether you should compare contexts as well? |
I don't have a strong opinion, but to add an option to the discussion: if you want to go the "two separate methods" route but don't want to be verbose, you can use PHPunit's method: it has "assertEquals" and "assertSame" for strict checking. Perhaps keep "isEqualTo" as it is, and make "isSameAs" always return true or false. |
I've already suggested |
looks like discussion is't moving to conclusion... |
I guess pretty much everything has been said: we just need to find a reasonable name for the new method. |
Mine goes to |
My ordered list of favorites:
|
Any other votes out there, @solodkiy, @VasekPurchart, @jiripudil, @eblanshey? |
While |
I tend to lean towards |
Is If considered a VO, the |
I'm not sure whether our Money objects can be considered VO as per Fowler's definition. They are immutable, don't have an identity, and can be compared by value, but comparison should exclude context indeed, so they probably don't qualify as VO. We can't really compare brick/money with moneyphp, as the latter does not support, AFAIK, custom scales, cash roundings, etc., so they don't need a context at all, and more naturally qualify as VO. We can't just have an I have the same feeling regarding moneyphp's API: their inequality methods ( |
PollOK let's do a real poll on 3 options, as I don't want to introduce a bias by asking to vote on a new method name only. I'd like everyone here to vote please. If you don't care, post "I don't care".
You can vote for just 1, 2 or all 3 options, in your order of preference. If you vote for option 3, add one or more method names in your order of preference. Vote template:
Note that I'm not planning to rename |
Option 1 Still think that exception in equals is side effect and you don't need to rely on it. If the same currency is important for you you should throw exception even before comparison right after input.
|
As already stated: Option 3 and |
Option 3 and |
Happy new year! 🎉 I'd like to revisit this issue and fix it once and for all. Re-reading the thread more than one year later, I'm now torn between:
What lead me to revisit the second point is @jkuchar's comment above, in particular:
My issue with this, however, is twofold:
Some fresh thoughts? |
It still makes sence to me what I have written. Equality and comparison are two different things to me. Imagine you are adding |
What do you with the |
As we're not getting anywhere here, I've finally released 0.4.4 with a new method: This may or may not change in the future; I understand that this solution does not make perfect sense from a mathematical standpoint, but it's practical, non-breaking, and the other proposed solution has its inconsistencies, too (at least IMO). And after nearly 2 years without a concensus, I prefer having something non-perfect, than having nothing at all. I'm closing this issue now, but I'm open to comments, in this thread, or preferably in a new one, if someone wants to bring new arguments to the table. Thanks to all for your participation. |
I need a simple bool method what answer me are the two money object completely equal (amount and currency) or not.
isEqualTo looks like this method, but it throws MoneyMismatchException exception if currencies are different.
What I should use for this problem?
The text was updated successfully, but these errors were encountered: