Clean View Models

Let’s talk about clean code.

I hope that there is no point in me writing a paragraph on why creating a clean code is important. But in short: less bugs, better maintainability and shorter change cycles.

As we all agree on that, let’s jump into a more specific example. How to create clean view models? That’s the question I first asked myself when I was introduced to the WPF. Since then I’ve worked on few different desktop applications and I think that I’ve finally settled on a small set of rules I really like to follow.

Of course a little disclaimer is in place as something that seems clean to one person might not by clean to another. So I’m definitely not encouraging you to follow those rules as if they were the one and only truth. But I hope that you will find at least some of them useful, especially if it’s your first time with the WPF (or any other C# GUI framework).

And as you might have already guessed, this post is focused mostly around C# and WPF. Of course some of those tips can also be applied to different technologies, but others will be quite specific to this one.

(BTW, in this post I assume that you know what view models are, how they work and what are their advantages and disadvantages).

Let’s kick off by looking at a piece of code that I would not consider to be clean. Please, take a moment to understand what it does – it’s relatively simple.

[Jump to the end of the post to see the final result of cleaning it up]

public class ProductViewModel : INotifyPropertyChanged
{
	private string _name;
	private ProductCategory _category;
	private decimal _price;
	private bool _isImported;
	private Visibility _detailsVisibility;

	public ProductViewModel(string name)
	{
		_name = name;
	}

	public string Name
	{
		get => _name;
		set
		{
			_name = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
		}
	}

	public decimal Price
	{
		get => _price;
		set
		{
			_price = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

	public ProductCategory Category
	{
		get => _category;
		set
		{
			_category = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Category)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

	public bool IsImported
	{
		get => _isImported;
		set
		{
			_isImported = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsImported)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

	public decimal Tax
	{
		get
		{
			if (!IsImported)
				return 0;

			switch (Category)
			{
				case ProductCategory.Car:
					return Price * 0.3m;
				case ProductCategory.Food:
					return Price * 0.2m;
				default:
					return Price * 0.1m;
			}
		}
	}
		
	public Visibility DetailsVisibility
	{
		get => _detailsVisibility;
		set
		{
			_detailsVisibility = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(DetailsVisibility)));
		}
	}

	public event PropertyChangedEventHandler PropertyChanged;
}

Of course, please, don’t understand me wrong. This code serves its purpose and doesn’t contain any bugs (maybe just some performance issues). So if you are creating code like this, that’s still ok. But let’s try to improve it.

I’ve mentioned that this code contains some performance issues. Where? Unfortunately, in every setter.

Our mistake is that we don’t check if a new value is actually new. Even if it’s the same as the previously stored value, we are still assigning it to the field (which is not that big of a deal) and invoke the PropertyChanged event (which is a problem).

We don’t actually know who might be subscribing to this event. Raising it might lead to redrawing of the UI or even spawning of some heavy background workers. It’s important to call the PropertyChanged event only if a given property has actually changed.

Let’s fix it by adding proper checks.

public class ProductViewModel : INotifyPropertyChanged
{
...

	public string Name
	{
		get => _name;
		set
		{
			if (_name == value) return;
			_name = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
		}
	}

	public decimal Price
	{
		get => _price;
		set
		{
			if (_price == value) return;
			_price = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

	public ProductCategory Category
	{
		get => _category;
		set
		{
			if (_category == value) return;
			_category = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Category)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

	public bool IsImported
	{
		get => _isImported;
		set
		{
			if (_isImported == value) return;
			_isImported = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsImported)));
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

...
		
	public Visibility DetailsVisibility
	{
		get => _detailsVisibility;
		set
		{
			if (_detailsVisibility == value) return;
			_detailsVisibility = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(DetailsVisibility)));
		}
	}

...
}

Did we fix it? Unfortunately not for all cases. What are we missing? The Tax property. If I have a product which is not imported and I change it’s price to something new I get notified that the Tax has changed. Even thought it’s still zero – the same as before.

How can we introduce a check that would prevent this from happening? Maybe one way to do it would be to see if our product is imported before invoking the PropertyChanged event in the Price setter. I definitely do not recommend that. It’s probably event better to live the code as it is now then to scatter the Tex calculation logic all around the place. At some point we might change our tax equation so that all of the products are taxed (even when not imported, just with a different rate) and forget to remove this check. It would be hard to debug it as in some cases the event would be raised and in others not.

So what is my advice? I think it’s best to extract the tax calculation logic to a separate method and add a _tax field which would store the current value. In fact, for view models, I would always discourage creating read only properties with complex getters. Instead we should use separate methods that update those properties whenever a change might be needed.

public class ProductViewModel : INotifyPropertyChanged
{
...
	private decimal _tax;
...

	public decimal Price
	{
		get => _price;
		set
		{
			if (_price == value) return;
			_price = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
			UpdateTax();
		}
	}

	public ProductCategory Category
	{
		get => _category;
		set
		{
			if (_category == value) return;
			_category = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Category)));
			UpdateTax();
		}
	}

	public bool IsImported
	{
		get => _isImported;
		set
		{
			if (_isImported == value) return;
			_isImported = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsImported)));
			UpdateTax();
		}
	}

	public decimal Tax
	{
		get => _tax;
		private set
		{
			if (_tax == value) return;
			_tax = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}

...

	private void UpdateTax()
	{
		if (!IsImported)
			Tax = 0;
		else
		{ 
			switch (Category)
			{
				case ProductCategory.Car:
					Tax = Price * 0.3m;
					break;
				case ProductCategory.Food:
					Tax = Price * 0.2m;
					break;
				default:
					Tax = Price * 0.1m;
					break;
			}
		}
	}

...
}

