views:

115

answers:

2

The Repository class has singleton behavior and the _db implements the disposable pattern. As excepted the _db object gets disposed after the first call and because of the singleton behavior any other call of _db will crash.

 [ServiceBehavior(InstanceContextMode=InstanceContextMode.Single)]
 public class Repository : IRepository
 {
    private readonly DataBase _db;

    public Repository(DataBase db)
    {
        _db = db;
    }

    public int GetCount()
    {
        using(_db)
        {
            return _db.Menus.Count();
        }
    }

    public Item GetItem(int id)
    {
        using(_db)
        {
            return _db.Menus.FirstOrDefault(x=>x.Id == id);
        }
    } 

  }

My question is, is there any way to design this class to work properly without removing the singleton behavior? The Repositoryclass will be serving big amount of requests.

+2  A: 

When you do this:

using(_db)

you are guaranteeing that Dispose() will be called on your object.

I would suggest changing this to use a static class with a private static member for _db.

public static class Repository {
  private static DataBase _db; 

  // static constructor
  static Repository () {
    _db = new Database();
  }

  public static int GetCount() {
    return _db.Menus.Count();
  }
  public Item GetItem(int id) {
    return _db.Menus.FirstOrDefault(x=>x.Id == id);
  }
}
Ray
+2  A: 

When you supply a DataBase instance to the Repository, this means creation and disposal is not in the scope of the Repository class. Therefore, Repository should not dispose that instance. You should remove the using statements, and you'll be fine. Write the Repository as follows:

public class Repository : IRepository
{
    private readonly DataBase _db;

    public Repository(DataBase db)
    {
        _db = db;
    }

    public int GetCount()
    {
        return _db.Menus.Count();
    }

    public Item GetItem(int id)
    {
        return _db.Menus.FirstOrDefault(x=>x.Id == id);
    } 
}
Steven
As the Repository class acts like a singleton, keeping a long running context is a bad practice. It may grow very fast in memory consumption.
I agree that keeping the context running for a long time is a bad practice. In that case you've got two options. 1. Don't use the `Repository` as a singleton and return a new instance on each request. 2. Pass the `DataBase` class on each call to the method. I'd go for option 1. Don't forget that next to the dispose problem, you need to make sure your `DataBase` is thread-safe when you return the `Repository` as a singleton (when you're in a multi-threaded environment).
Steven
if keeping a long running context is a bad practice why is it a singleton?
Vitalik
Because of performance. The Repository class will be serving big amount of requests.