View Model Base

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 ItemTypes { get; private set; }
public IEnumerable Companies { get; private set; }
public IEnumerable CompanyGTINs { get; private set; }

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

#region constructors
public ItemT

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

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

-
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:

-
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

-
Generally Bindings in your view look VERY strange. First of all I would recommend never ever bind DataContext inside view (`). 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 . 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.

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

StackExchange Code Review Q#1983, answer score: 5