1

Hi everyone,

I'm working on some code to display cards using MVVM design.

To make it quick, here is some sample of my code.

Here is my ViewModel :

public sealed class CardViewModel : BindableBase
{
    private int _id;
    public int Id
    {
        get { return _id; }
        set { SetProperty(ref _id, value); }
    }

    public CardViewModel(CardDTO card)
    {
        Id = card.Id;
    }
}

CardDTO is some transfer object that I receive through a service call.
Here is the code.

public sealed class CardDTO
{
    public int Id { get; set; }
}

In the view, each card display a different picture based on its Id.
To do this, a converter is used.

public sealed CharIdToFillConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
    {
        var id = (int)value;

        switch(id)
        {
            case 1:
                return new ImageBrush(...);
            case 2:
                return new ImageBrush(...);
            default:
                throw new NotImplementedException();
        }
    }

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

I don't like the switch in this code.
Is there a way, some good practice, to make it cleaner ?
Should I use polymorphism instead of my Id property ?

Thanks

rognar
  • 73
  • 4
  • 1
    "I don't like the switch in this code." What don't you like about it? Are you planning on having many more images? Do you use them in multiple places? For just two images it looks pretty clean. – Lithium Mar 15 '18 at 13:35
  • 1
    My first idea would be to stuff those ImageBrushes into a Resource collection and have them looked up by a key that incorporates id ... So you also will only have one instance of each. – Fildor Mar 15 '18 at 13:37
  • 1
    As a note, you shouldn't throw a NotImplementedException when the id isn't valid. Either return null or throw something like InvalidArgumentException. – Clemens Mar 15 '18 at 13:40
  • 1
    Alternatively to a ResourceDictionary you may also add an appropriate view model property that directly returns the desired ImageBrush. No real need for a converter. – Clemens Mar 15 '18 at 13:45
  • This should go on [codereview](https://codereview.stackexchange.com/) – FCin Mar 15 '18 at 13:51
  • 1
    When you mentioned polymorphism, do you mean data templates instead of converter or what? `CardDTO` is a model type, typically you would wrap it into its own viewmodel (what @Clemens say in comment above) and yes, you can then use different types to have different appearance, not only image. That way is preferrable over writing many converters, though I think template selector is also a good choice. – Sinatr Mar 15 '18 at 13:52
  • @Lithium For the post I only have two but in final code, I'll have eight. For the switch, I don't know why but I don't feel it well. – rognar Mar 15 '18 at 13:57
  • @Fildor I like your idea. I think I'll make a Dictionary(Id, ImageBrush). – rognar Mar 15 '18 at 14:00
  • @Clemens Ok for the Exception. But I don't like to have some class like ImageBrush referenced in a ViewModel. – rognar Mar 15 '18 at 14:01
  • 3
    The dictionary may simply be `App.Resources`. Just call `Application.Current.FindResource` to retrieve an ImageBrush for an id. – Clemens Mar 15 '18 at 14:22

1 Answers1

0

Since your conversion is one-way and you probably don't need to test the Brushes (because of its simple logic) and I assume that you want the best performance then Triggers are your friend. Don't rely on Converters too much because they are slower than Triggers. (more information regarding performance issues of converters in WPF)

You can achieve this either by using a DataTrigger on Id, or by using polymorphism (each derived type will have its own Style or Template, where WPF automatically chooses the right one based on the type of the object it is trying to visualize)

Bizhan
  • 16,157
  • 9
  • 63
  • 101
  • "Don't rely on Converters too much because they are slower than Converters" I think you mean they are slower than triggers, right? – Lithium Mar 15 '18 at 15:17