Entity Framework MVC updating db using a repository method [closed]

Question

I have inherited this code from a legacy project, and it is throwing a

New transaction is not allowed because there are other threads running in the session.

error.

Now I know the problem here is mainly that i'm not using a using statement in the repository, so the connection is stuck open.

My question is, how can I improve this code?

my initial ideas would be to 1) remove the entities from the constructor in the repository and have it created in the using statement (not yet put in).

Current Controller

static readonly IMyRepository myRepository = new MyRepository();

    public HttpResponseMessage PutObject(int id, int id2)
    {
        if(!myRepository.Update(id,id2))
        {
            return Request.CreateErrorResponse(HttpStatusCode.NotFound);
        }
        else
        {
            return Request.CreateResponse(HttpStatusCode.OK);
        }
    }

Repository

MyEntities _myEntities;

public MyRepository(MyEntities context)
{
    _myEntities = context;
}

public MyRepository()
{

}

    public bool Update(int id,int id2, string id3, int id4)
    {
        _myEntities = _myEntities ?? new MyEntities();

        //update by id if id2,id3 and id4 are zero
        if (id2 == 0 && id3 == "0" && id4 == 0)
        {
            var myobject = _myEntities.MyObjects.Where(x => x.id == id);
            if (myobject.Count() > 0)
            {
                MyObject temp = myobject.SingleOrDefault();
                temp.Processed = true;
                _myEntities.SaveChanges();
                return true;
            }
            else
            {
                return false;
            }
        }
        else
        {
            var myobject = _myEntities.MyObjects.Where(x => x.SourceID == id && x.ExternalID == id2 && x.InternalID == id3 && x.Code == id4 && x.Processed == false);
            if (myobject.Count() > 0)
            {
                myobject temp = myobject.SingleOrDefault();
                temp.Processed = true;
                _myEntities.SaveChanges();
                return true;
            }
            else
            {
                return false;
            }
        }
    }

My Proposals would lave me with something like :

Proposed Controller

IMyRepository myRepository = new MyRepository();

    public HttpResponseMessage PutObject(int id, int id2)
    {
        if(!myRepository.Update(id,id2))
        {
            return Request.CreateErrorResponse(HttpStatusCode.NotFound);
        }
        else
        {
            return Request.CreateResponse(HttpStatusCode.OK);
        }
    }

Proposed Repository

MyEntities _myEntities;

public MyRepository()
{

}

    public bool Update(int id,int id2, string id3, int id4)
    {
        //update by id if id2,id3 and id4 are zero
        if (id2 == 0 && id3 == "0" && id4 == 0)
        {
            using (var _myEntities = new MyEntities())
            {
                var myobject = _myEntities.MyObjects.Where(x => x.id == id);
                if (myobject.Count() > 0)
                {
                    MyObject temp = myobject.SingleOrDefault();
                    temp.Processed = true;
                    _myEntities.SaveChanges();
                    return true;
                }
                else
                {
                    return false;
                }
            }
        }
        else
        {
            using (var _myEntities = new MyEntities())
            {
                var myobject = _myEntities.MyObjects.Where(x => x.SourceID == id && x.ExternalID == id2 && x.InternalID == id3 && x.Code == id4 && x.Processed == false);
                if (myobject.Count() > 0)
                {
                    myobject temp = myobject.SingleOrDefault();
                    temp.Processed = true;
                    _myEntities.SaveChanges();
                    return true;
                }
                else
                {
                    return false;
                }
            }
        }
    }

Is this an acceptable change? are there other improvements I can make to this code? There are other parts of the codebase that are similar, and I want to make sure i'm doing it efficiently before I go changing other parts.

I also have another question here where I am looking at trying a whole different way of doing this (and getting rid of the respository)

Answer 1

If your MyRepository class performs creation and disposal of the DbContext (MyEntities I assume) for every single operation, it will mean it can never expose IQueryable< > datasets that further operations can be performed on (you'll always have to do .ToList() or .ToArray() on the result). Further, it will mean you'd need to explicitly use database transactions to control the atomicity of combined operations (e.g. if a particular API needs to make 3 separate Updates( ), and the changes should only be committed if all three succeed). If you're OK with these limitations, then what you've proposed can work, but it's definitely more flexible to have control over the lifetime of the DbContext at the business-logic level (above the Repository).