4
\$\begingroup\$

This code review request is tightly coupled with this SO question, this is the solution I implemented to solve the problem being asked about there.

All my ViewModels get constructor-injected with a Model that itself gets constructor-injected with a DbContext-derived class, and that works well in all cases, except when the View has a command to allow the user to discard pending changes, in which case the DbContext should be disposed and then reinstantiated.

Obviously since the DbContext is created by an IoC container it would be a very bad idea to just dispose and reinstantiate the context, so I came up with a solution involving a factory.

public interface IContextFactory<out TContext> where TContext : DbContext
{
    TContext Create();
}

public interface IDiscardModelChanges
{
    void DiscardChanges();
}

Now IDiscardModelChanges is implemented by the model, so to facilitate this I've created a base class that will ensure I have a context factory at hand:

public abstract class DiscardableModelBase<TContext> : IDiscardModelChanges, IDisposable
    where TContext : DbContext
{
    private readonly IContextFactory<TContext> _factory;

    protected TContext Context { get; private set; }

    protected DiscardableModelBase(IContextFactory<TContext> factory)
    {
        _factory = factory;
        Context = _factory.Create();
    }

    public virtual void DiscardChanges()
    {
        Context.Dispose();
        Context = _factory.Create();
    }

    public void Dispose()
    {
        Context.Dispose();
    }
}

The model that needs to control its DbContext then derives from this class:

public class SomeModel : DiscardableModelBase<SomeContext>, ISomeModel
{
    public SomeModel(IContextFactory<SomeContext> factory) 
        : base(factory) 
    { }

    /* methods that act upon protected Context property */
}

And then the ViewModel that needs to discard pending changes can do it like this (given a private readonly ISomeModel _model):

var model = _model as IDiscardModelChanges;
if (model != null) model.DiscardChanges();

As far as binding conventions are concerned, I'm keeping the existing "Context" convention, and adding a "ContextFactory" one:

_kernel.Bind(t => t.From(_dataLayerAssembly)
                   .SelectAllClasses()
                   .Where(type => type.Name.EndsWith("ContextFactory"))
                   .BindSingleInterface());

Bottom line, as far as ViewModels are concerned, nothing changes; the Model is still injected as an interface that exposes the available model methods, and the IDiscardModelChanges interface is only needed if the ViewModel needs to use it, and the ViewModel can't assume the interface is implemented by the Model.

Any known issues with this approach, any blatant mistake made?

\$\endgroup\$
  • \$\begingroup\$ AFAICT you don't need to throw away the context? code.msdn.microsoft.com/How-to-undo-the-changes-in-00aed3c4 \$\endgroup\$ – James Manning Oct 3 '13 at 7:46
  • \$\begingroup\$ Ugh isn't that painful? Loop through entities, switch upon state, reset state, ...I was referring to this post; disposing and recreating the context sounds like a much cleaner way to go, especially if I don't want to cherry-pick entity types I want to cancel changes for (i.e. when I want to drop all changes regardless). \$\endgroup\$ – Mathieu Guindon Oct 3 '13 at 14:33
  • \$\begingroup\$ Throwing away context might be even more painful. Saving all changes will have more impact to your performance than discarding them one by one. So why bother? \$\endgroup\$ – Frederik P. Jul 16 '14 at 7:57
2
\$\begingroup\$

Somewhat similar but still a bit different approach is to encapsulate your context into some container class and put this wrapper into IoC container instead of context itself. Might look like this:

interface IContextManager<TContext> : IDisposable
{
     TContext Context { get; }

     void ReloadContext();
     event Action ContextChanged;   
}

The obvious advantage (which might as well be a disadvatage, depending on context) is that this way you will always have a single instace of DbContext, and once it is reloaded - every model will use new instance (given they access it via IContextManager.Context property).

Also IDiscardModelChanges is a bad name for an interface. Interface name should not be a verb. It should either name a property of an object (e.g. IDiscardable), or name object itself (e.g. IDiscardableModel).

\$\endgroup\$
  • \$\begingroup\$ +1 for pointing out the "I discard model changes" bad naming, but I'm not sure get how such an interface would be used, care to add more code? Thanks for your input! \$\endgroup\$ – Mathieu Guindon Oct 3 '13 at 14:48
  • 1
    \$\begingroup\$ @retailcoder, the idea is to move the logic from DiscardableModelBase<TContext> to IContextManager<TContext>. This way all of your models will consume IContextManager<TContext> as their parameter. Essentially it is replacing inheritance with aggregation (by adding another abstraction which sole purpose is "managing" DbContext). \$\endgroup\$ – Nikita B Oct 4 '13 at 5:55
  • \$\begingroup\$ I see.. and I like what I'm seeing! :) \$\endgroup\$ – Mathieu Guindon Oct 4 '13 at 10:43
5
\$\begingroup\$

As stated by previous comments, there is no need to throw away the context. But since this is a code review, and not a concept review I'll leave that part aside.

All your TContext generics are to be inheriting from DbContext, but nowhere in any class do you use any of DbContext's properties or methods. You are only using the .Dispose method, which comes from the IDisposable interface. I've refactored your code to have this result:

public interface IContextFactory<out TContext> where TContext : IDisposable
{
    TContext Create();
}

public interface IDiscardModelChanges
{
    void DiscardChanges();
}

public abstract class DiscardableModelBase<TContext> : IDiscardModelChanges, IDisposable
where TContext : IDisposable
{
    private readonly IContextFactory<TContext> _factory;

    protected TContext Context { get; private set; }

    protected DiscardableModelBase(IContextFactory<TContext> factory)
    {
        _factory = factory;
        Context = _factory.Create();
    }

    public virtual void DiscardChanges()
    {
        Context.Dispose();
        Context = _factory.Create();
    }

    public void Dispose()
    {
        Context.Dispose();
    }
}

The changes might seem small, but the impact is big. In your example, you are going to pull in Entity Framework everywhere you are using this application, which isn't needed at all. You are not doing anything entity framework related, you are doing something with disposable objects. This could be a StreamWriter, etc...

PS: I know this post is old. But people should really be aware of the issue with this code.

\$\endgroup\$
  • \$\begingroup\$ Nice answer, it's never too late to review code on CR :) \$\endgroup\$ – Mathieu Guindon Jul 18 '14 at 16:01

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.