HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Convertors converting unconverted objects

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
convertingconvertorsunconvertedobjects

Problem

In my app, I have several convertors, like these two:

public class ThemeToTableStripeConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, string language)
    {
        if ((int)value == 1) { return new SolidColorBrush(Color.FromArgb(255, 34, 34, 34)); }
        return new SolidColorBrush(Color.FromArgb(255, 238, 238, 238));
    }

    public object ConvertBack(object value, Type targetType, object parameter, string language)
    {
        return null;
    }
}

public class ThemeToLBStyleConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, string language)
    {
        if ((int)value == 1) { return Application.Current.Resources["LBDark"]; }
        else if ((int)value == 2) { return Application.Current.Resources["LBGold"]; }
        else { return Application.Current.Resources["LBLight"]; }
    }

    public object ConvertBack(object value, Type targetType, object parameter, string language)
    {
        return null;
    }
}


In the first, I have an int representing a theme style, with 0, 1, and 2 being the three themes I support. Themes 0 and 2 are identical in the part affected by this (white background), while 1 has a black background. In the second, I want theme 0 to always be the default, no matter the value, and themes 1 and 2 to return different items.

Beyond the general comments, there are a couple things I would like specific feedback on. Both of these convertors are one-way convertors, and always will be; however, that return null; looks a little off - should I replace that with throw new NotImplementedException();? Instead of formatting my code like this:

if (condition) { return val1; }
return val2;


Should I write that as:

if (condition) { return val1; }
else { return val2; }

Solution

Avoid long one-liners:

This:

if ((int)value == 1) { return new SolidColorBrush(Color.FromArgb(255, 34, 34, 34)); }


Should be:

if ((int)value == 1)
{
    return new SolidColorBrush(Color.FromArgb(255, 34, 34, 34));
}


Typically one-liner if-statements are-so because they're short and can't be justified to take up 3 lines for something so small.

Avoid 'else' one-liners:

This is a clutter:

if (condition) { return val1; }
else { return val2; }


This isn't as big of a clutter, just space-consuming:

if (condition) 
{ 
    return val1; 
}
else 
{ 
    return val2; 
}


Acceptable alternatives:

I would do the following for maximum readability:

return condition ? val1 : val2;


Making use of the ternary operator and being the most maintainable/readable method.

In cases where you're not able to keep it one line, something like you had above is perfectly acceptable:

if (condition) { return val1; }
// Do something...?
return val2;


One-way converters:

I would consider the following situation before throwing an NotImplementedException().

public interface IOneWayValueConverter
{
    object Convert(object value, Type targetType, object parameter, string language);
}

// Optionally Adding "TwoWay" to the name (or some other similar naming method).
public interface IValueConverter : IOneWayValueConverter
{
    object ConvertBack(object value, Type targetType, object parameter, string language);
}


If this isn't an option (for any number of reasons) then it would seem the only acceptable answer to this situation would be throwing a NotImplementedException() as you stated.

Code Snippets

if ((int)value == 1) { return new SolidColorBrush(Color.FromArgb(255, 34, 34, 34)); }
if ((int)value == 1)
{
    return new SolidColorBrush(Color.FromArgb(255, 34, 34, 34));
}
if (condition) { return val1; }
else { return val2; }
if (condition) 
{ 
    return val1; 
}
else 
{ 
    return val2; 
}
return condition ? val1 : val2;

Context

StackExchange Code Review Q#82025, answer score: 2

Revisions (0)

No revisions yet.