6
\$\begingroup\$

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.

  1. Movie model

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

  1. MovieSet collection

    /// <summary>
    /// MoviesSet - ObservableCollection of Movies synchronized with DbSet of Movies
    /// </summary>
    public class MoviesSet : ObservableCollection<Movie>
    {
        public class MoviesContext : DbContext
        {
            public DbSet<Movie> Movies { get; set; }
    
            public MoviesContext() : base("MoviesContext")
            {
                // allows to recreate database if model changes
                Database.SetInitializer<MoviesContext>
                    (new DropCreateDatabaseIfModelChanges<MoviesContext>());
    
                // allows to use properly "AttachDBFilename" from Connection strings
                AppDomain.CurrentDomain.SetData("DataDirectory", System.IO.Directory.GetCurrentDirectory());
            }
        }
    
        private MoviesContext context = new MoviesContext();
    
        /// <summary>
        /// Save context to database
        /// </summary>
        public void SaveContext()
        {
            context.SaveChanges();
        }
    
        /// <summary>
        /// Add new movie to context
        /// </summary>
        /// <param name="movie"></param>
        public void AddMovie(Movie movie)
        {
            if ((context.Movies.Where(x => x.OrigName == movie.OrigName)).FirstOrDefault<Movie>() == null)
            {
                Add(movie);
    
                if (!string.IsNullOrEmpty(movie.OrigName))
                {
                    context.Movies.Add(movie);
                    context.Entry(movie).State = EntityState.Added;
                }
            }
        }
    
        /// <summary>
        /// Notify modification of Movie object
        /// </summary>
        /// <param name="movie"></param>
        public void NotifyMovieModified(Movie movie)
        {
            if ((context.Movies.Where(x => x.OrigName == movie.OrigName)).FirstOrDefault<Movie>() != null)
            {
                context.Entry(movie).State = EntityState.Modified;
            }
            else
            {
                context.Entry(movie).State = EntityState.Added;
            }
        }
    
        /// <summary>
        /// Delete movie from context
        /// </summary>
        /// <param name="movie"></param>
        public void DeleteMovie(Movie movie)
        {
            Remove(movie);
            context.Entry(movie).State = EntityState.Deleted;
            context.Movies.Remove(movie);
        }
    
        /// <summary>
        /// Load context from database
        /// </summary>
        public void LoadContext()
        {
            context.Movies.Load();
    
            foreach (Movie movie in context.Movies)
            {
                Add(movie);
            }
        }
    }
    
  2. ViewModel

    /// <summary>
    /// Movie model
    /// </summary>
    public class ViewModel : Mvvm.ObservableObject
    {
        MovieFileProcessor fileProc;
    
        public ICommand FindMoviesCommand { get; set; }         // find all movie files cmd
        public ICommand DownloadDataCommand { get; set; }       // download all unknown movies data cmd
        public ICommand PlayVideoCommand { get; set; }          // play selected movie cmd
        public ICommand VisitWebCommand { get; set; }           // visit selected movie web page cmd
        public ICommand DeleteCommand { get; set; }             // delete selected movie cmd
        public ICommand CopyCommand { get; set; }               // copy selected movie cmd
        public ICommand FormatFileNameCommand { get; set; }     // format selected movie files cmd
    
        // observable and serializable set of Movies
        public MoviesSet Movies { get; set; }
    
        // actually selected movie obj 
        private object selectedMovie;
        public object SelectedMovie
        {
            get
            {
                return selectedMovie;
            }
            set
            {
                if (selectedMovie != value)
                {
                    selectedMovie = value;
                    OnPropertyChanged("SelectedMovie");
                }
            }
        }
    
        // application configuration
        private NameValueCollection appCfg = ConfigurationManager.GetSection("appCfg") as NameValueCollection;
    
        public ViewModel()
        {
            FindMoviesCommand = new Mvvm.RelayCommand(FindMovieFiles);
            DownloadDataCommand = new Mvvm.RelayCommand(DownloadMoviesData);
            PlayVideoCommand = new Mvvm.RelayCommand(PlayVideo);
            VisitWebCommand = new Mvvm.RelayCommand(VisitWeb);
            DeleteCommand = new Mvvm.RelayCommand(DeleteMovie);
            CopyCommand = new Mvvm.RelayCommand(CopyMovie);
            FormatFileNameCommand = new Mvvm.RelayCommand(FormatFileName);
    
            fileProc = new MovieFileProcessor(appCfg["MoviesDir"]);
    
            Movies = new MoviesSet();
    
            // load Movies context from database
            Movies.LoadContext();
        }
    
        /// <summary>
        /// Delete selected movie
        /// </summary>
        /// <param name="obj">SelectedMovie</param>
        private void DeleteMovie(object obj)
        {
            try
            {
                fileProc.Delete(obj as Movie);
            }
            catch { }
    
            Movies.DeleteMovie(obj as Movie);
            Movies.SaveContext();
        }
    
        /// <summary>
        /// Copy movie to another directory
        /// </summary>
        /// <param name="obj">SelectedMovie</param>
        private void CopyMovie(object obj)
        {
            throw new NotImplementedException();
        }
    
        /// <summary>
        /// Rename selected movie video file (& subtitles file, & subdirectory)
        /// </summary>
        /// <param name="obj">SelectedMovie</param>
        private void FormatFileName(object obj)
        {
            fileProc.Rename(obj as Movie);
            Movies.NotifyMovieModified(obj as Movie);
            Movies.SaveContext();
        }
    
        /// <summary>
        /// Visit web page of selected movie
        /// </summary>
        /// <param name="obj">SelectedMovie</param>
        private void VisitWeb(object obj)
        {
            System.Diagnostics.Process.Start((obj as Movie).WebPage);
        }
    
        /// <summary>
        /// Play selected movie
        /// </summary>
        /// <param name="obj">SelectedMovie</param>
        private void PlayVideo(object obj)
        {
            Process.Start(appCfg["VideoPlayerPath"], (obj as Movie).VideoFile.ToString() + " -f");
        }
    
        /// <summary>
        /// Find movie files in movies directory
        /// </summary>
        /// <param name="obj">Not used</param>
        public void FindMovieFiles(object obj)
        {
            foreach (Movie movie in fileProc.FindMovies())
            {
                Movies.AddMovie(movie);
            }
    
            // Movies context not saved because OrigName (Primary Key) is empty at this moment
        }
    
        /// <summary>
        /// Download data from repository
        /// </summary>
        /// <param name="obj">Not used</param>
        public void DownloadMoviesData(object obj)
        {
            CsfdDataMiner miner = new CsfdDataMiner();
    
            new Task(() =>
            {
                foreach (Movie movie in Movies.Where(x => string.IsNullOrEmpty(x.OrigName)))
                {
                    movie.WebPage = miner.GetMoviePage(movie.VideoFile.Name);
                    miner.GetMovieData(movie);
                    Movies.NotifyMovieModified(movie);
                }
                Movies.SaveContext();
            }).Start();
        }
    }
    
  3. View - for completness

    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="*"></RowDefinition>
            <RowDefinition Height="Auto"></RowDefinition>
        </Grid.RowDefinitions>
        <DataGrid Grid.Row="0" x:Name="gridMovies" AutoGenerateColumns="False" Padding="10" CanUserAddRows="False" SelectionUnit="FullRow" SelectionMode="Single"
                  PreviewKeyDown="gridMovies_PreviewKeyDown" CanUserDeleteRows="False" IsReadOnly="True" 
                  ItemsSource="{Binding Movies}" SelectedItem="{Binding SelectedMovie}">
            <DataGrid.CellStyle>
                <Style TargetType="DataGridCell">
                    <!--Disable highlight of current selected cell-->
                    <Setter Property="BorderThickness" Value="0"/>
                </Style>
            </DataGrid.CellStyle>
            <DataGrid.Columns>
                <DataGridTextColumn Binding="{Binding VideoFile}" Header="VideoFile" />
                <DataGridTextColumn Binding="{Binding HasSubtitles}" Header="Subtitles" />
                <DataGridTextColumn Binding="{Binding OrigName}" Header="Name" />
                <DataGridTextColumn Binding="{Binding LocalName}" Header="Local Name" />
                <DataGridTextColumn Binding="{Binding Year}" Header="Year" />
                <DataGridTextColumn Binding="{Binding CountriesStrings}" Header="Countries" />
                <DataGridTextColumn Binding="{Binding GenresStrings}" Header="Genres" />
                <DataGridTextColumn Binding="{Binding Rating}" Header="Rating" />
            </DataGrid.Columns>
            <DataGrid.ContextMenu>
                <ContextMenu IsEnabled="{Binding Movies.Count}">
                    <MenuItem Command="{Binding PlayVideoCommand}" CommandParameter="{Binding SelectedMovie}" Header="Play Video"/>
                    <MenuItem Command="{Binding VisitWebCommand}" CommandParameter="{Binding SelectedMovie}" Header="Visit web"/>
                    <MenuItem Command="{Binding DeleteCommand}" CommandParameter="{Binding SelectedMovie}" Header="Delete movie"/>
                    <MenuItem Command="{Binding FormatFileNameCommand}" CommandParameter="{Binding SelectedMovie}" Header="Format file names"/>
                    <MenuItem Command="{Binding CopyCommand}" CommandParameter="{Binding SelectedMovie}" Header="Copy movie"/>
                </ContextMenu>
            </DataGrid.ContextMenu>
        </DataGrid>
        <Grid Grid.Row="2" >
            <Grid.ColumnDefinitions>
                <ColumnDefinition></ColumnDefinition>
                <ColumnDefinition></ColumnDefinition>
                <ColumnDefinition></ColumnDefinition>
            </Grid.ColumnDefinitions>
            <Button Grid.Column="1" Name="btnFindMovies" Content="Find Movies" Command="{Binding FindMoviesCommand}"></Button>
            <Button Grid.Column="2" Name="btnDwnldData" Content="Download Data" Command="{Binding DownloadDataCommand}"></Button>
        </Grid>
    </Grid>
