WPF MVVM Navigation

Following a lot of research and mainly based on this tutorial I came up with the following structure in my application:

ObjectBase - all view models inherit it:

public abstract class ObjectBase : INotifyPropertyChanged
{
    #region INotifyPropertyChanged

    public event PropertyChangedEventHandler PropertyChanged = delegate { };

    protected void SetProperty(ref T member, T val,
       [CallerMemberName]string propertyName = null)
    {
        if (object.Equals(member, val)) return;

        member = val;
        PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
    }

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

    #endregion INotifyPropertyChanged

    public abstract ObjectBase BackLocation { get; }

    public abstract event Action NavigateTo;

    public abstract string ViewHeader { get; }
}

Main Content ViewModel:

```
public class MainContentViewModel : ObjectBase
{
private ObjectBase _selectedView;
public ObjectBase SelectedView
{
get { return _selectedView; }
set
{
if (SelectedView != null)
{
SelectedView.NavigateTo -= SelectedView_NavigateTo;
}
if (SelectedView is IDisposable)
{
(SelectedView as IDisposable).Dispose();
}

SetProperty(ref _selectedView, value);
SelectedView.NavigateTo += SelectedView_NavigateTo;
}
}

public MainContentViewModel()
{
SelectedView = new ViewModels.FirstViewModel();
}

private void SelectedView_NavigateTo(ObjectBase viewToNavigateTo)
{
SelectedView = viewToNavigateTo;
}

#region Back Command
private CommandBase _backCommand;

public CommandBase BackCommand
{
get { return _backCommand ?? (_backCommand = new CommandBase(Back)); }
}

private void Back(ob

• I think ObjectBase name is too generic. Class name should reflect its purpose. ViewModelBase is an example of better naming. Same goes for FirstViewModel and SecondViewModel.

• Using as operator after using is should generally be avoided. You should either use is and then use strong cast, or use as and then a null check. The second option is usually better since this way you only cast object once.

• You should split your ObjectBase class in two. One class to contain INotifyPropertyChanged implementation, which you will use in your view models (ViewModelBase : INotifyPropertyChanged), and another class to contain page-specific properties, such as header, "back" reference, etc. (PageViewModelBase : ViewModelBase).

• SelectedView is really confusing name for a property, which holds a view model reference.

• There is a potential issue with your SelectedView setter. Calling SelectedView = SelectedView will probably crash your software, as it will leave SelectedView in disposed state. You should check viewmodels for equality first, and only then you should dispose the old viewmodel (your SetProperty method does not currently allow that).

• Your Back method has a similar issue, as it might set SelectedView to already disposed object.

• I would also use Stack in your MainContentViewModel to store back locations instead of storing them in each individual page. Otherwise you will run into issues for more complex scenarious such as "page1 -> page2 -> page3 -> page2". Will you be able to return to page1? Or will you circle between page2 and page3 instead?

• I think you should avoid using events in this scenario, it complicates things too much. Use interfaces instead.

Might look like this:

interface IPageManager
{
    void Back();
    void Forward() where T : IPage;
}

interface IPage
{
    bool CanRestore { get; }
    string Header { get; }
    void Open();
    void Close();
}

class MainContentViewModel : ViewModelBase, IPageManager
{
    private Stack _navigationHistory = new Stack();

    private IPage _selectedPage;
    public IPage SelectedPage
    {
        get { return _selectedPage; }
        set
        {
            if (Object.Equals(_selectedPage, value)) return;
            if (_selectedPage != null)
            {
                _selectedPage.Close();
            }
            SetProperty(ref _selectedPage, value);
            _selectedPage.Open();
        }
    }

    public void Back()
    {
        while (_navigationHistory.Any())
        {
            var page = _navigationHistory.Pop();
            if (!page.CanRestore) continue;
            SelectedPage = page;
            return;
        }

        Application.Current.MainWindow.Close();
    }

    public void Forward() where T : IPage
    {
        _navigationHistory.Push(SelectedPage);
        SelectedPage = YourViewModelFactory.Create();
    }

   ........
}

class LoginViewModel : ViewModelBase, IPage
{
    private readonly IPageManager _manager;

    public LoginViewModel(IPageManager manager)
    {
        _manager = manager;
    }

    public string Header
    {
        get { return "Login"; }
    }

    public bool CanRestore 
    {
        get { return true; }
    }

    public void Open()
    {
    }

    public void Close()
    {
    }

    private CommandBase _goForwardCommand;
    public CommandBase GoForwardCommand
    {
        get { return _goForwardCommand ?? (_goForwardCommand = new CommandBase(GoForward)); }
    }
    private void GoForward(object obj)
    {
        _manager.Forward();
    }
}

StackExchange Code Review Q#101784, answer score: 10