UnitConverter - for dynamic unit conversion

[angularsen]

I just added some new stuff to make it easier to dynamically convert between units, such as by user choice in a GUI. I was motivated by a stackexchange post, but this scenario has been on my mind for some time.

• New class UnitConverter
• New property Units on quantities, ex: LengthUnit[] Units { get; } on Length
• Renamed (obsoleted) UnitClass enum to QuantityType for new naming convention

This allows the following scenario:

// Get quantities for populating quantity UI selector
QuantityType[] quantityTypes = Enum.GetValues(typeof(QuantityType)).Cast<QuantityType>().ToArray();

// If Length is selected, get length units for populating from/to UI selectors
LengthUnit[] lengthUnits = Length.Units;

// Perform conversion by using .ToString() on selected units
double centimeters = UnitConverter.ConvertByName(5, "Length", "Meter", "Centimeter");
double centimeters2 = UnitConverter.ConvertByAbbreviation(5, "Length", "m", "cm");

I did eagerly merge this to master and release a nuget (3.70.0), although I probably should have done a PR first. I still would like to hear your thoughts on the design and any improvements/flaws you can think of.

Test cases:
https://github.com/anjdreas/UnitsNet/blob/master/UnitsNet.Tests/UnitConverterTest.cs

cc @eriove @JKSnd

Ref:
#220 @JenTechSystems
#27 @BrannonKing

[eriove]

GitHub's user interface for PRs is much better than when just browsing code so it has taken a while for me to get around to review this.

I would add some tests for when the input strings are wrong and also limit the number of catched exceptions in TryConvertByName(). I'm not sure but the current code could probably hide exceptions related to failing to load assemblies and security exceptions that are better to forward to the caller.

[angularsen]

Thanks for checking it out.

I would add some tests for when the input strings are wrong

I added two tests on invalid input

• TryConvertByName_ReturnsFalseForInvalidInput
• TryConvertByAbbreviation_ReturnsFalseForInvalidInput

I see I don't test that exceptions are thrown for invalid input on the non-try methods, though. That would probably be good to add, if that is what you meant?

and also limit the number of catched exceptions in TryConvertByName(). I'm not sure but the current code could probably hide exceptions related to failing to load assemblies and security exceptions that are better to forward to the caller.

Good question, I have always been of the impression that in .NET any method prefixed with Try basically means no exceptions will ever escape it. However, you may be right that certain exceptions should in fact escape. I really don't know?

Int32.TryParse

The TryParse method is like the Parse method, except the TryParse method does not throw an exception if the conversion fails. It eliminates the need to use exception handling to test for a FormatException in the event that s is invalid and cannot be successfully parsed.

It kind of reads like it will only catch FormatException, but if any other exceptions occur they will still leak out.

Then I found this guideline on try-parse pattern:

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern

I found this text snippet particularly interesting, but also a bit ambiguous:

When using this pattern, it is important to define the try functionality in strict terms. If the member fails for any reason other than the well-defined try, the member must still throw a corresponding exception.

Does this mean you should only catch expected exceptions, such as formatexception in the context of trying to parse, or possibly a UnitNotFoundException or ConvertException in the context of TryConvertByName()?

I think you may be on to something here, but would be good to see a definite, clear-cut guideline on it.

[angularsen]

And if we are to catch only known/expected exceptions, should the Convert methods catch and wrap these known exceptions as say a UnitNotFoundException : UnitsNetException and let all others pass through as-is? Then in TryConvert methods we handle only UnitNotFoundException and any other knowns, and let all other exceptions leak out?

I'm a bit afraid that the TryConvert methods will leak exceptions when the caller does not except any exceptions at all. At least I was of the impression that it would simply never fail like that.

[angularsen]

Found this guideline:
https://msdn.microsoft.com/en-us/library/ms229009(v=vs.100).aspx

Consider the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.
To implement The TryParse pattern, you provide two different methods for performing an operation that can throw exceptions in common scenarios. The first method, X, does the operation and throws the exception when appropriate. The second method, TryX, does not throw the exception, but instead returns a Boolean value indicating success or failure. Any data returned by a successful call to TryX is returned using an out (ByRef in Visual Basic) parameter. The Parse and TryParse methods are examples of this pattern.
Do provide an exception-throwing member for each member using the TryParse pattern.
It is almost always not correct design to provide only the TryX method because it requires understanding out parameters. Also, the performance impact of exceptions is not an issue for most common scenarios; you should provide methods that are easy to use in most common scenarios.

And this post matches my expectations, no exceptions:
https://stackoverflow.com/a/17207748/134761

[angularsen]

Added invalid string test cases:
2bc2d67

Also added two new exception types, UnitNotFoundException and QuantityNotFoundException for UnitConverter and its dynamic string-based conversions.

[eriove]

Eric Lippert has written many good blog post about exceptions. I like this one the most (since it is short and good as a reference).

The risk with catching everything is that we are hiding fatal exceptions from the caller.

But, my main reason for feeling strongly about not catching all exceptions (except for logging and re-throwing) is that we had some production code with an elusive bug that took 5 years to track down. It ended up being a case just like this one. We tried getting a value and when we didn't get it we reverted to the default. About once every year the program was running, another undocumented, exception was thrown. This deleted the actual setting entered by the user and replaced it with the default value. By only catching the expected exceptions we wouldn't have had that problem.

I have no idea what kind of exception this would be in this case, but since we are working with reflections and assemblies I have the feeling that we might trigger unexpected exception in very rare instances.

In similar instances I now usually try to break the code in all possible ways by changing the input arguments and make sure that Try() returns false.

[angularsen]

Nice article, thanks! It didn't make wiser of whether Try should leak fatal (out of memory) or exogenous (I don't know.. an environment-dependent edge case in reflection if there is such a thing?) exceptions, but he does hint to it:

Out of memory, thread aborted, and so on. There is absolutely no point in catching these because nothing your puny user code can do will fix the problem.

I'm still on the fence here on what is right. I see your point about the totally unexpected exception, but my counter argument is maybe that if you are calling a Try method in the first place, you expect it to fail at some point and need to design around that.

[eriove]

Sorry, I forgot to get back to this. Should have read the articles before linking them, they are nice but doesn't help us.

I'm getting more and more undecided when it comes to catching all exceptions. I would suggest to keep it as is for now.

[angularsen]

I've tossed the question to some communities and the consensus seemed to be to handle expected exceptions - or as Lipert would put it, boneheaded and vexing exceptions.

This means, don't try to catch things like OutOfMemoryException or ThreadAbortedException as you can't really recover from these anyway.

Another thing I learned is that Try methods are preferred by performance-sensitive code, which means a general try-catch is not what you want to do inside it. Instead it should rely on Try methods internally or test conditions itself to avoid exceptions where appropriate.

Consider the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.

So, I think definitely the Try methods should be rewritten to avoid exceptions in the first place and handle expected exceptions if any.