0
\$\begingroup\$

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)

\$\endgroup\$

closed as off-topic by t3chb0t, Mast, Toby Speight, Ludisposed, alecxe Dec 14 '17 at 18:20

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ This is not a repository but a service. A repository doesn't have such specific methods. Your other question seems to be gone now, but that's the way to go. Remove the redundant repository layer. Another thing: just execute myobject.SingleOrDefault() and check for == null. Now you run two queries to get the object. \$\endgroup\$ – Gert Arnold Dec 9 '17 at 10:29
  • \$\begingroup\$ Thanks for the information @GertArnold I've updated my proposed -repository-, i've also updated the link to my other question \$\endgroup\$ – kolin Dec 14 '17 at 8:17
  • \$\begingroup\$ This does not look like real code at all. \$\endgroup\$ – t3chb0t Dec 14 '17 at 8:31
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ludisposed Dec 14 '17 at 8:39
2
\$\begingroup\$

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).

\$\endgroup\$

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