MVVM - ObservableCollection & Entity Framework DbContext

I'm trying to catch WPF using MVVM pattern, where my set of models is implementing ObservableCollection as well as Entity Framework's DbContext.
My project is quite simple movies database (looking for movies on disc, getting info about these movies from web, and so on...).

I will be glad for any notes and recommendations for what to change / avoid in future.

I am especially afraid about my concept of MovieSet, which is "common implementation" of ObservableCollection & DbContext. Actually it works but I feel that it could be implemented in better way.

• Movie model

My model (Movie) implements commonly presented class ObservableObject and calls OnPropertyChanged(PropertyName) for whatever its property change.

-
MovieSet collection

```
///
/// MoviesSet - ObservableCollection of Movies synchronized with DbSet of Movies
///
public class MoviesSet : ObservableCollection
{
public class MoviesContext : DbContext
{
public DbSet Movies { get; set; }

public MoviesContext() : base("MoviesContext")
{
// allows to recreate database if model changes
Database.SetInitializer
(new DropCreateDatabaseIfModelChanges());

// allows to use properly "AttachDBFilename" from Connection strings
AppDomain.CurrentDomain.SetData("DataDirectory", System.IO.Directory.GetCurrentDirectory());
}
}

private MoviesContext context = new MoviesContext();

///
/// Save context to database
///
public void SaveContext()
{
context.SaveChanges();
}

///
/// Add new movie to context
///
///
public void AddMovie(Movie movie)
{
if ((context.Movies.Where(x => x.OrigName == movie.OrigName)).FirstOrDefault() == null)
{
Add(movie);

if (!string.IsNullOrEmpty(movie.OrigName))
{
context.Movies.Add(movie);
context.Entry(movie).State = Ent

An EF DbContext is an IDisposable object that should be as short-lived as possible.

There are a number of problems with this:

private MoviesContext context = new MoviesContext();

The MoviesSet class owns the MoviesContext instance, and should be responsible for properly disposing it. If the context is going to live as long as the MoviesSet class, then MoviesSet should implement IDisposable, and context.Dispose should be called in MoviesSet.Dispose.

But there's another problem: MoviesSet is essentially an ObservableCollection that knows how to load and save itself using an EF DbContext - it has way too many responsibilities!

This is detrimental on several levels:

• Say the application needs to expand, and you now need an observable collection of TheaterSchedule objects, or whatever; if you follow your current pattern, you'll have some TheaterSchedulesSet that instantiates and owns its very own DbContext - but then there's a relationship between Theater and Movie such as the two entities need to be in the same context: you're stuck.

• Nesting the class makes it even harder to locate where the "data access" concerns are actually addressed, increasing the risk of doubling up existing code somewhere down the maintenance lifetime of the application.

• You can't create an instance of a MoviesSet without also creating an instance of a MoviesContext and thereby connecting to the database: the DbContext is tightly coupled with the observable collection, in such a way that makes it impossible to write a single unit test for your ViewModel that doesn't hit the database: this literally defeats the purpose of MVVM, which would be to decouple classes and separate concerns.

The ViewModel is doing I/O and database work in its constructor; this means if a network or any other error occurs, the runtime will fail to construct the object instance, and who knows what's going to happen then.

A constructor is responsible for constructing an object - assigning private readonly fields for example. Picture:

var foo = new ViewModel();

As a C# developer, what do you expect the above instruction does? As a C# developer, I expect a new ViewModel instance to be created and a reference to that object to be assigned to my foo variable. Nothing more, nothing less.

Now you're telling me that this seemingly harmless little instruction is going to hit the file system and send a query over to some database and populate a collection of entities? Please don't do that!

If you want something to happen when the view is being displayed, then handle the appropriate event on the view, and have the view execute a VM command that populates whatever data needs to be displayed - but don't do that in the constructor!

There's no need for any of the ICommand properties to expose a public setter. What prevents outside code from doing this?

var vm = new ViewModel();
vm.FindMoviesCommand = vm.VisitWebCommand;
vm.DownloadDataCommand = vm.VisitWebCommand;
vm.DeleteCommand = vm.VisitWebCommand;
vm.CopyCommand = vm.VisitWebCommand;
vm.FormatFileNameCommand = vm.VisitWebCommand;
vm.VisitWebCommand = myOwnLittleCommand;

It shouldn't be possible to do this.

The comments to the right of each command property are all utterly useless: remove them. If a command's name isn't descriptive enough, rename it. If that's still not enough, use XML comments with a ` tag so that the descriptions are actually helpful (they'll show up in Visual Studio's IntelliSense), not just noise.

Speaking of comments:

// application configuration
private NameValueCollection appCfg = ConfigurationManager.GetSection("appCfg") as NameValueCollection;

How about this instead:

private NameValueCollection settings = ConfigurationManager.GetSection("appCfg") as NameValueCollection;

Comments that exist only to explain the purpose of a poorly named variable should be removed, they're not solving the problem, only the symptom. Use meaningful names, avoid disemvoweling. Here the variable is an abstraction over some appCfg section of XML configuration: at that level you don't care what the actual config section name is.

All in all, you've got MVVM down: the View knows about its ViewModel, the ViewModel knows about the Model, and more importantly the ViewModel knows nothing of any View, and the Model knows nothing of any View or ViewModel.

But you urgently need to untangle the data access from the model (counting MoviesSet as part of the model).

Careful with naming, too: MoviesContext suggests that there's potentially a context-per-entity, which is really a terrible idea. Also MoviesSet suggests that the class is inherited from some Set, when it's in fact derived from ObservableCollection: ObservableMovieCollection would be a much better name IMO.

The XAML Button tags don't need a Name. XAML elements very seldom need a Name. And when they do, it's an x:Name` that they need. Als

StackExchange Code Review Q#144541, answer score: 8