\$\endgroup\$
6
\$\begingroup\$

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 <summary> 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<Movie>, 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. Also the tags should be self-closing:

<Button Grid.Column="1" Content="Find Movies" Command="{Binding FindMoviesCommand}" />
<Button Grid.Column="2" Content="Download Data" Command="{Binding DownloadDataCommand}" />

Same for the RowDefinition tags, should be self-closing.

This looks suspicious:

<ContextMenu IsEnabled="{Binding Movies.Count}">

If you used a command binding here, then the command's CanExecute implementation would automatically handle the menu's IsEnabled property. But here, you're binding a bool property to an int value, without a converter: I don't think this binding works at all.

This isn't needed:

CommandParameter="{Binding SelectedMovie}"

The SelectedMovie is already known to the ViewModel, there's no need to pass it around as a command parameter.

Speaking of the SelectedMovie:

private object selectedMovie;
public object SelectedMovie
{
    get
    {
        return selectedMovie;
    }
    set
    {
        if (selectedMovie != value)
        {
            selectedMovie = value;
            OnPropertyChanged("SelectedMovie");
        }
    }
}

I think you're making your life more complicated than it needs to be, by making it an object; it should be a Movie.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for very detailed answer. I will try to refactor as much as possible following your reasonable suggestions. I just have some questions: 1. How my ViewModel knows about View and how my Model knows about my ViewModel? 2. I feel that whole MovieSet smells. But can you suggest my some other pattern, to be able to use it for both for observable binding and together with Entity framework? \$\endgroup\$ – Majak Oct 18 '16 at 18:03
  • \$\begingroup\$ The VM shouldn't know anything about a view, ever; it's the other way around (the view binds on VM properties, so it needs to know about the VM; the VM must exist without a view). As for the MovieSet functionality I'd suggest extracting the data access stuff out of there and perhaps add a DbContext (or some IUnitOfWork wrapper interface) constructor parameter to the VM, so your command implementations can work with it. \$\endgroup\$ – Mathieu Guindon Oct 18 '16 at 18:12
  • \$\begingroup\$ Ok I follow you with decoupling MovieSet from DbContext. So I have last question, how can I in the cleanest possible way remove binding of view to viewmodel's props? Let's say this one: <MenuItem Command="{Binding PlayVideoCommand}" CommandParameter="{Binding SelectedMovie}" Header="Play Video"/> \$\endgroup\$ – Majak Oct 18 '16 at 18:36
  • \$\begingroup\$ You don't - that's exactly how it's done. The View binds to ViewModel properties, that's MVVM. \$\endgroup\$ – Mathieu Guindon Oct 18 '16 at 18:37
  • \$\begingroup\$ Oh ok I was little confused, this makes sense. So how do then I violate view -> VM or VM -> model relation rules in my case? \$\endgroup\$ – Majak Oct 18 '16 at 18:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.