Now it’s much better. We call the PropertyChanged event only when it’s actually needed.

We’ve got rid off those nasty performance issues. But unfortunately our code became even longer. And on top of that the view model still contains some code that doesn’t really belong there. I’m talking about the tax calculation logic. In this case it’s relatively simple, but in practice it might require performing a database query or some more advanced calculations.

Let’s move it to a separate class.

public class ProductViewModel : INotifyPropertyChanged
{
	private readonly TaxService _taxService;

...

	public ProductViewModel(TaxService taxService, string name)
	{
		_taxService = taxService;

		_name = name;
	}

...

	private void UpdateTax()
	{
		Tax = _taxService.CalculateTax(Price, Category, IsImported);
	}

...
}

My rule here is that I always question if a property Update method is not to long when it has more then 2 lines. One for a precondition check (checking if the value should be calculated in the first place) and one for the calculation and assignment (which internally checks if the value is actually new). Of course it’s just a rule of thumb and longer constructions are also allowed, though should always be verified.

This is more of an observation then a rule, but I’ve noticed that those Update methods are usually parameterless, don’t return any values and there is no more then one of them for a single property. That’s why I call them Update methods, as Update[PropertyName] is usually the best name you can give them (and calling them like that keeps the code consistent and predictable).

From now on setters should only perform the initial checks, do the assignments to their back fields, raise the PropertyChanged events and call the Update methods of all dependent properties (if needed). It extremely simplifies the process of keeping track of internal dependencies. When you create a new Update method or modify an existing one, all you have to do is to make sure that all of the properties used within it, call it in their setters. You have to make it your habit (without it you might end up with a view models in a corrupted state).

Next issue in our code is in the constructor. Within it we assign a string to the backing field of the Name property. Currently it’s ok, but what would happen if we were to introduce a new property: ShortName, consisting of first 3 letters of the full Name? The code would probably look something like this:

public class ProductViewModel : INotifyPropertyChanged
{
...
	private string _name;
	private string _shortName;
...

	public ProductViewModel(TaxService taxService, string name)
	{
		_taxService = taxService;

		_name = name;
	}

	public string Name
		{
		get => _name;
		set
		{
			if (_name == value) return;
			_name = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
			UpdateShortName();
		}
	}

	public string ShortName
	{
		get => _shortName;
		set
		{
			if (_shortName == value) return;
			_shortName = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ShortName)));
		}
	}

...

	private void UpdateShortName()
	{
		ShortName = Name.Substring(0, Math.Min(3, Name.Length));
	}

...
}

We’ve avoided calling the PropertyChanged event multiple times for the ShortName if the first 3 letters of the Name have not changed, but ended up with a serious bug. After we create an object with the name provided in the constructor we don’t update the ShortName property. The state of our view model becomes corrupted.

Of course we could call the UpdateShortName method in the constructor, but it’s not really a good solution. It would leave us with this thought in the back of our heads that we have to keep track of performed changes to manually trigger state reevaluation. Urgh.

And all of that even though we already have a piece of code that does this for us – the Name setter.

This is how we’ve came to the next, very important rule: never access backing fields outside of their front properties. To emphasise this let’s use code locality and move our fields closer to where they should only be used from (and also let’s fix the constructor).

public class ProductViewModel : INotifyPropertyChanged
{
	private readonly TaxService _taxService;
		
	public ProductViewModel(TaxService taxService, string name)
	{
		_taxService = taxService;

		Name = name;
	}
		
	private string _name;
	public string Name
		{
		get => _name;
		set
		{
			if (_name == value) return;
			_name = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
			UpdateShortName();
		}
	}
		
