Understanding the MVVM concepts and validation of my code

Question

I have been learning MVVM concepts and trying to implement it in my new project. I want to validate my work that I have been doing these past days. I want to know if I follow correctly the MVVM pattern. I understand that everyone has their way of working/thinking, but as I mention earlier what I need to know is, am I respecting the CORE of MVVM

What I want to know:

1. Do I follow the MVVM pattern?
2. Is there a better / alternative approach that I could use (following the MVVM pattern, of course)?

After learning, I came to the conclusion that I need some Helpers class, so I've created some.

The clViewModelBase class which implements the INotifyPropertyChanged interface, will help me to notify if any of my properties has changed. I did this so I don't have to implement the interface on each and every single View Model:

using System.ComponentModel;

class clViewModelBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    protected void OnPropertyChanged(string propertyName)
    {
        this.OnPropertyChanged(new PropertyChangedEventArgs(propertyName));
    }

    protected virtual void OnPropertyChanged(PropertyChangedEventArgs e)
    {
        var handler = this.PropertyChanged;
        if (handler != null)
        {
            handler(this, e);
        }
    }
}

The clDelegateCommand class which implements the ICommand interface, which will help me to fire up the events on the button:

using System;
using System.Windows.Input;

class clDelegateCommand : ICommand
{
    private Action<object> _executionAction;
    private Predicate<object> _canExecutePredicate;

    public clDelegateCommand(Action<object> execute)
        : this(execute, null)
    { }

    public clDelegateCommand(Action<object> execute, Predicate<object> canExecute)
    {
        if (execute == null)
        {
            throw new ArgumentNullException("execute");
        }

        this._executionAction = execute;
        this._canExecutePredicate = canExecute;
    }

    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }

    public bool CanExecute(object parameter)
    {
        return this._canExecutePredicate == null ? true : this._canExecutePredicate(parameter);
    }

    public void Execute(object parameter)
    {
        if (!this.CanExecute(parameter))
        {
            throw new InvalidOperationException("The command is not valid for execution, check the CanExecute method before attempting to execute.");
        }

        this._executionAction(parameter);
    }
}

The clConverter which implements the IMultiValueConverter interface, will update my values only after the user has clicked on Save. This class returns an array of the updated values.

using System;
using System.Windows.Data;
using System.Collections;
using System.Linq;

class clConverter : IMultiValueConverter
{
    public clConverter() { }

    #region IMultiValueConverter Methods
    public object Convert(object[] values, System.Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        return values.ToList();
    }

    public object[] ConvertBack(object value, System.Type[] targetTypes, object parameter, System.Globalization.CultureInfo culture)
    {
        //throw new System.NotImplementedException();
        return ((IEnumerable)value).Cast<object>().Select(x => x.ToString()).ToArray();
    }
    #endregion
}
the clBaseAddEditDeleteViewModel which derives the clViewModelBase, this is to control my form display

class clBaseAddEditDeleteViewModel : clViewModelBase
{
    #region Variables
    private bool _IsListEnabled;
    private bool _IsDetailEnabled;
    #endregion

    #region Properties
    public bool IsListEnabled
    {
        get { return this._IsListEnabled; }
        set
        {
            this._IsListEnabled = value;
            OnPropertyChanged("IsListEnabled");
        }
    }
    public bool IsDetailEnabled
    {
        get { return this._IsDetailEnabled; }
        set
        {
            this._IsDetailEnabled = value;
            OnPropertyChanged("IsDetailEnabled");
        }
    }
    #endregion

    public void SetNormalUIDisplay()
    {
        this.IsListEnabled = true;

        this.IsAddMode = false;
        this.IsDetailEnabled = false;
    }

    public void SetEditUIDisplay()
    {
        this.IsAddMode = true;
        this.IsDetailEnabled = true;

        this.IsListEnabled = false;
    }
}

Here's my Model that obviously derives the clViewModelBase and also the IDataErrorInfo interface to manage my errors, ID cannot be empty etc...

class clPart : clViewModelBase, IDataErrorInfo
{
    #region Variables
    private string _ID;
    #endregion

    #region Properties
    /// <summary>
    /// Gets or Set the ID of this part
    /// </summary>
    public virtual string ID 
    { 
        get { return this._ID; }
        set 
        {
            this._ID = value;
            OnPropertyChanged("ID");
        } 
    }
    #endregion

