Webador / sendcloud

Provides a PHP client to interact with the SendCloud API in an object-oriented way.
MIT License
16 stars 15 forks source link

Added intval and ceil to not loose precision during weight conversion #29

Closed AlexisPPLIN closed 10 months ago

AlexisPPLIN commented 10 months ago

This pull request fixes #27.

Conversion from Kg to Grams now uses intval and ceil. I've also added this special case to the existing test of ShippingMethod.

Hope this will be merged soon !

AlexisPPLIN commented 10 months ago

This also inlines the operation again. If there are no additional steps involved I think introducing a new variable reduces readability.

I'm okay with that, see my last commit.

Isn't it better to use round() for this as it corrects float calculation imprecision both ways?:

After a bit of thinking, I think is better to use :

So we always give a valid range of weights even if round errors occurs.

villermen commented 10 months ago

So we always give a valid range of weights even if round errors occurs.

@AlexisPPLIN We're not trying to fix rounding errors but float math precision errors. We have no say in what way the imprecision will occur, but we always know it will be close to the intended result. I still think rounding inaccuracies away is the better option for this.

Consider in your proposed situation we would receive 1.001 for both the min and max weights:

var_dump(1.001 * 1000); // float(1000.9999999999999)
// Note that this might as well have resulted in 1001.0000000000001 or something close to that.

We would ceil() the min weight and floor() the max weight, leaving us with a range of 1001 to 1000 which is both invalid and weird. We've removed almost 1 for the max and almost 0 for the min because we're biased in how we round.

I hope this managed to convince you =)