View Model Base

Question

I needed to write code in WPF for a client. The application is done with prism (not my decision, they already had people working on it), so I got a sparkling clean new module to work in. Since this was my first time writing WPF code going to production, I wanted to handle the code in a good manner. I decided to go MVVM.

I structured the module as follows (I do need to move the RelayCommand and ViewModelBase):

• Services (IItemTypeService.cs, ItemTypeService.cs)
• ViewModels (ItemTypeViewModel.cs, ItemTypeViewModel.cs, RelayCommand.cs, ViewModelBase.cs)
• Views (ItemTypeAdminView.xaml, ItemTypeDetailView.xaml)

The ViewModelBase implements INotifyPropertyChanged.

The ItemTypeAdminView is the main view. ItemTypeDetailView is a user control and is put into the ItemTypeAdminView. The ItemTypeAdminView will in the future be expanded with other sections, and those sections will use the same information as the ItemTypeDetailView.

Therefore, I made a ViewModel for the ItemTypeAdminView, not for the ItemTypeDetailView. Every ItemType is coming from Linq2SQL (yes, I know about EF - I wasn't involved in this part), and is wrapped in a ItemTypeViewModel so it can implement the INotifyPropertyChanged.

The code for the ItemTypeAdminViewModel:

public class ItemTypeAdminViewModel : ViewModelBase
{
    #region private fields
    private ICollectionView collectionView;
    private IItemTypeService itemTypeService;
    #endregion

    #region automatic properties
    public ObservableCollection<ItemTypeViewModel> ItemTypes { get; private set; }
    public IEnumerable<Company> Companies { get; private set; }
    public IEnumerable<CompanyGTIN> CompanyGTINs { get; private set; }

    private Company selectedCompany;
    public Company SelectedCompany
    {
        get { return selectedCompany; }
        set
        {
            selectedCompany = value;
            LoadCompanyGTINs();
        }
    }
    #endregion properties

    #region constructors
    public ItemTypeAdminViewModel(IItemTypeService itemTypeService)
    {
        this.itemTypeService = itemTypeService;
        Initialize();
    }
    #endregion

    #region private methods
    private void Initialize()
    {
        //Should I wrap in Try/Catch? Try/Catch is expensive, must find something else...
        ItemTypes = new ObservableCollection<ItemTypeViewModel>(itemTypeService.GetItemTypes());
        Companies = itemTypeService.GetCompanies();
        collectionView = CollectionViewSource.GetDefaultView(ItemTypes);
    }

    private void LoadCompanyGTINs()
    {
        CompanyGTINs = itemTypeService.GetCompanyGTINs(selectedCompany.ID);
        OnPropertyChanged("CompanyGTINs");
    }
    #endregion

    #region commands
    public ICommand GoToFirstItemType
    {
        get
        {
            return new RelayCommand(() => collectionView.MoveCurrentToFirst(),
                () => collectionView.CurrentPosition >= 1);
        }
    }

    public ICommand GoToLastItemType
    {
        get
        {
            return new RelayCommand(() => collectionView.MoveCurrentToLast(),
                () => collectionView.CurrentPosition < (ItemTypes.Count - 1));
        }
    }

    public ICommand NextItemType
    {
        get
        {
            LoadCompanyGTINs();
            return new RelayCommand(() => collectionView.MoveCurrentToNext(),
                () => collectionView.CurrentPosition < (ItemTypes.Count - 1));
        }
    }

    public ICommand PreviousItemType
    {
        get
        {
            return new RelayCommand(() => collectionView.MoveCurrentToPrevious(),
                () => collectionView.CurrentPosition >= 1);
        }
    }
    #endregion
}

And the code for the ItemTypeDetailView XAML:

<Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto" />
            <RowDefinition Height="Auto" />
            <RowDefinition Height="*" />
            <RowDefinition Height="Auto" />
        </Grid.RowDefinitions>

        <Grid DataContext="{Binding ItemTypes}">
            <Grid.ColumnDefinitions>
                <ColumnDefinition />
                <ColumnDefinition />
            </Grid.ColumnDefinitions>
            <Grid.RowDefinitions>
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
            </Grid.RowDefinitions>

            <TextBlock Text="Name:" />
            <TextBox Grid.Column="2" Text="{Binding Name}" />

            <TextBlock Grid.Row="1" Text="Description:" />
            <TextBox Grid.Column="1" Grid.Row="1" Text="{Binding Description}" />

            <TextBlock Grid.Row="2" Text="Manufacturer:" />
            <ComboBox Grid.Column="1" Grid.Row="2"
                      DisplayMemberPath="Name"
                      ItemsSource="{Binding Path=DataContext.Companies, RelativeSource={RelativeSource FindAncestor, AncestorType=UserControl, AncestorLevel=2}}"
                      Name="ManufacturerComboBox"
                      SelectedItem="{Binding Mode=TwoWay, Path=DataContext.SelectedCompany, RelativeSource={RelativeSource FindAncestor, AncestorType=UserControl, AncestorLevel=2}}"
                      SelectedValue="{Binding ManufacturerID}"
                      SelectedValuePath="ID" />

            <TextBlock Grid.Row="10" Text="Manufacturer Product Code:"/>
            <TextBox Grid.Column="1" Grid.Row="10" Text="{Binding ManufacturerProductCode}" />

            <TextBlock Grid.Row="11" Text="Manufacturer Product GS1 Code:"/>
            <TextBox Grid.Column="1" Grid.Row="11" Text="{Binding ManufacturerProductGS1Code}" />

            <TextBlock Grid.Row="12" Text="GS1GTINID:"/>
            <ComboBox DisplayMemberPath="GTIN"
                      Grid.Column="1" Grid.Row="12"
                      IsSynchronizedWithCurrentItem="True"
                      ItemsSource="{Binding Path=DataContext.CompanyGTINs, RelativeSource={RelativeSource FindAncestor, AncestorType=UserControl, AncestorLevel=2}}"
                      SelectedValue="{Binding GS1GTINID}"
                      SelectedValuePath="ID" />
        </Grid>

        <Grid Grid.Row="2">
            <StackPanel Grid.Row="4" Orientation="Horizontal">
                <Button Command="{Binding GoToFirstItemType}">&lt;&lt;</Button>
                <TextBlock Margin="5,0,5,0" />
                <Button Command="{Binding PreviousItemType}">&lt;</Button>
                <TextBlock Margin="5,0,5,0" />
                <Button Command="{Binding NextItemType}">&gt;</Button>
                <TextBlock Margin="5,0,5,0" />
                <Button Command="{Binding GoToLastItemType}">&gt;&gt;</Button>
                <TextBlock Margin="5,0,5,0" />
                <Button Command="{Binding New}">New</Button>
            </StackPanel>
        </Grid>
    </Grid>

I am really curious about what could be improved structure/code wise and/or what should be left out.

Answer 1

ViewModel generally looks good though I would still propose some changes:

1. I do not see why do you need collectionView field. You're moving it's CurrentItem but neither it nor the entire view is exposed anywhere.

2. public ICommand NextItemType. Why do you have LoadCompanyGTINs() call here? You have several similar Move... methods but only here you have this call.

---

Regarding View:

1. You have strange layout in the inner Grid. It has two columns but you're setting inner Grid.Control attached properties to 1 and 2 instead of 0 and 1

2. Generally Bindings in your view look VERY strange. First of all I would recommend never ever bind DataContext inside view (<Grid DataContext="{Binding ItemTypes}">). If something inside view has separate DataContext then it should be a separate View+ViewModel. Because of this binding you have to use RelativeSource inside for some other controls which is not good. The bindings inside first grid which do not have RelativeSource also look strange. For example <TextBox Grid.Column="2" Text="{Binding Name}" />. With such binding I think it tries to locate a Name property on ItemTypes collection but this is an ObservableCollection which doesn't have such property so it should not work. The same about Description.

3. If you really need RelativeSource then at least try to avoid using it with RelativeSource={RelativeSource FindAncestor, AncestorType=UserControl, AncestorLevel=2}. I've tried to guess what will be the result Source for binding but had no luck. I would recommend using something like RelativeSource={RelativeSource FindAncestor, AncestorType=local:ItemTypeDetailView} instead.

Generally I would insist only on getting rid of those weird bindings in your View, the rest is more or less ok.