Transitioning a NavigationPane to use MVVM

Question

A friend of mine asked for help with his UWP app. Me being a .NET developer with an okay amount of WPF experience decided I could give a hand as well as give some good tips and tricks. After a few days of me learning and reading and comparing the two technologies I can see a lot of similarities. As I've learned in the past, though it isn't always a matter of "Yes you could do it this way but you shouldn't", I found out that UWP apps support the MVVM pattern and after a few failed attempts I was able to finally make a Core project (some people call it a Shared, or Common) that the UWP app could use and I could make a 3rd project for unit testing (for when I want to start adding testing to the logic of this already complete and published UWP app).

The first thing I wanted to do was to find the smallest and easiest bit of code that I could take out of the MainPage's code behind and move it to a NavigationViewModel so I could have it code reviewed and see if I should continue down this path. So with much more talking here I go. In the Core project I made a INagivationViewModel.

namespace TheocraticCalendar.Core
{
    public interface INavigationViewModel
    {
        bool IsExpanded { get; }
        int NavigationPanelWidth { get; }

        void SetDeviceFamily(string family);
        void CollapseNavigationPane();
        void ToggleNavigationPaneExpansion();
    }
}

In a new folder I wired up a very simplistic NotificationBase that I found from someplace:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Runtime.CompilerServices;

namespace TheocraticCalendar.ViewModels
{
    public class NotificationBase : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        protected bool SetProperty<T>(ref T field, T value, [CallerMemberName] string property = null)
        {
            if (EqualityComparer<T>.Default.Equals(field, value)) return false;
            field = value;
            RaisePropertyChanged(property);
            return true;
        }

        protected bool SetProperty<T>(T currentValue, T newValue, Action DoSet, [CallerMemberName] string property = null)
        {
            if (EqualityComparer<T>.Default.Equals(currentValue, newValue)) return false;
            DoSet.Invoke();
            RaisePropertyChanged(property);
            return true;
        }

        protected void RaisePropertyChanged(string property)
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(property));
        }
    }

    public class NotificationBase<T> : NotificationBase where T : class, new()
    {
        protected T This;

        public static implicit operator T(NotificationBase<T> thing) { return thing.This; }

        public NotificationBase(T thing = null)
        {
            This = (thing == null) ? new T() : thing;
        }
    }
}

Then wired up a NavigationViewModel:

using TheocraticCalendar.Core;

namespace TheocraticCalendar.ViewModels
{
    public class NavigationViewModel : NotificationBase, INavigationViewModel
    {
        public bool IsExpanded
        {
            get { return _expanded; }
            private set { SetProperty(ref _expanded, value, nameof(IsExpanded)); }
        }

        public int NavigationPanelWidth
        {
            get { return _navigationPanelWidth; }
            private set { SetProperty(ref _navigationPanelWidth, value, nameof(NavigationPanelWidth)); }
        }

        private bool _expanded;
        private int _navigationPanelWidth;

        public void SetDeviceFamily(string family)
        {
            if (family != "Windows.Mobile")
                NavigationPanelWidth = 50;
        }

        void INavigationViewModel.CollapseNavigationPane()
        {
            IsExpanded = false;
        }

        void INavigationViewModel.ToggleNavigationPaneExpansion()
        {
            IsExpanded = !IsExpanded;
        }
    }
}

I then added this to the MainPage's ctor (I didn't rename the NavigationPanel so it is still MySplitView):

    private readonly Core.INavigationViewModel _navigationViewModel;
    public MainPage()
    {
        this.InitializeComponent();
        CultureInfo.CurrentUICulture = App.culture;
       // other code emitted for brevity
        _navigationViewModel = new ViewModels.NavigationViewModel();
        MySplitView.DataContext = _navigationViewModel;
    }

on the navigation's button Tapped event (when a user taps on a navigation item:

    private void ListBoxItem_Tapped(object sender, TappedRoutedEventArgs e)
    {
        _navigationViewModel.CollapseNavigationPane();
       // other code emitted for brevity, but which 
       // TODO: move navigation logic code the navigation panel view model

    }

when the hamburger button is tapped:

    private void HamburgerButton_Click(object sender, RoutedEventArgs e)
    {
        _navigationViewModel.ToggleNavigationPaneExpansion();
    }

Finally, on the MainPage.OnLoaded event (which I might submit another code review for as it sits this moment because I have no clue how I'm going to clean it up:

    private void Page_Loaded(object sender, RoutedEventArgs e)
    {
        //21 lines of code having to do with settings
        _navigationViewModel.SetDeviceFamily(AnalyticsInfo.VersionInfo.DeviceFamily);
        //27 more lines of code with more logic!!!
    }

Is this a good move to start down this path? Thankfully the project isn't very big (compared to code base I have at work). Is it fair to treat much of the code like I would in WPF land? Can I use DI (I have yet to find a solid yes or no answer to this?

Sometimes I feel like some of the SO answers are people looking at the UWP code and thinking it's WPF and put in their two cents without thinking... like I did one time thinking objective-C was the same language as C#).

Answer 1

Sometimes I feel like some of the SO answers are people looking at the UWP code and thinking it's WPF and put in their two cents without thinking...

And yet there are only two lines related directly to UWP:

if (family != "Windows.Mobile")
    NavigationPanelWidth = 50;

and

_navigationViewModel.SetDeviceFamily(AnalyticsInfo.VersionInfo.DeviceFamily);

---

I wired up a very simplistic NotificationBase that I found from someplace:

People shouldn't use someone else's code if they don't know how to do it correctly. As a matter of fact the most part of the question is based on this borrowed code and should actually be closed because this is considered off-topic.

Let's take for example this API:

protected bool SetProperty<T>(ref T field, T value, [CallerMemberName] string property = null)

That you call like this

private set { SetProperty(ref _navigationPanelWidth, value, nameof(NavigationPanelWidth)); }

Its signature suggests it would expect three parameters but it doesn't. It requires only two of them. The third one is provided by the compiler thus the CallerMemberName attribute on the last of them.

The correct call should be:

private set { SetProperty(ref _navigationPanelWidth, value); }

I also wonder what's the purpose of returning a bool if this is nowhere used.

The same with checking the equality of the new and old values:

if (EqualityComparer<T>.Default.Equals(field, value)) return false;

How often will it happen that the same value is set again? Like never. And if something is triggering this (often) then there is a bug that needs to be fixed at the source and not prevented from affecting the performance by checking the new value.

---

Is it fair to treat much of the code like I would in WPF land? Can I use DI?

You've removed most parts of the application leaving only three one-line event handlers without context and a notifier that is not yours. There is nothing to review anymore and I don't know any reason why you shouldn't use DI.