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.

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.

share|improve this question
add comment

1 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.

share|improve this answer
    
Thank you for your review :-) 1) I don't think I get this one. I use a field so that the commands (first, next, previous, last) can access that. 2) You're right. That shouldn't belong there at all. I put it there to check something out and got sidetracked. 3) Again - this was indeed wrong. Fixed it. 4) You'd be right to think that Text="Binding Name" gets it from the ItemTypes collection, because that's what the containing Grid's datacontext is set to. ObservableCollection indeed does not have a Name property, but the ItemTypes that are contained in it do. –  DerMeister Apr 19 '11 at 18:07
    
4) Cont'd What I wanted to do here is not to have a ViewModel for the ItemTypesDetailView, but move everything up one level. Hence, the ItemTypes collection is in ItemTypeAdminViewModel and not have a ItemTypesDetailViewModel that has the ItemTypes collection in it. The reason for this is that there will be other User Controls that will also need some of the information of ItemTypes. 5) I had to use AncestorLevel=2 here because Prism modules do not have a Main Window, just a... eh.. 'Main' user control. –  DerMeister Apr 19 '11 at 18:11
add comment

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.