Issue3888 fix

[jinek]

Issue3888 fix

Summary

Improper exceptions behavior removed. Unnecessary "as operator" removed. Refactoring.

Fixes #3888

Details

More information in top 5 comments here #3888

Suggested Reviewers

@mairaw

[dnfclas]

All CLA requirements met.

[jzeferino]

This can still produce FormatException.

[jzeferino]

The try catch should be there.

[jinek]

According to example description there is no any values in this example that can produce FormatException and this TemplateSelector does not have meaning in working with other data types and data formats.
If you are talking about future changes to this code, note that in fact this can also produce OutOfMemoryException, InvalidCastException, ThreadAbortException and any type of exception in this lines as well as in any other lines. Catching any of exceptions we do not expect will lead to an issue, silently caught and masked by the behavior implemented inside catch block. For example: catch(FormatException){return null;} will produce the behavior not stated in example description and behavior neither user of application nor user (developer) of this class expect.

Statement from c# Programming Guide:
Do not catch an exception unless you can handle it and leave the application in a known state. If you catch System.Exception, rethrow it using the throw keyword at the end of the catch block.
More information here: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/exceptions/

So, allowing this code to throw FormatException actually allows user if this type to be notified about wrong usage (as Exceptions in .NET introduced for).

If we want to make this situation better we can write additional code, like:

try
{
var num = Convert.ToInt32((string)item);
}
catch(InvalidCastException invalidCastException)
{
throw new InvalidCastException($"Every item passed to this template must be of type {typeof(String)}",invalidCastException);
}
catch(FormatException formatException)
{
throw new InvalidCastException($"Every item string passed to this template must be convertible to {typeof(int)}",invalidCastException);
}

This is only a small part of possible exceptional situations, but it shows several times more code than necessary for TemplateSelector explainations.

I see the possibility to create generic TemplateSelector where T must be if type int. In this case it is possible to avoid errors in runtime and to get them in compile time, but again, there will be too much of code.

[jzeferino]

If thats the desired behavior i'm ok with it.

[jinek]

Sorry, with what exactly?

[jzeferino]

The exception handling.

[jinek]

Anyway, FormatException is not possible in this example, don't you agree?

[jzeferino]

Yes I agree.

[svick]

I think there's a typo in the class name: it's "Numder" when it should be "Number". Since you're already refactoring the class, do you want to fix that too? (And when doing that, don't forget to also change the name in XAML.)

[mairaw]

Thanks so much @jinek! The changes look good. Since we have a VB pair for this snippet, that would also have to be changed.
https://github.com/jinek/docs/tree/Issue3888/samples/snippets/visualbasic/VS_Snippets_Wpf/ContentControlTemplateSelector/VisualBasic

Do you wanna make that change or should I? Thanks!

[mairaw]

That's great @jinek. Thanks for making the changes to both VB and C# samples. Your fix should go live with our next update cycle, probably in the next day or so.

[jinek]

@mairaw You are welcome!
Thank you too!
I see more similar issues within docs. May be it makes sense to create a common task for that? But it can be hard to merge all of them simultaneously