Practical .NET

Separating the UI Code from the Business Code: What's Reasonable?

Peter Vogel moves a Windows Form to a modern design pattern, which forces him to think about how his code should be divided up among his classes. In the end, he comes up with some rules for making those decisions.

• By Peter Vogel
• 05/05/2014

In this column I'm going to look at a real-world application to address some issues about how code should be divided between the presentation layer and the rest of the application. While there's lots of code here (including a look at how to use the ObservableCollection class), it's the principles that result from structuring that code that I think really matter. If you want, you can think of this column as an obsessively detailed discussion of what the Single Responsibility Principle really means and how it applies in structuring your application.

Moving to Model-View-ViewModel
My client wants, as part of upgrading a Windows Forms application, to move to Test-Driven Development (TDD) and automated testing. I'm a big fan of TDD: it's easy to do, doesn't require any tools other than Visual Studio, improves the quality of my code, and fits easily with my development style (I'm more about evolving an application than meticulous planning/design).

But to support automated testing, I have to move existing code out from behind the Windows form and into a self-contained ViewModel class independent of the form. I can test a standalone ViewModel in an automated way because I don't need to interact with the form to run the ViewModel through its paces.

That gives me two goals in structuring my form. First, I want to have as little code in my form and as much code in my ViewModel class as possible. That goal maximizes the code under automated testing. Second, I want the code in the form to be as simple as possible. That goal minimizes the possibility of me getting anything wrong in the form (ideally, I want there to be no control logic in the form -- no If statements, no loops and so on).

The Form's Responsibility
My question is: How do I decide what code I should keep in the form? At least one piece of code has to be present in the form: I need to instantiate my ViewModel class and store it somewhere. I do that in a read-only property that exposes the form's ViewModel to other code in the same project:

Private _VM As InvoicePremiumVM
Friend ReadOnly Property VM As InvoicePremiumVM
  Get
    If _VM Is Nothing Then
      _VM = New InvoicePremiumVM
    End If
    Return _VM
  End Get
End Property

In this design, events fired by controls in the form will do one of two things: either pass data from the form to properties in my ViewModel, or call methods on my ViewModel (the methods in the ViewModel will then use the data in the ViewModel properties to do their work).

As an example, the method in Listing 1 both passes data to the ViewModel and calls a method in it when the user clicks a button.

Private Sub btnNew_Click(...) Handles btnNew.Clicked
  Me.Cursor = System.Windows.Forms.Cursors.WaitCursor
  Dim NewAction As String = String.Empty
  If rbNew.Checked Then
    NewAction = ActionTypes.NewItem
  ElseIf rbReDo.Checked Then
    NewAction = ActionTypes.ReDo
  End If
  If NewAction <> VM.Action Then
    VM.Action = NewAction
    VM.RefreshCompanies()
  End If
  Me.Cursor = System.Windows.Forms.Cursors.Arrow
End Sub

While all the business logic is in the ViewModel class, this example still leaves a lot of code in the form. For instance, should the form decide on its own whether to display the WaitCursor? I could have my ViewModel class flag long-running methods through some property on the class. If I did that, the value of the LongRunning property on the ViewModel could just be wired up to the form's Cursor property:

If VM.IsRefreshCompaniesLongRunning Then
  Me.Cursor = System.Windows.Forms.Cursors.WaitCursor
End If
...rest of method...
If Me.Cursor = System.Windows.Forms.Cursors.WaitCursor then
  Me.Cursor = System.Windows.Forms.Cursors.Arrow
End If

I decided that, even if that was the right thing to do, it was more work than I was willing to invest -- I'd need to add a LongRunning property to my ViewModel (and set it correctly) for every method on my ViewModel. That just struck me as an opportunity for error. I'd also need to add the two If blocks around every call to a method on my ViewModel. Because I'm trying to avoid having control logic in my form, that was another strike against this design. I stayed with my original design and let the form decide when to change the cursor.

Controlling Processing
This example still includes the logic to decide whether to call the RefreshCompanies method in the form. Because I'm trying to eliminate control logic from my form, I move that code into the ViewModel. My RefreshCompanies method now begins like this:

Private OldAction As String
Friend Sub RefreshCompanies()
  If OldAction = Action Then
    Exit Sub
  Else
    OldAction = Action
  End If
  ...rest of method...

With that code gone, my form method gets simpler and ends up looking like this:

Me.Cursor = System.Windows.Forms.Cursors.WaitCursor
If rbNew.Checked Then
  VM.Action = ActionTypes.NewItem
