3
\$\begingroup\$

I am creating a converter for use in a Windows 8 XAML app (MVVM). As you can see in the code below, the converter is used to convert a bool into one of two strings from a language resource file.
My question is, is it better to create one instance of ResourceLoader for the converter class or should I continue creating new instances each time the method is called?

class BooleanToBudgetYearStringConverter : IValueConverter
{
    private const string CALENDAR = "BudgetCycle_Calendar",
                        FISCAL = "BudgetCycle_Fiscal";

    private Windows.ApplicationModel.Resources.ResourceLoader loader;

    public object Convert(object value, Type targetType, object parameter, string language)
    {
        string entryName = (value is bool && (bool)value) ? CALENDAR : FISCAL;  //The name of the entry desired from the entry file.
        return new Windows.ApplicationModel.Resources.ResourceLoader().GetString(entryName);
    }

    public object ConvertBack(object value, Type targetType, object parameter, string language)
    {
        return (value is string && (string)value == new Windows.ApplicationModel.Resources.ResourceLoader().GetString(CALENDAR));
    }
}

I am specifically concerned about speed and memory use.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Are you sure you should be newing up ResourceLoader objects at all? It seems like Windows.ApplicationModel.Resources.ResourceLoader has static methods on it for getting access to ResourceLoader objects in a singleton manner already. Have you tried using Windows.ApplicationModel.Resources.ResourceLoader.GetForCurrentView().GetString(entryName) instead? \$\endgroup\$ Commented Apr 23, 2014 at 1:39
  • \$\begingroup\$ If the tools support it, neither: let the framework manage and provide instances as appropriate. Not only is this better for client code; it also supports encapsulation, performance, and testing. If not, consider the cost of instantiation (probably negligible) before picking the better choice. \$\endgroup\$ Commented Apr 23, 2014 at 3:22

2 Answers 2

Reset to default
5
\$\begingroup\$

Use an Attribute

The class should be tagged with:

[ValueConversion(typeof(Boolean), typeof(String))]

If it's of the normal, old-school .NET variety (desktop, client profile, WP8). Windows store apps don't appear to have an analogous attribute in their .NET libraries, and thus this recommendation can be ignored when compiling such a program.

Drop Type Checks

If the passed-in value is not of the expected type, you should not do your conversion; you should allow an exception to be thrown, so that you know an invalid binding has taken place (and you can go correct the offending code). This applies to both conversion and back-conversion.

Explicit Back Conversion

Check for an explicit match -- don't default to returning "false" on a non-match with the CALENDAR string. Instead, check that the input string matches with the FISCAL string. Throw an exception if the input string matches neither acceptable string.

Use a Singleton

The documentation for Windows.ApplicationModel.Resources.ResourceLoader implies that the default constructor you're using should result in an object that is identical in function to the one available from the static GetForCurrentView() function. If not, then set up your own singleton pattern to ensure that you have at most one shared copy of the resource object around at any given time.

\$\endgroup\$
6
  • \$\begingroup\$ +1 nice answer. I think the converter will still work without the attribute, care to expand on what the role of the ValueConversionAttribute is? \$\endgroup\$ Commented Apr 23, 2014 at 2:32
  • 1
    \$\begingroup\$ @Mat'sMug It's a clear (and machine readable) declaration of intent, so nobody has to guess. From the documentation, "When implementing the IValueConverter interface, it is a good practice to decorate the implementation with a ValueConversionAttribute attribute to indicate to development tools the data types involved in the conversion[...]." Now, as to what specific tools use it, and how they use it, I don't know. I would assume it helps prevent bogus bindings. :) \$\endgroup\$ Commented Apr 23, 2014 at 2:40
  • \$\begingroup\$ For some reason System.Windows.Data and hence ValueConversion is not available in my Windows 8.1 app. \$\endgroup\$ Commented Apr 23, 2014 at 16:14
  • \$\begingroup\$ @Trisped Well, that's confusing, because IValueConverter is in System.Windows.Data. Are you referencing the PresentationFramework assembly from your project? Can you provide the fully-qualified name of the IValueConverter you're deriving from? \$\endgroup\$ Commented Apr 23, 2014 at 17:04
  • 2
    \$\begingroup\$ @Trisped Ah, right, that little nightmare. It doesn't appear that they ported that attribute over to the new flavor of .NET libraries. My apologies; I didn't realize you meant Metro (er... "windows store") app when you said "Windows 8." It's even more confusing in that the (non-store-app) MSDN page says this attribute is supported in the Win8 phone, as well as in the client profile... so it's a mystery to me why it's dropped from the windows store app API. :/ \$\endgroup\$ Commented Apr 23, 2014 at 17:37
3
\$\begingroup\$ 
private const string CALENDAR = "BudgetCycle_Calendar",
                    FISCAL = "BudgetCycle_Fiscal";

Would be much prettier as two instructions:

private const string CALENDAR = "BudgetCycle_Calendar";
private const string FISCAL = "BudgetCycle_Fiscal";

private Windows.ApplicationModel.Resources.ResourceLoader loader;

Given using Windows.ApplicationModel.Resources;, the above declaration can be simplified to:

private ResourceLoader loader;

The variable is declared, but never assigned, and isn't used anywhere.

I'd take @Travis' advice and assign it like this:

private ResourceLoader loader = ResourceLoader.GetForCurrentView();

This greatly improves the readability of your return statements:

return loader.GetString(entryName);

and

return (value is string && value.ToString() == loader.GetString(CALENDAR));

(follow @Travis' advice here too, about explicit back conversion / well, his entire answer actually)

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.