	private string _shortName;
	public string ShortName
	{
		get => _shortName;
		private set
		{
			if (_shortName == value) return;
			_shortName = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ShortName)));
		}
	}
		
	private decimal _price;
	public decimal Price
	{
		get => _price;
		set
		{
			if (_price == value) return;
			_price = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
			UpdateTax();
		}
	}
		
	private ProductCategory _category;
	public ProductCategory Category
	{
		get => _category;
		set
		{
			if (_category == value) return;
			_category = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Category)));
			UpdateTax();
		}
	}
		
	private bool _isImported;
	public bool IsImported
	{
		get => _isImported;
		set
		{
			if (_isImported == value) return;
			_isImported = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsImported)));
			UpdateTax();
		}
	}
		
	private decimal _tax;
	public decimal Tax
	{
		get => _tax;
		private set
		{
			if (_tax == value) return;
				_tax = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Tax)));
		}
	}
		
	private Visibility _detailsVisibility;
	public Visibility DetailsVisibility
	{
		get => _detailsVisibility;
		set
		{
			if (_detailsVisibility == value) return;
			_detailsVisibility = value;
			PropertyChanged(this, new PropertyChangedEventArgs(nameof(DetailsVisibility)));
		}
	}
...
}

There is one more thing that I don’t like about our constructor. It’s the fact that it has two different responsibilities. It requests view model’s dependencies (in our case a TaxService instance) and initializes the state of the ViewModel (the Name property). Why is this an issue? Because if we were to use a dependency injection framework (what I really recommend doing) it will become tricky to instantiate those objects. Some of the parameters will be impossible to resolve for the DI engine and we will end up creating a multitude of custom factories. Nasty.

The rule here is for the constructor to only get resources the view model depends on, but not state related date. All values can be assigned after its creation. Overall we should think of view models the same way we think of GUI controls. They store and provide a state, usually related to a single entity, but this entity can change throughout their lifetime. In our case a product view model should be able to store information about a bike in one moment and about a TV a moment later. At first it might sound as something that complicates stuff quite a bit, but in fact it makes life much easier. We don’t have to live with this idea that some things can change and others are constant. From now on everything changes, but the nice thing is that everything changes in the same way. There are no special cases and hidden assumptions. And even if we never change the underlying product in out view model after the first assignment, the person that will jump into this project after us, will not have to discover those special constraints before making changes to the view model.

So far we’ve managed to clean up the interface of our view model. It no longer raises unnecessary events, it behaves in a consistent manner and it’s easy to instantiate. But internally it’s still a mess. The keyword here is: repetition.

And a remedy: a ViewModel base class. It should take care of checking constraints, raising events and calling Update methods of all properties that depend on the current one. This is an example implementation:

internal class ViewModel : INotifyPropertyChanged
{
	public ViewModelChangePropagation<T> Set<T>(ref T field, T value, [CallerMemberName] string propertyName = "")
	{
		var change = new ViewModelChange<T>(field, value);

		if (change.Changed)
		{
			field = value;
			PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
		}

		return new ViewModelChangePropagation<T>(change);
	}

	public event PropertyChangedEventHandler PropertyChanged;
}

class ViewModelChange<T>
{
	public T OldValue { get; }
	public T NewValue { get; }

	public bool Changed { get; }

	public ViewModelChange(T oldValue, T newValue)
	{
		OldValue = oldValue;
		NewValue = newValue;

		Changed = !(newValue?.Equals(oldValue) ?? false);
	}
}

class ViewModelChangePropagation<T>
{
	public ViewModelChange<T> Change { get; }

	public ViewModelChangePropagation(ViewModelChange<T> change)
	{
		Change = change;
	}

	public ViewModelChangePropagation<T> Then(Action action)
	{
		if (Change.Changed)
			action();

		return this;
	}
}

I will admit that I’m not 100% happy with this one (because of some excessive allocations). I’ve copied it from one of my old projects. But still, it gets the job done. The most important part here is the Set<T> method of the ViewModel class. Using the CallerMemberName attribute it gets the name of the property from which it’s called to rise the PropertyChanged event in case the value of our backing field has actually change. It also allows the caller to specify which fields depend on the current one (using the Then metod of the ViewModelChangePropagation class). Multiple dependencies can be chained together if necessary.

But enough talking, let’s see it in action.

public class ProductViewModel : ViewModel
{
	private readonly TaxService _taxService;
		
	public ProductViewModel(TaxService taxService)
	{
		_taxService = taxService;
	}
		
	private string _name;
	public string Name
	{
		get => _name;
		set => Set(ref _name, value)
			.Then(UpdateShortName);
	}
		
	private string _shortName;
	public string ShortName
	{
		get => _shortName;
		private set => Set(ref _shortName, value);
	}
		