    #region IDataErrorInfo Members

    string IDataErrorInfo.Error
    {
        get { return null; }
    }

    string IDataErrorInfo.this[string propertyName]
    {
        get
        {
            return GetValidationError(propertyName);
        }
    }

    #endregion

    #region Validation

    /// <summary>
    /// All the properties that has to be validated
    /// </summary>
    static readonly string[] ValidatedProperties = 
    {
        "ID"
    };

    public bool IsValid
    {
        get
        {
            foreach (string property in ValidatedProperties)
                if (GetValidationError(property) != null)
                    return false;

            return true;
        }
    }

    private string GetValidationError(String propertyName)
    {
        string error = null;

        switch (propertyName)
        {
            case "ID":
                error = ValidatePartID();
                break;
        }

        return error;
    }

    private string ValidatePartID()
    {
        if (String.IsNullOrWhiteSpace(this.ID))
        {
            return "The Part ID cannot be empty.";
        }

        return null;
    }

    #endregion
}

My view consist of 2 forms: my MainWindow and the User Control. MainWindow contains 2 sections:

1. Left section that has my ListBox, for each part I will add a user control for it
2. Right section that has all my details displayed

Main Window XAML:

<Window x:Class="Mvvm.MainWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:Helpers="clr-namespace:Mvvm.ViewModel.Helpers"
    xmlns:vm="clr-namespace:Mvvm.View"
    Title="MainWindow" Height="483" Width="922.725" WindowStartupLocation="CenterScreen">

<Window.Resources>
    <Helpers:clConverter x:Key="PartConverter" />
</Window.Resources>

<Grid x:Name="grdMain" RenderTransformOrigin="0.449,0.707">
    <Label x:Name="lblPartID" Content="Part ID" HorizontalAlignment="Left" Margin="382,86,0,0" VerticalAlignment="Top" Width="109"/>
    <TextBox x:Name="txtPartID" 
             IsEnabled="{Binding IsDetailEnabled}" 
             Text="{Binding Path=CurrentSelectedPart.ID, Mode=OneWay, ValidatesOnDataErrors=True}" 
             HorizontalAlignment="Left" Height="23" Margin="498,89,0,0" TextWrapping="Wrap" VerticalAlignment="Top" Width="195"/>

    <ToolBar x:Name="tbSubMenu" HorizontalAlignment="Left" Margin="367,35,0,0" VerticalAlignment="Top" Width="548" Height="28">
        <Button x:Name="btnSaveDetails" IsEnabled="{Binding Path=IsDetailEnabled}" Command="{Binding SaveCommand}">
            <Image Source="Images/Save_6530.ico"/>

            <Button.CommandParameter>
                <MultiBinding Converter="{StaticResource PartConverter}" Mode="TwoWay">
                    <Binding ElementName="txtPartID" Path="Text"/>
                </MultiBinding>
            </Button.CommandParameter>
        </Button>

        <Separator/>
        <Button x:Name="btnEditDetail" Command="{Binding EditCommand}">
            <Image Source="Images/PencilTool_206.png"/>
        </Button>
        <Separator/>
        <Button x:Name="btnCancelDetail" IsEnabled="{Binding Path=IsDetailEnabled}" Command="{Binding CancelCommand}">
            <Image Source="Images/Cancel(build)_194_32.bmp"/>
        </Button>
    </ToolBar>

    <ListBox Name="listParts" HorizontalAlignment="Left" Height="402" Margin="10,40,0,0" IsEnabled="{Binding Path=IsListEnabled}"
             VerticalAlignment="Top" Width="352" SelectedItem="{Binding Path=CurrentSelectedPart}"
             ItemsSource="{Binding Path=CollectionPart}" >
        <ListBox.ItemTemplate>
            <DataTemplate>
                <vm:ucPart />
            </DataTemplate>
        </ListBox.ItemTemplate>
    </ListBox>
</Grid>
</Window>

My MainWindow:

base.DataContext = new clPartViewModel();

My ViewModel:

using System;
using System.Collections.ObjectModel;
using System.Windows.Input;
using System.Collections;
using System.Linq;
using System.Collections.Generic;
using Mvvm.Model;
using Mvvm.ViewModel.Helpers;

