HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Entity Framework with multiple connections open?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withopenmultipleframeworkconnectionsentity

Problem

I'm learning Entity Framework and I'm wondering if this code is the proper way of doing this. Basically this is my attempt at refactoring a very long controller in ASP.Net MVC 4. I'm using Entity Framework 5 as well.

What I did was I separated out some code into a more common method (buildUpdatedAnswerDetailRecord).

Notice in my parent function I have a connection open because I'm doing other DB related operations (population of data into properties). But then in the buildUpdatedAnswerDetailRecord method I'm opening another connection and then passing back a AnswerDetail.

Is this the right way to do this or should I have just kept 1 connection open and just duplicated code?

```
using (var db = new LEAP_Professional_DAL.DAL.LEAPEntitiesDAL())
{

//local variable of a AnswerDetail
AnswerDetail adItem;

//Doing a lot of other Database work (filling other properties)
....

adItem = buildUpdatedAnswerDetailRecord(evaluation.IsLeader, item, backButtonItem.AnswerDetailKey);
db.Entry(adItem).State = EntityState.Modified;

....
//Doing a lot of other Database work (filling other properties)

}

private AnswerDetail buildUpdatedAnswerDetailRecord(Boolean isLeader, EvaluationObject item, int? detailKey)
{
try
{
AnswerDetail buildAdItem;
using (var db = new DAL.DAL.EntitiesDAL())
{
buildAdItem = db.AnswerDetails.Find(detailKey);
if (buildAdItem != null)
{
((IObjectContextAdapter)db).ObjectContext.Detach(buildAdItem);
if (isLeader == true)
{
buildAdItem.Comment = item.LeaderComment;
buildAdItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value;
buildAdItem.UpdatedBy = userName;
buildAdItem.UpdateStamp = DateTime.Now;
}
else
{

Solution

Just a quick note about the way exceptions are handled:

catch (Exception ex)
{
    throw new Exception("buildUpdatedAnswerDetailRecord Error Occured: " + ex.Message.ToString());
}


You're shooting yourself in the foot: you're receiving a potentially very meaningful and helpful exception, with a very detailed stack trace, then you're grabbing its message and throwing everything else away (including the stack trace), and throwing a meaningless System.Exception.

At the very least, you should be putting ex as the new exception's InnerException, so as to retain all the details about the exception that was thrown. See this answer for more details about why throwing System.Exception is bad.

Code Snippets

catch (Exception ex)
{
    throw new Exception("buildUpdatedAnswerDetailRecord Error Occured: " + ex.Message.ToString());
}

Context

StackExchange Code Review Q#49531, answer score: 4

Revisions (0)

No revisions yet.