MVVM design feels too unwieldy, am I doing it wrong?

Question

I've recently started creating a WPF application, and am just hoping that someone can confirm to me that I'm building my overall system architecture properly, or correct me if I'm heading off in the wrong direction somewhere. Especially since I'm trying to do MVVM, there are a lot of layers involved, and I'm not sure I'm doing things properly.

Here's a simplified description of the system:

Data is stored in an SQL Server database, which is accessed through Linq to SQL. Let's say that the database contains two tables, USERS and USER_GROUPS. Each table has an auto-generated Linq to SQL class, DB_USER and DB_USER_GROUP.

Now in the application, I want to display a ListBox with each ListBoxItem containing various UI elements for displaying/modifying the users' info, which is done using a DataTemplate.

I have a view model class for the window, which uses a Linq to SQL query (joining the two tables) to populate an ObservableCollection<User> named UserList, which the ListBox in the window has bound as its ItemsSource. User is a class implementing INotifyPropertyChanged that handles all the formatting/getting/setting of database data into what's needed by the WPF controls. The section of code handling this is something like:

DBDataContext db = new DBDataContext();

var allUsers = from user in db.USERs
                   .Where(u => u.ENABLED == true)
               from group in db.USER_GROUPs
                   .Where(g => g.GROUPID == u.GROUPID)
                   .DefaultIfEmpty()
               select new { user, group };

foreach (var user in allUsers)
{
    User u = new User(db, user.user, user.group);
    UserList.Add(u);
}

So the User class is constructed with private properties for a DB_USER, a DB_USER_GROUP, and the database DataContext class. All of a User's public properties basically wrap the relevant columns, with their get methods returning the values for WPF to use, and set changing the column(s) and then calling SubmitChanges() on the private DataContext property to update the database.

This is all working fine, but it feels a little unwieldy, so I'm just wondering if I've missed something that would make it cleaner. Specifically, storing a DataContext inside each element of UserList seems odd, but I wasn't sure of a better method to be able to update the database whenever data was changed in the UI.

Any feedback is appreciated, and please let me know if anything's unclear, I'm not sure how well I've explained it.

Answer 1

Starting off, let's put some labels on what you are doing here: DB_USER is your Model and User is your ViewModel (I 'd have preferred UserViewModel for the latter just so that it's more clear what's going on).

One thing that's immediately obvious is that it's not really proper for your ViewModel to have functionality suited to your Model, i.e. that DataContext does not belong where it currently is. This is a piece of information that should either be in your Model, or alternatively encapsulated in some DataStore/DataService (take your pick) class. Your ViewModel would then be responsible, when the time comes to save any changes, to tell the DataStore "here's an updated snapshot of this model, please save it for me" (this would most likely be exposed to the UI through an ICommand). This feels cleaner and underscores the idea that your ViewModel is a layer that adapts the realities of your model to your choice of UI.

Other than the above, there's nothing in what you describe that I feel needs to be "corrected". However, I can offer some suggestions regarding things that you have not elaborated on.

Exposing data from a Model through a ViewModel is always something that can be implemented in many ways. When considering what approach to take, you should take into account the possibility of the same Model being exposed through different Views at the same time. In this case, IMHO the preferred approach is to have a separate ViewModel for each View (the Views may well be of different types, so they could have different expectations from the ViewModel adapter thus pointing to multiple types of ViewModels as well), so you would need to use a pattern that allows changes to be communicated from one ViewModel to any others in "real time".

One way to do this would be to make your Models implement INotifyPropertyChanged themselves and have each ViewModel hook into its Model for notifications, so when a change occurs ViewModel A pushes the change to the Model, and the Model notifies ViewModel B.

However personally I don't like polluting my Models with what is in essence code that only caters to the needs of the UI, so another approach is needed. That would be making the DataService I mentioned above expose functionality (methods and events) through which ViewModel A can tell the service "hey, the Model I 'm wrapping has had some changes" ; note that this is different from "I want you to persist the current snapshot of this Model". ViewModel B has already hooked into a suitable "ModelChanged" event, so it gets notified and pulls the updated information from the service. This has the added benefit that if at any time the service detects that the backing data repository has been updated by a source external to the current process, there's a ready made mechanism to broadcast a "Calling all ViewModels: Model X has been updated, any interested parties please talk to me about learning the details" message.

Above all, always keep in mind that there is no "one true MVVM style" and there are myriads of possible approaches. Which one to take depends not only on hard facts and the current position of the slider on the YAGNI/HyperEngineering scale, but also on, dare I say, your taste.