/// <summary>
/// 
/// </summary>
class clPartViewModel : clBaseAddEditDeleteViewModel
{
    #region Variables
    private ObservableCollection<clPart> _CollectionPart;
    private clPart _CurrentSelectedPart;

    private clDelegateCommand _EditCommand;
    private clDelegateCommand _SaveCommand;
    private clDelegateCommand _CancelCommand;
    #endregion

    #region Properties
    public ObservableCollection<clPart> CollectionPart
    {
        get { return this._CollectionPart; }
        set
        {
            if (this._CollectionPart == value)
                return;

            this._CollectionPart = value;
            OnPropertyChanged("CollectionPart");
        }
    }

    public clPart CurrentSelectedPart
    {
        get { return this._CurrentSelectedPart; }
        set
        {
            if (this._CurrentSelectedPart == value)
                return;

            this._CurrentSelectedPart = value;
            OnPropertyChanged("CurrentSelectedPart");
        }
    }

    public ICommand EditCommand
    {
        get 
        {
            if (this._EditCommand == null)
                this._EditCommand = new clDelegateCommand(new Action<object>(EditPart), null);

            return this._EditCommand; 
        }
    }

    public ICommand CancelCommand
    {
        get
        {
            if (this._CancelCommand == null)
                this._CancelCommand = new clDelegateCommand(new Action<object>(CancelPart), null);

            return this._CancelCommand;
        }
    }

    public ICommand SaveCommand
    {
        get
        {
            if (this._SaveCommand == null)
                this._SaveCommand = new clDelegateCommand(new Action<object>(SaveChanges), null);

            return this._SaveCommand;
        }
    }

    #endregion

    /// <summary>
    /// Cunstructor of the clPartViewModel class
    /// </summary>
    public clPartViewModel()
    {
        this.CollectionPart = new ObservableCollection<clPart>
        {
            new clPart { ID = "[0000001]" },
            new clPart { ID = "[0000002]" },
            new clPart { ID = "[0000003]" },
            new clPart { ID = "[0000004]" },
            new clPart { ID = "[0000005]" }
        };

        this.SetNormalUIDisplay();
    }


    private void EditPart(object obj)
    {
        this.SetEditUIDisplay();
    }

    private void CancelPart(object obj)
    {
        // TODO : revert changes
        this.SetNormalUIDisplay();
    }

    private void SearchByPartID(string ID)
    {
        this.CollectionPart.Where(x => x.ID == ID);
    }

    private void SaveChanges(object obj)
    {
        string[] Part = ((IEnumerable)obj).Cast<object>().Select(x => x.ToString()).ToArray();

        if (IsPartIdDuplicate(Convert.ToString(Part[0])))
            return;

        this._CurrentSelectedPart.ID = Convert.ToString(Part[0]);

        this.SetNormalUIDisplay();
    }

    /// <summary>
    /// Validate if we have duplicates ID's
    /// </summary>
    /// <returns>True, if we have duplicates</returns>
    public bool IsPartIdDuplicate(string IDToValidate)
    {
        foreach (clPart Parts in this.CollectionPart)
        {
            if (Parts.ID == IDToValidate && Parts.ID != this.CurrentSelectedPart.ID)
                return true;
        }

        return false;
    }
}

My user control view XAML to load in my ListBox:

<UserControl x:Class="Mvvm.View.ucPart"
         xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
         xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
         xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
         xmlns:d="http://schemas.microsoft.com/expression/blend/2008" 
         mc:Ignorable="d" 
         d:DesignHeight="60" Width="230">

<Grid Name="DetailsGruid" HorizontalAlignment="Left" VerticalAlignment="Center" Width="230">
    <Grid.RowDefinitions>
        <RowDefinition Height="30" />
        <RowDefinition />
    </Grid.RowDefinitions>

    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="75" />
        <ColumnDefinition Width="150" />
    </Grid.ColumnDefinitions>

    <Border Grid.RowSpan="2" Grid.ColumnSpan="3" BorderBrush="DarkRed" 
            BorderThickness="1,1,1,1" CornerRadius="8,8,8,8" />

    <Image Name="ImgPart" Grid.RowSpan="2" Grid.Column="0" Margin="5" Source="/Mvvm;component/Images/Save_6530.ico" />

    <TextBox Grid.Row="0" Grid.Column="1" Name="txtID" Width="Auto" Margin="5"
                HorizontalAlignment="Stretch" VerticalAlignment="Center" IsEnabled="False"
                Text="{Binding Path=ID}" />
