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
{
buildAdItem.Comment = item.TeacherComment;
buildAdItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value;
buildAdItem.UpdatedBy = userName;
buildAdItem.UpdateStamp = DateTime.Now;
}
}
}
return buildAdItem;
}
catch (Exception ex)
{
throw new Exception("buildUpdatedAnswerDetailRecord Error Occured: " + ex.Message.ToString());
}
}
-
\$\begingroup\$ Why ou do not just use Unit of Work with Repository Pattern I can post for you an example this is very good to manage the multipe database Connections and give you a very good separation of concerns. I can post for you an example how to use UOW and Repository pattern in EF but the learn curve is Little bit hight. \$\endgroup\$Bassam Alugili– Bassam Alugili2014年05月14日 06:59:05 +00:00Commented May 14, 2014 at 6:59
-
\$\begingroup\$ @BassamAlugili - Please post an example or a good link to a good tutorial. Any and all suggestions welcomed. \$\endgroup\$webdad3– webdad32014年05月14日 14:39:14 +00:00Commented May 14, 2014 at 14:39
3 Answers 3
This answer is just a little remark about this redundant part:
if (isLeader == true) { buildAdItem.Comment = item.LeaderComment; buildAdItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value; buildAdItem.UpdatedBy = userName; buildAdItem.UpdateStamp = DateTime.Now; } else { buildAdItem.Comment = item.TeacherComment; buildAdItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value; buildAdItem.UpdatedBy = userName; buildAdItem.UpdateStamp = DateTime.Now; }
Which could be simplified to:
buildAdItem.UpdatedBy = userName;
buildAdItem.UpdateStamp = DateTime.Now;
if (isLeader)
{
buildAdItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value;
buildAdItem.Comment = item.LeaderComment;
}
else
{
buildAdItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value;
buildAdItem.Comment = item.TeacherComment;
}
- Don't compare a
bool
totrue
orfalse
- just use the Boolean value as the condition, that's what abool
is for! - Avoid copy+pasting code. There's never a very good reason to do that.
Is this the right way to do this or should I have just kept 1 connection open and just duplicated code?
Your code structure isn't very clear (it's actually very unclear) from what you've given us - based on what I'm seeing I don't see why the two methods couldn't be using the same DbContext
, nor why using the same connection would result in "duplicated code".
Also the entity state manager is pretty good at managing entities all by itself, it seems to me that you're making your life harder than it needs to be by detaching entities and managing state yourself.
-
\$\begingroup\$ So you are saying I could of passed in the DBContext? As far the Duplicated code. I was duplicating code before I refactored it. \$\endgroup\$webdad3– webdad32014年05月12日 15:10:04 +00:00Commented May 12, 2014 at 15:10
-
2\$\begingroup\$ Yes, you can pass db context into functions. \$\endgroup\$CSharper– CSharper2014年05月12日 15:14:28 +00:00Commented May 12, 2014 at 15:14
-
\$\begingroup\$ Where am I detaching Entities and management the state myself? \$\endgroup\$webdad3– webdad32014年05月12日 15:18:24 +00:00Commented May 12, 2014 at 15:18
-
\$\begingroup\$ @webdad3 Here:
((IObjectContextAdapter)db).ObjectContext.Detach(buildAdItem);
and here:db.Entry(adItem).State = EntityState.Modified;
;) - and CSharper is correct, there's no harm in passing yourDbContext
around as a parameter, but at the end of the refactoring process it would be nice to have a class that encapsulates theDbContext
and exposes methods that work with it. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2014年05月12日 15:20:23 +00:00Commented May 12, 2014 at 15:20 -
2\$\begingroup\$ @webdad3 I'd keep it simple - just make a service class that takes a
DbContext
constructor parameter (the constructor assigns aprivate readonly DbContext _context
field), and then the public methods can just consume_context
, and whoever created the instance of that service class (the controller perhaps?) is also responsible for creating and disposing of theDbContext
:) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2014年05月12日 15:46:04 +00:00Commented May 12, 2014 at 15:46
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.
-
\$\begingroup\$ Also calling
ToString()
onex.Message
is redundant,ex.Message
is already astring
;) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2014年05月12日 16:00:04 +00:00Commented May 12, 2014 at 16:00
yes, you can pass db context through functions
using(MyEntities context = new MyEntities())
{
SomeObject record = //pull record;
SomeFunction(record, context);
}//connection closes here
void SomeFunction(SomeObject record, MyEntities context)
{
//do some stuff on record
}