What is wrong with using static class like the one below?
For one thing, your class isn't static, which means your client code can create an instance of it... which would make no sense at all.
Before the advent of static classes, you would prevent instantiation by sealing the class and providing a non-public default/parameterless constructor, like this:
public sealed class TenantManager
{
private TenantManager() { }
Making the class static pretty much does that for you, and prevents anyone from inheriting the class and/or calling its constructor.
In Object-Oriented Programming, a class defines the "blueprint" for an object - its type. By making a class static, you're denying it the ability to create objects; you're writing procedural code, like vba macro-recorder code in a module - you're giving anyone anywhere the ability to run this code, because the code lives in a global state, a static context, a type, as opposed to an object. This simply isn't OOP code.
It wouldn't be that bad, if all there ever was to your application was a Tenant entity. I don't know about the rest of your app, but say you have new requirements and now you have to add a Landlord entity, with the corresponding LandlordManager class.
So you go and implement another static class:
public static class LandlordManager
{
public static IEnumerable<Landlord> GetAll(AppDbContext context)
{
return context.Landlords;
}
public static Landlord GetById(int id, AppDbContext context)
{
return context.Landlords.Find(id);
}
public static Landlord Create(Landlord entity, AppDbContext context)
{
var landlord = context.Landlords.Add(entity);
context.SaveChanges();
return landlord;
}
public static void Update(Landlord entity, AppDbContext context)
{
context.Entry(entity).State = EntityState.Modified;
context.SaveChanges();
}
public static void Delete(int id, AppDbContext context)
{
var landlord = context.Landlords.Find(id);
context.Landlords.Remove(landlord);
context.SaveChanges();
}
}
Notice anything? Yes, I fixed the vertical whitespace and the shy indentation. Anything else? Isn't it striking how similar the two classes are? The two types desperately want to share an interface, but the classes being static, they can't.
public interface IEntityManager<TEntity>
{
IEnumerable<TEntity> GetAll(AppDbContext context);
TEntity GetById(int id, AppDbContext context);
TEntity Create(TEntity entity, AppDbContext context);
void Update(TEntity entity, AppDbContext context);
void Delete(TEntity entity, AppDbContext context);
}
The fact that every single member of the interface takes an AppDbContext parameter, is a very strong indicator that the parameter belongs at instance level. This simplifies the interface to this:
public interface IEntityManager<TEntity>
{
IEnumerable<TEntity> GetAll();
TEntity GetById(int id);
TEntity Create(TEntity entity);
void Update(TEntity entity);
void Delete(TEntity entity);
}
This is really starting to look like something familiar - in fact only the name is different:
public interface IRepository<TEntity>
{
IEnumerable<TEntity> GetAll();
TEntity GetById(int id);
TEntity Create(TEntity entity);
void Update(TEntity entity);
void Delete(TEntity entity);
}
I do not use repository. The entity framework is uow/repository.
The thing is, you are using a repository. You just gave it a different name. I can call an apple an orange all I want, it's still an apple. Yes, the EF DbContext is a powerful unit-of-work. Yes, an EF DbSet<T> is a nice repository. Yet you still feel the need to tuck it into its own type - and that's a good thing!
The difference between your TenantManager and the above IRepository<TEntity> interface, is that TenantManager is tightly coupled with AppDbContext, while it's an implementation detail as far as IRepository<TEntity> is concerned - the consuming/client code doesn't need to even know about AppDbContext. In your code, AppDbContext is a dependency: not only your code needs to be tightly coupled with a TenantManager, it's also coupled with AppDbContext. "Manager" is a smell all by itself - you don't have a "Manager", you have a repository... whether you like it or not!
Good design doesn't spread its dependencies to the client/consuming code; good design is cohesive, and has low coupling. That's why SOLID OOP guidelines tell us to depend on abstractions, not concrete types. With client code that only needs to know about an IRepository<Tenant>, the only concrete type it's working with is Tenant. With your code, it needs to know about Tenant, TenantManager, and AppDbContext. That's not exactly what low coupling is about!
Many speak of using dependency injection.
There's a reason for this. Loosely coupled code is much easier to write tests for. And the DI principle (all of SOLID actually) completely steer your code/design in that direction.
Say you have a controller - with your code it could look like this:
public class SomeController : Controller
{
public ActionResult Tenant(int id)
{
using (var context = new AppDbContext(connectionString))
{
var tenant = TenantManager.GetById(id, context);
return View(tenant);
}
}
}
With proper DI it could look like this:
public class SomeController : Controller
{
private readonly IRepository<Tenant> _repository;
public SomeController(IRepository<Tenant> repository)
{
_repository = repository;
}
public ActionResult Tenant(int id)
{
var tenant = _repository.GetById(id);
return View(tenant);
}
}
Both do exactly the same thing. Yet, only one of these two snippets can be unit tested.
