UnitOfWork, removing abstraction of abstraction (Repository)

Question

I'm creating a small project, but in the past I've used a Repository pattern for it, and it just seemed too bloated. I ended up having repo classes with huge amounts of queries. I'm trying to move away from that. I invite you to critique/help/guide me on my journey to less abstraction:

Note: I like the syntax style of Simple.Data and "Fluentness" of FluentApi. So my naming/style is inspired by them.

I start off with an IUnitOfWork Interface:

public interface IUnitOfWork : IDisposable
{
    void Save();
    IDbSet<T> Set<T>() where T : class;
}

My DB Context (pretty standard):

public partial class MyDbContext : DbContext, IUnitOfWork
{
    public MyDbContext() : base("DBConnection") { }

    public new IDbSet<T> Set<T>() where T : class
    {
        return base.Set<T>();
    }
    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.Configurations.Add(new UsersMap());
    }
    public void Save()
    {
        SaveChanges();
    }
}

A Query object/service:

public class UserService : IService
{
    private readonly IUnitOfWork unitOfWork;

    public UserService(IUnitOfWork _u)
    {
        this.unitOfWork = _u;
    }
    public string GetUserName(string Name)
    {
        return unitOfWork.Set<User>().Single(s => s.Name == Name).UserName;
    }

}

Here's my Database Class (name inspired by Simple.Data):

public sealed partial class Database
{
    private readonly IUnitOfWork context;

    public Database(IUnitOfWork _context = null)
    {
        if (_context == null)
            this.context = new MyDbContext();
        else
            this.context = _context;
    }

    public static Database Open()
    {
        return new Database();
    }

    // I may move these into a separate file:
    public UserService Users { get { return new UserService(context); } }
}

Usage:

var userName = Database.Open().Users.GetUserName("John");
Console.WriteLine(userName);

This is all fine and good for simple tasks. But let's say I want to do multiple things whilst the Database connection is open. I could move Database.Open() to a separate variable then use it like this:

var database = Database.Open();
var userName = database.Users.GetUserName("John");
var userFirstName = database.Users.GetFirstName("john123");

So my question is, am I doing it wrong? Obviously, my main concern is to not open multiple connections to the Database, and use it like a UnitOfWork.

My other question is about how to handle CRUD. I have IService there (currently empty) because I was thinking of putting Insert/Update/Delete in there and adding methods to each Service class - BUT, this turns into a type "Repository" which I was trying to avoid. I don't think it's right for me to expose the DbSet<TEntity>() so I can .Add()/.Attach() - so what should I do here?

I'm still at the planning stage of this, so any changes/recommendations are welcome.

Answer 1

I realize you said

my naming/style is inspired by them.

But I would still point out some of your naming conventions which don't align with C#s.

• Method/Function level variables should be camelCase instead of PascalCase

  public string GetUserName(string Name)
  public string GetUserName(string name)
  

• Same as before, and as a note _underscores should only be used for class level variables.

  public UserService(IUnitOfWork _u)
  public UserService(IUnitOfWork u)
  

• Not required, but a common practice is also that you do indeed use underscores for class level variables

  private readonly IUnitOfWork unitOfWork;
  private readonly IUnitOfWork _unitOfWork;
  

---

Make sure you are disposing(closing) your database object. I presume by the way you have your class setup that when your database object is disposed it will close, in which case I would recommend using the using keyword. Which would make your above code look more like this in practice.

using(var database = Database.Open())
{
    var userName = database.Users.GetUserName("John");
    var userFirstName = database.Users.GetFirstName("john123");
}

This ensures that you are always closing your connection in the end.

---

Be careful when debugging your code, and remember (depending on your db setup) if you pause the debug in your code while the connection is open, you wont be able to use the database. Also if you have any plans to do mulithreading with this you'll want to use locks.

Answer 2

You're using Entity Framework, which is a unit-of-work & repository pattern implementation: getting rid of your repositories / abstractions of abstractions was the right thing to do.

I'm not sure what the role of a Database is though:

Obviously, my main concern is to not open multiple connections to the Database, and use it like a UnitOfWork.

Your IUnitOfWork interface already exposes its DbSet<TEntity>'s via the Set<TEntity>() generic method, so a unit of work's client code can access users like this:

using (var unitOfWork = new UnitOfWork("connection string"))
{
    var users = unitOfWork.Set<User>();
}

The Set<TEntity>() method signature on the IUnitOfWork interface already matches that of DbContext.Set<TEntity>(), there's no need for member shadowing here, this code is perfectly redundant:

public new IDbSet<T> Set<T>() where T : class
{
    return base.Set<T>();
}

It's good that you're using the new keyword to show that the member shadowing is intentional though.

By exposing that method, you give your clients the ability to work with an IQueryable<TEntity> - which is good... if your clients are services; and this is what UserService seems to be doing:

public string GetUserName(string Name)
{
    return unitOfWork.Set<User>().Single(s => s.Name == Name).Name;
}

I just don't get the usefulness of a method that takes a parameter and hits the database... only to return that very same value.

This would be more useful:

public User GetById(int id)
{
    return unitOfWork.Set<User>().Single(s => s.Id == id);
}

The problem with exposing UserService on Database, is that you're going to eventually have a Database exposing a service for every entity out there, including entities the client code doesn't care about. The client code should be able to depend on the service itself, and not care about the rest of the details.

By depending on a service, the client code gets much more focused, and you can tell what the code does (can do?) just by looking at its constructor. Clearly a class that depends on IUserService is going to be working with User entities. A class that depends on Database could do just about anything...

Another problem with this:

public UserService Users { get { return new UserService(context); } }

Is that it makes a hidden dependency. The constructor only says "I need a IUnitOfWork", but in reality the class also has a dependency (/coupling) with UserService, and everything else you're newing up here. The net result is higher coupling and lower cohesion, and that can't be good.

I'd get rid of the Database class.