Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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());
        }
    }
share|improve this question
    
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. –  Bassam Alugili May 14 at 6:59
    
@BassamAlugili - Please post an example or a good link to a good tutorial. Any and all suggestions welcomed. –  webdad3 May 14 at 14:39

3 Answers 3

up vote 3 down vote accepted

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 to true or false - just use the Boolean value as the condition, that's what a bool 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.

share|improve this answer
    
So you are saying I could of passed in the DBContext? As far the Duplicated code. I was duplicating code before I refactored it. –  webdad3 May 12 at 15:10
2  
Yes, you can pass db context into functions. –  C Sharper May 12 at 15:14
    
Where am I detaching Entities and management the state myself? –  webdad3 May 12 at 15:18
    
@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 your DbContext around as a parameter, but at the end of the refactoring process it would be nice to have a class that encapsulates the DbContext and exposes methods that work with it. –  Mat's Mug May 12 at 15:20
2  
@webdad3 I'd keep it simple - just make a service class that takes a DbContext constructor parameter (the constructor assigns a private 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 the DbContext :) –  Mat's Mug May 12 at 15:46

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
}
share|improve this answer

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.

share|improve this answer
    
Also calling ToString() on ex.Message is redundant, ex.Message is already a string ;) –  Mat's Mug May 12 at 16:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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