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

Careful with string vs int #133

Closed
frederikbosch opened this issue Jan 12, 2016 · 7 comments
Closed

Careful with string vs int #133

frederikbosch opened this issue Jan 12, 2016 · 7 comments

Comments

@frederikbosch
Copy link
Member

Now that there are no more integer limitations (#115), we have to be careful with int and string usage vs strict comparison. This is something that I did not cover in my PR but now realize that should be taken care of. Especially with these lines.

These lines are now buggy, because the following will return false.

(new Money(1, new Currency('EUR')))->equals(new Money('1', new Currency('EUR')))

I guess we should convert the amount to a string in the constructor with (string). PR is on the way.

@frederikbosch
Copy link
Member Author

PR created. I was partially right: isZero, isPositive and isNegative were fine. Nonetheless added a test for them. equals was not fine since the line was changed in #131. However, I do agree, strict comparison is better than loose comparison.

@sagikazarmark
Copy link
Collaborator

Is this a good idea with the PhpCalculator

@frederikbosch
Copy link
Member Author

Good question, let's test it.

@frederikbosch
Copy link
Member Author

Added another commit to the PR. The PhpCalculator now returns string where required.

@sagikazarmark
Copy link
Collaborator

So calculators return strings and integers based on what?

@frederikbosch
Copy link
Member Author

Calculators return an integer in the compare method, so 1, 0 and -1. Other methods return a string now.

@frederikbosch
Copy link
Member Author

Fixed in #136.

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