Understanding the MVVM concepts and validation of my code

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:

• Do I follow the MVVM pattern?

• 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 _executionAction;
private Predicate _canExecutePredicate;

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

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

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

pu

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 _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 _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(EditPart);
    public static readonly DelegateCommand SaveCommand = new DelegateCommand(new Action(SaveChanges), null);
    private static readonly DelegateCommand CancelCommand = new DelegateCommand(new Action(CancelPart), null);

    private ObservableCollection _parts;
    public ObservableCollection 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:

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 it

StackExchange Code Review Q#64543, answer score: 12