NodaMoney icon indicating copy to clipboard operation
NodaMoney copied to clipboard

POC: Turn Currency into a class

Open gliljas opened this issue 3 years ago • 4 comments

Inspired by the comment in issue #83, which makes a lot of sense. Currency is not similar to a value type.

Also turned Money in a readonly struct, and custom JSON serialization support was needed as a result.

gliljas avatar Apr 17 '21 22:04 gliljas

Commenting on this because I happened to see it while submitting an issue. I have a lot of code that depends on currency not being nullable and this would break a lot of that. This is definitely a breaking change for a lot of people.

michaeldileo avatar May 26 '21 13:05 michaeldileo

Yes, it would definitely warrant a new major version, but I still would strongly recommend that this change is made. No Money value would ever have a null currency, and C# 8's nullability could help.

gliljas avatar May 26 '21 18:05 gliljas

I personally am not wild about throwing an argument exception in the Money constructor. Replacing with a "null object" pattern would probably be better. That would help prevent from breaking existing code.

However, my personal preference is to not make this fundamental change, but I'm also not an actual contributor on this lib.

michaeldileo avatar May 26 '21 22:05 michaeldileo

I personally am not wild about throwing an argument exception in the Money constructor. Replacing with a "null object" pattern would probably be better. That would help prevent from breaking existing code.

That could indeed be an option, but why would we throw an exception, unless it's explicitly called with a null Currency instance?

However, my personal preference is to not make this fundamental change, but I'm also not an actual contributor on this lib.

Neither am I. It's just that NodaMoney is the closest thing to a .NET Money standard there is, and this design flaw is a bit......perplexing. It would be nice to see NodaMoney reach the maturity that NodaTime has, although of course, there's no actual link except for the Noda name.

gliljas avatar May 26 '21 22:05 gliljas