ElseIf rbReDo.Checked Then
  VM.Action = ActionTypes.ReDo
End If
VM.RefreshCompanies()
Me.Cursor = System.Windows.Forms.Cursors.Arrow

But that leads to another question: Should my ViewModel class require the form to translate the radio buttons rbNew and rbReDo into the updates to the Action property? My ViewModel could have a separate Boolean property for each Action and let the form just set them without an If statement, like this:

VM.NewItem = rbNew.Checked
VM.ReDoItem = rbReDo.Checked

While that eliminates an If statement in my form, it also opens up the possibility of an error: The business process only supports having one Action picked at a time (either New or ReDo). This redesign would let the form lead the ViewModel into error by setting both the NewItem and ReDoItem properties. I'd have to make the ViewModel more complicated to check for and handle this error. The original code, while it violated my guideline around removing control logic from the form, is very simple: If I do make a mistake in setting the ViewModel Action property, it will be both obvious and easy to fix. For these reasons, I'll keep the If statements for setting the Action property in my form.

Managing the Display
Listing 2 shows the method in my form's code file where I centralized all of my screen management. The method decides (among other things) what controls are visible.

Private Sub FormatScreen()
  If VM.Action = InvoiceAction.NewInvoice Then
    lblnewSInvoice.Visible = False
    ...more properties set...
    dgvDetails.Size = New Size(dgvDetails.Size.Width, 540)
    dgvDetails.Columns("InvoiceNumber").Visible = False
  Else
    lblnewSInvoice.Visible = True
    txtSInvoice.Visible = True
    ...more properties set...
    dgvDetails.Size = New Size(dgvDetails.Size.Width, 315)
    dgvDetails.Columns("InvoiceNumber").Visible = True
  End If

This adds a lot of code to my form (including another If statement). I have some options here that could simplify my code. First, I could add a property on my ViewModel for each control that manages the control's visibility. That would let me make the controls visible or invisible without an If statement, like this:

lblnewSInvoice.Visible = VM.IsInvoiceVisible
txtSInvoice.Visible = VM.IsInvoiceVisible
grpPrintOptions.Visible = VM.IsPrintOptionsVisible
...more settings...

But that design is just going to create work for me: Changes to my UI will now force me to add new properties to my ViewModel. Looked at another way, the properties on my ViewModel would decide what controls I could have on my form.

As an alternative, I could have a single property on my form that flags to the form what data (controls) to display. In fact, the Action property that I'm testing does that; but it's not a Boolean, so I can't use it to directly update the Visible property of the form's controls. Furthermore, not all the values I'm setting are Booleans: one statement in this block sets the size of a control. For these reasons, I settle for a simple rewrite that leaves me with a single If statement but removes much of the code:

Dim MoreDataVisible As Boolean

If VM.Action = InvoiceAction.NewInvoice Then
  MoreDataVisible = False
  dgvDetails.Size = New Size(dgvDetails.Size.Width, 540)
Else
  MoreDataVisible = True
  dgvDetails.Size = New Size(dgvDetails.Size.Width, 315)
End If
lblnewSInvoice.Visible = MoreDataVisible
txtSInvoice.Visible = MoreDataVisible
...more settings...

Getting Data to the Form
I also need to address how the form will know that there's data to display -- when will the form check the properties on my ViewModel class and move that data to the controls? This form is different from most because it consists almost entirely of lists: a combobox, a checkbox list and a grid. I can use the .NET ObservableCollection to tell the form when to update its controls (for "non-list" properties in my ViewModel, I'd use the INotifyPropertyChanged interface -- a topic I'll discuss in a later column).

The first step in getting list data out of my ViewModel and into the form is to declare the collections in my ViewModel as ObservableCollections:

Friend Property Companies As New ObservableCollection(Of Company)
Friend Property Invoices As New ObservableCollection(Of Invoice)
Friend Property Groups As New ObservableCollection(Of String)

It would be great if I could just tie these collections to the DataSource properties on the listing controls on my form. The reality, though, is that I frequently have processing to do in managing those controls. As a result, the next step -- after instantiating my ViewModel -- is to wire up a method to each ObservableCollection's CollectionChanged property so I have some place to put that processing code:

VM = New MyVM
AddHandler VM.Invoices.CollectionChanged, AddressOf CompaniesChanged
AddHandler VM.Invoices.CollectionChanged, AddressOf InvoicesChanged
AddHandler VM.Groups.CollectionChanged, AddressOf GroupsChanged