	private decimal _price;
	public decimal Price
	{
		get => _price;
		set => Set(ref _price, value)
			.Then(UpdateTax);
	}
		
	private ProductCategory _category;
	public ProductCategory Category
	{
		get => _category;
		set => Set(ref _category, value)
			.Then(UpdateTax);
	}
		
	private bool _isImported;
	public bool IsImported
	{
		get => _isImported;
		set => Set(ref _isImported, value)
			.Then(UpdateTax);
	}
		
	private decimal _tax;
	public decimal Tax
	{
		get => _tax;
		private set => Set(ref _tax, value);
	}
		
	private Visibility _detailsVisibility;
	public Visibility DetailsVisibility
	{
		get => _detailsVisibility;
		set => Set(ref _detailsVisibility, value);
	}

	private void UpdateTax()
	{
		Tax = _taxService.CalculateTax(Price, Category, IsImported);
	}

	private void UpdateShortName()
	{
		ShortName = Name.Substring(0, Math.Min(3, Name.Length));
	}
}

After those changes all of the implementation details of the view model are safely hidden in the base class.

This is just a simple example of a ViewModel base class. In practice I’ve created quite few of them with much more complex interfaces that allowed me (also in a single call) to manage event subscriptions on external objects, automatically dispose of old values in case we are their owners, or even manage collections. But it doesn’t change the core idea that is very well visible also in the example above.

It’s worth mentioning that in the past I used to work with a different interface of a ViewModel base class. Instead of returning the ViewModelChangePropagation object from the Set method I used to return a boolean value indicating if the property has changed. My setters looked like this:

private bool _isImported;
public bool IsImported
{
	get => _isImported;
	set
	{
		if(Set(ref _isImported, value))
		{
			UpdateTax();
		}
	}
}

The problem here is not only about the fact that the code is a little bit longer (most of it are just brackets). The actual problem is a temptation to add some more complex logic into the body of the if statement – as nothing is stopping us from doing it. And a good design is not only abut making it easy to create proper code, but also about making it harder to create improper one (even when it comes to the cleanness of the code).

But I’m aware that following a patter like this might be a huge change, especially for existing projects. In a situation when you don’t want to use in its full form I would still recommend you to at least abstract away the checks for equality and raising of the PropertyChanged events into a separate base class.

And last, but not least, don’t try to enforce on a view the way in which it should manage its presentation layer. To be more precise, don’t use view-specific types (like Visibility enum or Color) in your view models. If you want to persist an information indicating if a given part of your UI should be visible (or highlighted) store it as a boolean. Let the view decide if it’s going to apply it by setting the visibility property of a control or by changing its opacity. This is not a responsibility of a view model to care about those details. This will not only increase the reusability of your code (though I must agree that sharing view models is rather rare), but also significantly lower their complexity in more advanced scenarios. Remember, converters are your friends.

This is the final form of out view model after applying all of those changes:

public class ProductViewModel : ViewModel
{
	private readonly TaxService _taxService;
		
	public ProductViewModel(TaxService taxService)
	{
		_taxService = taxService;
	}
		
	private string _name;
	public string Name
	{
		get => _name;
		set => Set(ref _name, value)
			.Then(UpdateShortName);
	}
		
	private string _shortName;
	public string ShortName
	{
		get => _shortName;
		set => Set(ref _shortName, value);
	}
		
	private decimal _price;
	public decimal Price
	{
		get => _price;
		set => Set(ref _price, value)
			.Then(UpdateTax);
	}
		
	private ProductCategory _category;
	public ProductCategory Category
	{
		get => _category;
		set => Set(ref _category, value)
			.Then(UpdateTax);
	}
		
	private bool _isImported;
	public bool IsImported
	{
		get => _isImported;
		set => Set(ref _isImported, value)
			.Then(UpdateTax);
	}
		
	private decimal _tax;
	public decimal Tax
	{
		get => _tax;
		private set => Set(ref _tax, value);
	}
		
	private bool _isExpanded;
	public bool IsExpanded
	{
		get => _isExpanded;
		set => Set(ref _isExpanded, value);
	}

	private void UpdateTax()
	{
		Tax = _taxService.CalculateTax(Price, Category, IsImported);
	}

	private void UpdateShortName()
	{
		ShortName = Name.Substring(0, Math.Min(3, Name.Length));
	}
}

Much cleaner.

Of course the topic is much broader, but those are the rules that I wanted to show you. I hope that you’ve found at least some of them useful and eye opening. And please, from now on let’s not only focus on cleanness of code in our back-end components, but also in parts of our projects that are closer to the GUI.

An example project where I was following presented rules can be found here: https://github.com/TomaszRewak/WaveDotNet

Cheers.

You may also like...

Leave a Reply

Your email address will not be published. Required fields are marked *