patterncsharpMinor
Convertors converting unconverted objects
Viewed 0 times
convertingconvertorsunconvertedobjects
Problem
In my app, I have several convertors, like these two:
In the first, I have an
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
Should I write that as:
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:
Should be:
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:
This isn't as big of a clutter, just space-consuming:
Acceptable alternatives:
I would do the following for maximum readability:
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:
One-way converters:
I would consider the following situation before throwing an
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
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.