3
\$\begingroup\$

I have an MVVM WPF application that will make use of a repository and unit-of-work.

This is my Entity Framework database-first model:

public partial class tUser
{    
    public string ID { get; set; }
    public string Name { get; set; }
    public string Surname { get; set; }
    public string Password { get; set; }
}

My implementation of the UsersRepository - noticed I didn't use a Generic Repository pattern:

public class UsersRepository : IUserRepository
{
    private DbEntities db;

    public UsersRepository (DbEntities db)
    {
        this.db = db;
    }

    public tUser GetById(object userId)
    {
        return db.tUser.Find(userId);
    }

    public IEnumerable<tUser> GetAll()
    {
        return db.tUser.ToList();
    }

    public virtual IEnumerable<tUser> GetSome(
        Expression<Func<tUser, bool>> filter = null,
        Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null,
        string includeProperties = "")
    {
        IQueryable<tUser> query = db.Set<tUser>();

        if (filter != null)
        {
            query = query.Where(filter);
        }

        foreach (var includeProperty in includeProperties.Split
            (new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries))
        {
            query = query.Include(includeProperty);
        }

        if (orderBy != null)
        {
            return orderBy(query).ToList();
        }
        else
        {
            return query.ToList();
        }
    }

    public void InsertUser(tUser user)
    {
        db.tUser.Add(user);
    }

    public void DeleteUser(object userId)
    {
        var user = db.tUser.Find(userId);
        db.tUser.Remove(user);
    }

    public void UpdateUser(tUser user)
    {
        db.Entry(user).State = EntityState.Modified;
    }

    public void Save()
    {
        db.SaveChanges();
    }

    #region dispose
    private bool disposed = false;

    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                db.Dispose();
            }
        }
        this.disposed = true;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    #endregion
}

The Unit-Of-Work implementation:

public class UnitOfWork : IDisposable
{
    private DbEntities db = new DbEntities();

    private IUserRepository userRepository;
    public IUserRepository UserRepository
    {
        get
        {
            return userRepository ?? new UsersRepository(db);
        }
    }

    public void Save()
    {
        db.SaveChanges();
    }

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

The UsersManager window ViewModel:

public class UsersManagerViewModel : ObservableObject
{
    private UnitOfWork unit = new UnitOfWork();

    private string _searchText;
    public string SearchText
    {
        get { return _searchText; }
        set
        {
            if (_searchText != value)
            {
                _searchText = value;
                SelectedUser = null;
                RaisePropertyChanged();
            }
        }
    }

    private UserViewModel _selectedUser;
    public UserViewModel SelectedUser
    {
        get { return _selectedUser; }
        set
        {
            if (_selectedUser != value)
            {
                _selectedUser = value;
                RaisePropertyChanged();
            }
        }
    }
}

UserViewModel:

public class UserViewModel : ObservableObject
{
    private readonly tUser entity;

    public UserViewModel()
        : this(new tUser())
    { 
    }

    public UserViewModel(tUser _entity)
    {
        this.entity = _entity;
    }

    public tUser Model { get { return this.entity; } }

    public string UserName
    {
        get { return entity.Name; }
        set
        {
            entity.Name = value;
            RaisePropertyChanged();
            RaisePropertyChanged("UserFullName");
        }
    }

    public string SurName
    {
        get { return entity.Surname; }
        set
        {
            entity.Surname = value;
            RaisePropertyChanged();
            RaisePropertyChanged("UserFullName");
        }
    }

    public string UserFullName
    {
        get { return entity.Name + " " + entity.Surname; }
        set { RaisePropertyChanged(); }
    }
}

Are my Implementations "correct" as far as respecting the design patterns go? Should I remove the private DbEntities db; field from my repository and wrap each call with a using (var db = new DbEntities()) instead?

\$\endgroup\$
  • \$\begingroup\$ In practical terms, there's no difference. The connection is pooled by the framework, and it's matched by connection string. So long as that is the same, you'll be re-using the same connection. The framework will also open and close the connection automatically. \$\endgroup\$ – Anthony Fawcett Nov 27 '15 at 3:42
1
\$\begingroup\$

UsersRepository

private DbEntities db;

public UsersRepository (DbEntities db)
{
    this.db = db;
}  

You should define DbEntities db as readonly to prevent it beeing changed by accident.


The public virtual IEnumerable<tUser> GetSome( Expression<Func<tUser, bool>> filter = null, Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null, string includeProperties = "") method should have some parameter validation at least for the includeProperties parameter. This parameter should be checked for null to throw an early exception.


This

public void DeleteUser(object userId)
{
    var user = db.tUser.Find(userId);
    db.tUser.Remove(user);
}  

can be simplified by using the GetById() method like so

public void DeleteUser(object userId)
{
    var user = GetById(userId);
    db.tUser.Remove(user);
}  

You are using this inconsistently. You are reffering to this only with the disposed variable but never with the db.


#region dispose  

Please read Are regions an antipattern or code smell


UnitOfWork

private IUserRepository userRepository;
public IUserRepository UserRepository
{
    get
    {
        return userRepository ?? new UsersRepository(db);
    }
}  

In this way userRepository will always be null. Either return each time a new UsersRepository(db); or change the getter to

private IUserRepository userRepository;
public IUserRepository UserRepository
{
    get
    {
        return userRepository ?? (userRepository = new UsersRepository(db));
    }
}   

What happens if Dispose() is called twice ? You get a NullReferenceException. So either implement it like you did in the UsersRepository or add a simple null check for db.

\$\endgroup\$

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.