2
\$\begingroup\$

Is this repository code written according to best practices? The Last Section I included it in the repository as well.

class HR_Repository<T> : IRepository<T> where T : class
{
    private readonly  LoginDataContext dataContext;
    public HR_Repository(LoginDataContext dataContext)
    {
    this.dataContext = dataContext;
    }


    public void Commit()
    {
        dataContext.SubmitChanges();
    }

    public IList<T> FindAll()
    {
        var table = this.LookupTableFor(typeof(T));
        return table.Cast<T>().ToList();
    }

    public IQueryable<T> Find()
    {
        var table = this.LookupTableFor(typeof(T));
        return table.Cast<T>();
    }

    public void Add(T item)
    {
        var table = this.LookupTableFor(typeof(T));
        table.InsertOnSubmit(item);
    }   

    public void Delete(T item)
    {
        var table = this.LookupTableFor(typeof(T));
        table.DeleteOnSubmit(item);
    }

    private ITable LookupTableFor(Type entityType)
    {
        return dataContext.GetTable(entityType);
    }
}


public static class UserQueries
{
    public static Employee ByUserName(this IQueryable<Employee> employees, string username)
    {
        return employees.Where(u => u.User_Name == username).FirstOrDefault();
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I don't feel like compiling this, but I assume that it must not work? That seems to be why this was closed, since its clearly written. I rather wish someone who cared enough to close this would explain what the problem is. \$\endgroup\$
    – Magus
    Commented Apr 18, 2014 at 19:12
  • \$\begingroup\$ Agreed. This at least deserves an explanation. At a glance, I don't see why this wouldn't compile, but I don't have a C# compiler on my home machine. \$\endgroup\$
    – asteri
    Commented Apr 18, 2014 at 22:02
  • \$\begingroup\$ It works. I guarantee it! \$\endgroup\$ Commented Apr 18, 2014 at 23:11

1 Answer 1

1
\$\begingroup\$

I don't like the underscore in the class name, HR_Repository. It's a generic name for a generic class, which is good, but seeing that it implements IRepository<T> I would expect something like this:

public class EmployeeRepository : IRepository<Employee>

I like that you're initializing your private readonly LoginDataContext in the constructor, from a constructor argument.

private readonly  LoginDataContext dataContext;
public HR_Repository(LoginDataContext dataContext)
{
    this.dataContext = dataContext;
}

I like it because this implementation of IRepository<T> has a dependency on the LoginDataContext - by passing it into the constructor, you're doing constructor injection and that's awesome. Ideally the dependency would be an abstraction (abstract class or interface), which makes it easier to inject as a mock or fake implementation when you're writing unit tests. Even if you never write these tests, the fact that your code would be testable is a very good thing.

I don't like that your static helper provides extension methods to IQueryable<Employee>, it's not clear how/where this code is being used, the benefits aren't immediately apparent.


As for the pattern itself, it looks like a hybrid Unit-of-Work / Repository implementation, because of the Commit method. But your implementation exposes ITable and IQueryable, which leaks out the ORM into the calling code.

If you consider the client as, say, a controller class (assuming MVC), the controller would have a dependency on the unit-of-work, which has a dependency on the data context and lets you work with one or more repositories, which all share the data context of the unit-of-work:

UoW+Repository

(I skipped UML classes, hope this doesn't look too weird)


Disclaimer

I'm biased. I'm not a fan of unit-of-work/repository pattern, at least not with Entity Framework. I think Entity Framework's DbContext is a unit-of-work, and the IDbSet<TEntity> properties it exposes are repositories - I think wrapping this in another level of abstraction has very little benefits and adds a considerable amount of complexity to the code.

Instead, I simply wrap DbContext with an interface:

public interface IUnitOfWork
{
    void SaveChanges();
    IDbSet<TEntity> Set<TEntity>();
}

public class MyContext : DbContext, IUnitOfWork
{
    IDbSet<MyEntity> MyEntities { get; set; }
    // ...
}

Then I usually have a "service" class that takes an IUnitOfWork dependency; I write closely related queries inside that class, but the "service" only returns materialized results, it doesn't expose IQueryable<T>, even less so ITable. A controller will take a factory that can instantiate such a "service", wrapping its instance in a using block to ensure proper disposal.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Here's some rantage from the great and powerful Rob Conery supporting your distaste for unit-of-work+repository: wekeroad.com/2014/03/04/… \$\endgroup\$ Commented Apr 20, 2014 at 19:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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