</Grid>

Answer 1

Scratching the surface...

Naming

I have to second @BCdotNET's comment, the "cl" prefixes are Hungarian, confusing and against C# naming standards.

Types should be named in PascalCase; the lowercase prefix makes for very confusing code - even the syntax highlighter is confused!

private clDelegateCommand _EditCommand;
private clDelegateCommand _SaveCommand;
private clDelegateCommand _CancelCommand;

Also note, that convention for private fields is camelCase - I like that you're preceding them with an underscore (avoids having to qualify with this every time you're referring to them), but they should still be camelCase:

private DelegateCommand _editCommand;
private DelegateCommand _saveCommand;
private DelegateCommand _cancelCommand;

_{(notice how syntax highlighting picks it up now!)}

Thanks to that leading underscore, instead of this:

public clPart CurrentSelectedPart
{
    get { return this._CurrentSelectedPart; }
    set
    {
        if (this._CurrentSelectedPart == value)
            return;

        this._CurrentSelectedPart = value;
        OnPropertyChanged("CurrentSelectedPart");
    }
}

You can now do this:

private Part _selectedPart;
public Part SelectedPart
{
    get { return _selectedPart; }
    set
    {
        if (_selectedPart == value)
        {
            return;
        }

        _selectedPart = value;
        OnPropertyChanged("SelectedPart");
    }
}

..which brings me to my next poit.

#region

You don't need them. Remove them. All.

#region Variables
private ObservableCollection<clPart> _CollectionPart;
private clPart _CurrentSelectedPart;

private clDelegateCommand _EditCommand;
private clDelegateCommand _SaveCommand;
private clDelegateCommand _CancelCommand;
#endregion

"Variables" is too wide of a term - they're private fields. It could have been worse though:

#region private fields
private ObservableCollection<clPart> _CollectionPart;
private clPart _CurrentSelectedPart;
#endregion

#region commands
private clDelegateCommand _EditCommand;
private clDelegateCommand _SaveCommand;
private clDelegateCommand _CancelCommand;
#endregion

Now consider this:

