zenta-ab / fortnox.NET

Dotnet SDK for Fortnox API
MIT License
17 stars 7 forks source link

Usage of decimal instead of float doesn't follow Fortnox API spec #31

Closed brokenprogrammer closed 3 years ago

brokenprogrammer commented 3 years ago

Me and @kwestground talked about using decimal in favor of float for all floating-point values for the extra precision and to prevent potential errors (See What Every Computer Scientist Should Know About Floating-Point Arithmetic if interested)

This was acted upon in #28 however on second thought this might have been done too soon. Reason is that if you follow the Fortnox API documentation they only specify float.

What is our action on this? There are some data types that are specified as string in our library but shouldn't be and I am currently correcting this in my branch https://github.com/zenta-ab/fortnox.NET/tree/feature/type-changes

But should we revert the float to decimal change? And if not should we do the same changes with integers and change them all into long? In my oppinion we should follow the api specification as that will provide a correct interface for our users to use.

kwestground commented 3 years ago

I think we should stick with decimal. Fortnox's own C# SDK uses decimal. I think the Fortnox API documentations haven't taken in account the problems that may occur using an float i C# but instead uses this data type that their programming language uses for handling numbers.

brokenprogrammer commented 3 years ago

If their API only supports float and we allow decimal or double to be used when for example updating values then they would most likely be truncated anyway.

In my oppinion we should strive to be correct and follow the documentation of their API and not follow how another SDK implemented their solution.

Another problem that I personally do not like with using bigger datatypes than needed is that there is alot of memory wasted for no reason. I believe a decimal in C# is 16 bytes while a float is 4, wasting 12 bytes per allocated decimal. If we go the same route with representing integers as long we'd most likely be wasting an additional 4.

GGAlanSmithee commented 3 years ago

@kwestground I agree that the most correct behaviour, as far as modelling the real world, is to use decimal. However, if in fact the Fortnox API (to which this SDK interface against) does not use decimal, but rather float, we have three problems:

  1. We give the user a false sense of correctness
  2. We do not reflect the API that we implement 3.It is not obvious how we would communicate point 1 and 2 to the user of this SDK, which means it could lead to hard-to-detect bugs in user space

I would say that this is a limitation / bug in the Fortnox API (and probably their backend as well), and until they fix that, using decimals in our SDK won't change anything unfortunatley.

Do you agree with this @kwestground?

@brokenprogrammer have you verified that the Fortnox SDK does use float? And if so, can we contact them to see if that is a API contract limitation or backend datastructure bug, and if the prior, see if they would be willing to change this? They must surely be aware of the implications, so their decision is probably well founded, let's find out why.

brokenprogrammer commented 3 years ago

I got a response from my contact at Fortnox, he states that the code was copied into the documentation and it wont be 100% accurate untill they have automated documentation in place. For now their official SDK is the most accurate source of which data type is the correct one.

So looks to me that we go with the biggest datatype for the right job, aka decimal for all floating point values and long for all integer values.

thoughts @kwestground @GGAlanSmithee ?

GGAlanSmithee commented 3 years ago

@brokenprogrammer sounds right to me. Maybe we can add a note about this is the docs (maybe start of a FAQ), in case someone wonders why we deviate from the official API docs.

brokenprogrammer commented 3 years ago

I recieved word from Fortnox that they plan to release their new documentation "soon after new years".

With this said I think we can expect an update from them soon and update our SDK accordingly. With this I think we can close this issue.