5
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your input (and also for commenting mainly on the pattern rather than the basic stuff - im new to CodeReview, so i probably should have specified this is my question). The purpose of the Database class (in my mind) was essentially a UnitOfWork class, that works like FluentApi, ie, has a readable method calls. It's there to avoid exposing the DbSet<T> and MyDbContext directly. This library will be used by my website, and I figured it'd be easier to manage having the queries in query objects, rather than in MVC project. \$\endgroup\$ – jzm May 22 '14 at 23:57
  • \$\begingroup\$ You say "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" but is this really a bad thing? Client code would point to Database.Open().Whatever; but that can then point to a service which holds queries for that particular Entity, rather than the web app containing queries. I suppose this still is an abstraction... \$\endgroup\$ – jzm May 23 '14 at 0:04
  • \$\begingroup\$ It is a bad thing, it gives you a god object that knows about everything there is to know about data. That's the role of a whole DAL assembly! Put your interfaces and abstract classes under some MyApp.Abstract namespace. The DAL's client doesn't even need to know there's a database involved, the data might as well come from avian carriers, it couldn't care less - it doesn't even care that the DAL is implementing the very same service interfaces it's using. In fact, it doesn't even know there's a DAL. Inversion of Control! \$\endgroup\$ – Mathieu Guindon May 23 '14 at 0:30
  • \$\begingroup\$ Fair enough. How would you suggest I change my code? I'm not sure I feel comfortable just exposing IQueryable<T>. \$\endgroup\$ – jzm May 23 '14 at 1:09
  • \$\begingroup\$ You don't expose it, your DAL implements interfaces from the BLL assembly, the services - the business logic uses services, these things don't expose IQueryable<T>, they give you a UserModel to consume directly. The BLL doesn't reference the DAL, it's the other way around. \$\endgroup\$ – Mathieu Guindon May 23 '14 at 1:34

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.