The CollectionChanged event passes these methods an e parameter with several useful properties, two of which I use. The property called Action reports what change was made to the collection (whether items were added, removed or the collection was reset/cleared). The other property I use is NewItems, which lists all of the items that have been added to the collection since the last time the event was raised.

In this form, I typically need to handle two scenarios: When the ViewModel collection is cleared/reset and when one or more items are added to the ViewModel collection. My ViewModel Invoices collection is displayed in a grid that sees a lot of interaction with the user, so to support that, I set the grid's DataSource to a BindingList. In the CollectionChanged method for the Invoices, I check to see if there are any new items and, if there aren't, I set the grid's DataSource to an empty BindingList. If there are some NewItems, I add them to the BindingList in the grid's DataSource:

Private Sub InvoicesChanged(sender As Object, e As NotifyCollectionChangedEventArgs)
  Select e.Action
    Case NotifyCollectionChangedAction.Reset
      dgvDetails.DataSource = New BindingList(Of Invoice)
    Case NotifyCollectionChangedAction.Add
      For Each inv As Invoice In e.NewItems
        CType(dgvDetails.DataSource, BindingList(Of Invoice)).Add(inv)
      Next
  End If
End Sub

Some controls, however, don't have a DataSource property, so I have to work with those controls' Items collection. A CollectionChanged event handler for those controls is marginally simpler than my previous method:

Private Sub GroupsChanged(sender As Object, e As NotifyCollectionChangedEventArgs)
  Select e.Action
    Case NotifyCollectionChangedAction.Reset
      cblGroup.Items.Clear()
    Case NotifyCollectionChangedAction.Add
      For Each grp As String In e.NewItems
        cblGroup.Items.Add(grp, True)
      Next
  End If
End Sub

For some controls, I need to do a little more than just update their collections. This example adds a default first item to a combobox and ensures the item is displayed in the control when the corresponding collection is cleared:

Private Sub CompaniesChanged(sender As Object, e As NotifyCollectionChangedEventArgs)
  Select e.Action
    Case NotifyCollectionChangedAction.Reset
      cmbCompany.Items.Clear()
      cmbCompany.Items.Add("-- All Companies --")
    Case NotifyCollectionChangedAction.Add
      For Each cmp As New_Item In e.NewItems
        cmbCompany.Items.Add(cmp)
      Next
  End Select
  Me.cmbCedingCompany.SelectedIndex = 0
End Sub

Testing the ViewModel
I can now make sure my ViewModel code does what it's supposed to do without interacting with the form. In my tests, I move data into the ViewModel properties, call the ViewModel methods, and then check other properties on the ViewModel to see if the ViewModel has done the right thing:

Dim piVM As InvoicePremimumVM = New InvoicePremimumVM()
piVM.CompanyNumber = 19
piVM.RefreshCompanies()
Assert.IsNotNull(piVM.SelectedInvoices, "Selected Invoices not initialized")
Assert.AreNotEqual(0, piVM.SelectedInvoices.Count, "SelectedInvoices not loaded")

But I can't avoid doing some integration testing between the form and the ViewModel. For instance, if you set a checkbox Checked property at design time, the designer will set that property during the form's initialization, triggering an event in the form. That event might call code in the ViewModel that, in turn, triggers code back in the form that updates some other control on the form. The problem is that "other control" in the form may not have been initialized yet and the form will die. You won't find that problem without displaying the form at least once.

And, by the way, if you do have that problem, the simplest answer is to initialize the control in the form's Initialize event, rather than letting the designer generate the code to do it:

VM = New InvoicePremimumVM
rbNew.Checked = True
rbStandardPrint.Checked = True

Guidelines
Based on my experience with this project, my current guidelines are:

• Any code that touches a control belongs in the form.
• The code in my form should be as simple and as short as possible (and no simpler or shorter). Simple logic in the form is better than complicated, error-prone code elsewhere.
• My ViewModel just makes data available; it doesn't specify what the UI does.

These guidelines are, of course, just more specific variations on the Single Responsibility Principle -- but they give me the guidance I need. Will reasonable people disagree with me about these guidelines? Or, even if they agree with these guidelines, disagree with me about specific applications of them? Sure. But now I have a basis for having that discussion (and my clients have a basis for overruling me).

You may question whether it's worthwhile to implement this design pattern in an environment that wasn't built to support it. I can say it was much less work than I originally expected (but then, I am paid by the hour). And, yes, I recognize that many of these issues would go away if I was using Windows Presentation Foundation (WPF); but the client isn't on board with WPF (at least, not yet). Besides, if I used WPF, I wouldn't get to think about these problems. And thinking about problems is the best part of programming.