class PartViewModel : AddEditDeleteViewModelBase
{
    public static readonly DelegateCommand EditCommand = new DelegateCommand(new Action<object>(EditPart);
    public static readonly DelegateCommand SaveCommand = new DelegateCommand(new Action<object>(SaveChanges), null);
    private static readonly DelegateCommand CancelCommand = new DelegateCommand(new Action<object>(CancelPart), null);

    private ObservableCollection<Part> _parts;
    public ObservableCollection<Part> Parts
    {
        get { return _parts; }
        set
        {
            if (_parts == value)
            {
                return;
            }

            _part = value;
            OnPropertyChanged("Parts");
        }
    }

    private Part _selectedPart;
    public Part SelectedPart
    {
        get { return _selectedPart; }
        set
        {
            if (_selectedPart == value)
            {
                return;
            }

            _selectedPart = value;
            OnPropertyChanged("SelectedPart");
        }
    }

Notice the public static readonly commands - keep it simple, there's no need for a getter here.

---

Commands

The DelegateCommand objects could just as well be in another code file, it wouldn't stop the XAML from being able to use it in a CommandBinding.

That's actually the beauty of these little commands there, they're delegate commands - the view model needs only to expose a public method that anyone can access and say "that method will be called with that command".

But your methods are all private - you've basically made your class untestable.

private void EditPart(object obj)
{
    this.SetEditUIDisplay();
}

Also I think it would be clearer to qualify the inherited method SetEditUIDisplay with base instead of this, since the method is inherited from the base.

public void EditPart(object obj)
{
    base.SetEditUiDisplay();
}

And now you can call EditPart from test code, ... ...and test that the base class works. I don't like this. What's SetEditUIDisplay doing anyway?

Ah-ha!

public void SetNormalUIDisplay()
{
    this.IsListEnabled = true;

    this.IsAddMode = false;
    this.IsDetailEnabled = false;
}

public void SetEditUIDisplay()
{
    this.IsAddMode = true;
    this.IsDetailEnabled = true;

    this.IsListEnabled = false;
}

Why aren't the fields assigned in the same order in the two methods? Why are there two methods? Why isn't IsAddMode called IsEditMode if it's only enabled in EditUI?

From a MVVM standpoint, what does a view model have to care about whether a list is enabled or not? This is strictly a presentation concern, that would be best handled by the view.

Here:

<Button x:Name="btnSaveDetails" IsEnabled="{Binding Path=IsDetailEnabled}" Command="{Binding SaveCommand}">

Your commands have a CanExecute method, that you're not using. When a Button has a command binding, IsEnabled is automagically handled by the command itself.

The x:Name attribute is only needed if you need to access a control from the code-behind; that's good news, you can remove all that extraneous markup!

If the command is a public static readonly field of a class your XAML has access to, it could look like this:

<Button Command="{x:Static local:SomeClass.SaveCommand}">

Where local is a CLR namespace you've defined in the XAML header, and SomeClass is the class that contains the static SaveCommand instance.

I wouldn't bother with <Button.CommandParameters> either, since the value of txtPartId.Text is irrelevant, it's the underlying SelectedPart.Id that we're really after... and that value is already known by the view model, and the logic that determines what to do when the SaveCommand is fired, is all in the view model anyway; I find not passing a command parameter much cleaner.

Taking a value from the view model, binding it to a text box, clicking a button and then passing that value from the text box, into the view model as a parameter, seems to me like flying from New York to Paris to Boston: a car would have been enough.

---

Construction

This comment is undoubtedly useless:

/// <summary>
/// Cunstructor of the clPartViewModel class
/// </summary>
public clPartViewModel()

The body of your default constructor is interesting:

public clPartViewModel()
{
    this.CollectionPart = new ObservableCollection<clPart>
    {
        new clPart { ID = "[0000001]" },
        new clPart { ID = "[0000002]" },
        new clPart { ID = "[0000003]" },
        new clPart { ID = "[0000004]" },
        new clPart { ID = "[0000005]" }
    };

    this.SetNormalUIDisplay();
}

I think it would be more flexible to pass an IEnumerable<Part> as a constructor parameter; the ViewModel doesn't really care where the data comes from. This removes a responsibility to the view model, and brings it closer to the Single Responsibility Principle.

public PartViewModel(IEnumerable<Part> parts)
{
    _parts = new ObservableCollection<Part>(parts);
}

Now the data might as well come from a hard-coded enumerable that the client code comes up with, or from a file, a database, a stone tablet or a satellite. The view model doesn't care about how its data (model) is created. It just needs a model.

---

There's so much more to say about this code, I'll stop here for now.

Answer 2

Picking up some of what Mat's Mug left

clDelegateCommand

public event EventHandler CanExecuteChanged
{
    add { CommandManager.RequerySuggested += value; }
    remove { CommandManager.RequerySuggested -= value; }
}

I use to call CommandManager.RequerySuggested -= value; before I add it. Doing this won't lead to any problems, if the same value hasn't been subscibed to the eventhandler, but it ensures that it isn't subscribed twice. Assume you already subscribed to the handler and do it again, you will receive the same event 2 times.

 public event EventHandler CanExecuteChanged
 {
     add 
     { 
         CommandManager.RequerySuggested -= value; 
         CommandManager.RequerySuggested += value; 
     }
     remove { CommandManager.RequerySuggested -= value; }
 }

clBaseAddEditDeleteViewModel and clPart

Here a check is missing for the properties changed

public bool IsListEnabled
{
    get { return this._IsListEnabled; }
    set
    {
        this._IsListEnabled = value;
        OnPropertyChanged("IsListEnabled");
    }
}

The event is raised each time the property is set. It should be only raised if the property is changed instead.

public bool IsListEnabled
{
    get { return this._IsListEnabled; }
    set
    {
        if (this._IsListEnabled == value) {return;}
        this._IsListEnabled = value;
        OnPropertyChanged("IsListEnabled");
    }
}

Check all of your properties.

Style

Sometimes you use braces {} for single if statements, sometimes you don't.
Be consequent on the style you choose. Many prefer to use always braces for single if statement or single lined for loops, so do I.