4
\$\begingroup\$

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<T>(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<ObjectBase> 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(object obj)
    {
        if (SelectedView.BackLocation != null)
        {
            SelectedView = SelectedView.BackLocation;
        }
        else
        {
            Application.Current.MainWindow.Close();
        }
    }
    #endregion Back Command

    #region ObjectBase Fields
    public override event Action<ObjectBase> NavigateTo;
    public override string ViewHeader { get { return "Main View"; } }
    public override ObjectBase BackLocation { get { return null; } }
    #endregion
}

Main Content View:

<Window x:Class="WpfNavigationTest.Views.MainContentView"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:views="clr-namespace:WpfNavigationTest.Views"
    xmlns:viewModels="clr-namespace:WpfNavigationTest.ViewModels">
<Window.DataContext>
    <viewModels:MainContentViewModel/>
</Window.DataContext>
<Window.Resources>
    <DataTemplate DataType="{x:Type viewModels:FirstViewModel}">
        <views:FirstView/>
    </DataTemplate>

    <DataTemplate DataType="{x:Type viewModels:SecondViewModel}">
        <views:SecondView/>
    </DataTemplate>
</Window.Resources>
<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="Auto"/>
        <RowDefinition Height="Auto"/>
        <RowDefinition Height="*"/>
    </Grid.RowDefinitions>
    <Button Content="Go Back" Command="{Binding BackCommand}"/>
    <Separator Margin="0,15" Grid.Row="1"/>
    <ContentPresenter Content="{Binding SelectedView}" Grid.Row="2"/>
</Grid>

Sample View Model 1:

public class FirstViewModel : ObjectBase
{
    #region GoForward Command
    private CommandBase _goForwardCommand;
    public CommandBase GoForwardCommand
    {
        get { return _goForwardCommand ?? (_goForwardCommand = new CommandBase(GoForward)); }
    }
    private void GoForward(object obj)
    {
        NavigateTo(new ViewModels.SecondViewModel(DateTime.Now.ToString()));
    }
    #endregion
    public override ObjectBase BackLocation { get { return null; } }
    public override string ViewHeader { get { return "First View"; } }

    public override event Action<ObjectBase> NavigateTo;
}

Sample View Model 2:

public class SecondViewModel : ObjectBase
{
    private string _addMessage;

    public SecondViewModel(string addedMessage)
    {
        _addMessage = addedMessage;
    }

    public override ObjectBase BackLocation { get { return new ViewModels.FirstViewModel(); } }

    public override string ViewHeader { get { return $"Second View. Message: {_addMessage}"; } }

    public override event Action<ObjectBase> NavigateTo;
}

In general, this works as expected but I would like to know if I'm at the right direction and how can I improve this?

\$\endgroup\$

1 Answer 1

10
\$\begingroup\$
  1. 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.
  2. 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.
  3. 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).
  4. SelectedView is really confusing name for a property, which holds a view model reference.
  5. 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).
  6. Your Back method has a similar issue, as it might set SelectedView to already disposed object.
  7. 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?
  8. 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<T>() where T : IPage;
}

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

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

    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<T>() where T : IPage
    {
        _navigationHistory.Push(SelectedPage);
        SelectedPage = YourViewModelFactory.Create<T>();
    }

   ........
}

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<WelcomeViewModel>();
    }
}
\$\endgroup\$
13
  • \$\begingroup\$ Thanks for the response. in regards to section 3, I don't see a SelectedView = SelectedView in my setter \$\endgroup\$
    – Yoav
    Aug 25, 2015 at 10:49
  • \$\begingroup\$ In regards to your recommendation about using a Stack I'm not sure how this can solve a situation where the back location may vary according to some logic. setting a new instance of the back location explicitly wouldn't make it safer to use? \$\endgroup\$
    – Yoav
    Aug 25, 2015 at 10:54
  • \$\begingroup\$ also, the main reason that I implemented this as above is in order to have the ability to pass parameters to the constructor of the forward location \$\endgroup\$
    – Yoav
    Aug 25, 2015 at 10:58
  • \$\begingroup\$ Its just an exmaple of a scenario, where your setter will dispose object and keep the disposed reference as your SelectedView. \$\endgroup\$
    – Nikita B
    Aug 25, 2015 at 11:03
  • \$\begingroup\$ I don't see any problems with navigating to different view depending on some context. You just have to call Forward<T> with different T, and thats it. If you also need to pass an argument, then you can add an argument. Make it void Foraward<T>(object args = null) or something. The point here is to separate viewmodel creation logic from the rest of your code (by implementing some kind of factory), otherwise you will find yourself copypasting huge chunks of code as soon as your navigation becomes more complex. \$\endgroup\$
    – Nikita B
    Aug 25, 2015 at 11:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.