Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In my WP8 application I need to show a couple of radio buttons to let the user select a value out of an enum. I don't want to hardcode either the value or the text (description) showed to the user. I would like my Model/ViewModel drive them for me.

As of now, this is what I have:

//enum defined in model
 public enum Units
    {
        [Description ("Meters/km")]
        Metric,
        [Description ("Feet/Miles")]
        Imperial
    }

Converters defined to handle string and Boolean conversions

//<Summary>Converts ENum to boolean and back
// Convert: uses parameter passed in, returns true if current value of the Enum matches parameter
//ConvertBack: if value is true, sets the value of the ENum to parameter passed in
//</summary>
[ValueConversion (typeof (Enum), typeof (Boolean))]
public class EnumToBooleanConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        string parameterString = parameter as string;
        if ( parameterString == null )
            return DependencyProperty.UnsetValue;

        if ( Enum.IsDefined (value.GetType (), value) == false )
            return DependencyProperty.UnsetValue;

        object parameterValue = Enum.Parse (value.GetType (), parameterString);

        return parameterValue.Equals (value);

    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        return value.Equals (true) ? Enum.Parse(targetType, parameter as String) : DependencyProperty.UnsetValue;
    }
}

//<Summary>Converts ENum to strings, uses Description for the parameter passed in, or parameter as string</summary>
[ValueConversion (typeof (Enum), typeof (String))]
public class EnumToStringConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        string parameterString = parameter as string;
        if ( parameterString == null )
            return DependencyProperty.UnsetValue;

        if ( Enum.IsDefined (value.GetType (), value) == false )
            return DependencyProperty.UnsetValue;

        var desc = (value.GetType ().GetField (parameterString).GetCustomAttributes (typeof (DescriptionAttribute), false).FirstOrDefault() as DescriptionAttribute);
        if ( desc != null )
            return desc.Description;
        else
            return parameter.ToString ();
    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        throw new NotImplementedException ();
    }
}

In the XAML:

   <Grid  Margin="0,10,0,0">
    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition />
    </Grid.RowDefinitions>
    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="Auto" />
        <ColumnDefinition Width="Auto" />
    </Grid.ColumnDefinitions>
    <TextBlock Text="How do you measure distance?"
               Margin="6,0,0,0"
               Grid.ColumnSpan="2"
               Grid.Row="0" />
    <RadioButton Grid.Row="1"
                 Grid.Column="0"
                 Content="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToStringConverter}}"
                 IsChecked="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
    <RadioButton Grid.Row="1"
                 Grid.Column="1"
                 Content="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToStringConverter}}"
                 IsChecked="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
</Grid>

Does anyone here think I am on the right track with my approach? Any suggestions on improving the code (in terms of logic, clarity, performance)?

share|improve this question

3 Answers 3

up vote 7 down vote accepted

This looks perfectly reasonable. In fact, the only thing I would really change is this:

string parameterString = parameter as string;
if ( parameterString == null )
    return DependencyProperty.UnsetValue;

if ( Enum.IsDefined (value.GetType (), value) == false )
    return DependencyProperty.UnsetValue;

You have this in both Convert methods, and it would be just as clear like this:

string parameterString = parameter as string;
if ( parameterString == null || !Enum.IsDefined (value.GetType (), value) )
    return DependencyProperty.UnsetValue;

Additionally, you could concatenate this:

object parameterValue = Enum.Parse (value.GetType (), parameterString);
return parameterValue.Equals (value);

Like this:

return Enum.Parse (value.GetType(), parameterString).Equals (value);

For your XAML, you do not need to put each property on its own individual line. I typically only break them up when they do not fit on my screen all at once, more like this

<TextBlock Text="How do you measure distance?" Margin="6,0,0,0" Grid.ColumnSpan="2" Grid.Row="0" />
<RadioButton Grid.Row="1" Grid.Column="0"
             Content="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToStringConverter}}"
             IsChecked="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />

Finally, for your UI, you most likely want to label your radio buttons as what they are, and perhaps change your UI a little to accommodate the labels and still look good. I would recommend a ComboBox if you have trouble adjusting the UI to look good. You could then bind the SelectedItem or SelectedIndex to a value in your VM and use a converter to change the value as needed.

<ComboBox Text="Label">
    <ComboBoxItem>Metric (meters)</ComboBoxItem>
    <ComboBoxItem>Imperial (feet)</ComboBoxItem>
</ComboBox>
share|improve this answer
    
Thank you for the answer. Regarding the line breaks, I think its more of personal choice. This way its easier for me to read as each property will be on its own line. –  Adarsha Jan 14 at 1:14
    
Yes, you are completely correct that it is personal choice. I have always done it the other way, with each line having similar lengths, so I just mentioned it. –  Hosch250 Jan 14 at 1:18
    
Could you please explain more on the "you most likely want to label your radio buttons as what they are" part. For the Radio Buttons, I am already getting the labels from model from its description. Do you mean I need to change the description or just hardcode it in the XAML? I don't want to go for comboboxes as the are not easy to use on a Phone. I might actually just use a toggle button, and have two labels (bound to descriptions) at either end. –  Adarsha Jan 14 at 1:40
    
In your XAML, you do not specify which button is for Imperial measurements, and which is for Metric measurements. At least, it is not in the XAML in the question. I have ComboBoxes in my app, I have an easier time using them than a tiny radio button I can't even see when I set my finger down, but this is very much personal preference. –  Hosch250 Jan 14 at 1:58
    
True, I am experimenting with ToggleSwitchButton(from the toolkit) and ComboBoxes, need to finalize. –  Adarsha Jan 14 at 2:17

While I admit that chances are low that another Unit will pop up, I do feel that your solution is not really following the "spirit" of WPF/Silverlight. Particularly the fact that you code two RadioButtons with virtually the same code doesn't sit well with me. It feels conveluted, especially the enum-to-boolean conversion.

I'd much rather have a List of enums which then binds to a ListBox which exposes radioButtons via a template, as in this SO solution or this blog post.


I'm also not a fan of using a Grid like you did; I'd much prefer StackPanels:

<StackPanel Orientation="Vertical">
    <TextBlock Text="How do you measure distance?" Margin="6,0,0,0" />

    <StackPanel Orientation="Horizontal">
        <RadioButton Content="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToStringConverter}}"
                     IsChecked="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
        <RadioButton Content="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToStringConverter}}"
                     IsChecked="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
    </StackPanel>
</StackPanel>

I find that even simple Grids often introduce complexity where it isn't needed, which is evidenced by Grid.Row and Grid.Column. Because next thing another row needs to be added, and then another, and before you know it you've got ten RowDefinitions; and then one of the rows need to be removed or a new row needs to be inserted in the middle...

share|improve this answer
    
I don't agree that ListBox is the way to go. ListBox was suggested as rather a hack before .Net 4.0 when IsChecked binding was broken. In fact WPF team internally uses[similar converter][1] (Search for enum). Only change in my way is missing x:static as its not supported in WP8. I considered using Grid as I wanted both horizontal and vertical layout control. Nested Stackpanel vs one Grid should give similar performance, right? [1]blogs.msdn.com/b/jaimer/archive/2009/09/22/… –  Adarsha Jan 13 at 17:01

I think it is worth mentioning, that this design decision will bite you back as soon as you start working on localization. There is no easy way to localize attributes, because you can only pass constant string values to attribute constuctor.

If localization is out of the question, then this design is fine, I guess.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.