Parent/child relationships: adding/updating details to an answer

Question

Answer is the Parent table and AnswerDetail is the child. Below works, but I'm wondering if there is a better way to do this using EF?

My Method Signature is this:

    [HttpPost]
    public ActionResult Evaluator(EvaluationVM evaluation, string command)

It is coming from a MVC 4 application.

Answer AnswerRecord;
AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID)).Where(y => y.LeaderID != null).FirstOrDefault<Answer>();

if (AnswerRecord == null)
{
     //Add New Parent Record:
     AnswerRecord.CreatedBy = userName;
     AnswerRecord.CreateStamp = DateTime.Now;
     db.Entry(AnswerRecord).State = EntityState.Added;

     AnswerRecord.UpdatedBy = userName;
     AnswerRecord.UpdateStamp = DateTime.Now;
 }

 AnswerRecord.UpdatedBy = userName;
 AnswerRecord.UpdateStamp = DateTime.Now;

 //I use this block if I know the child record is new:
 adItem = db.AnswerDetails.Find(item.LeaderAnswerDetailKey);
  if (adItem != null)
  {
       adItem.Comment = item.LeaderComment;
       adItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value;
       adItem.UpdatedBy = userName;
       adItem.UpdateStamp = DateTime.Now;
       db.Entry(adItem).State = EntityState.Modified;
  }

 foreach (EvaluationObject item in evaluation.ResultSet)
 {
 //I use this block if I am updated the child record
 adItem = new AnswerDetail();
 adItem.QuestionID = item.QuestionID.Value;
 adItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value;
 adItem.Comment = item.TeacherComment;

 adItem.CreatedBy = userName;
 adItem.CreateStamp = DateTime.Now;
 adItem.UpdatedBy = userName;
 adItem.UpdateStamp = DateTime.Now;
 adItem.AnswerKey = AnswerRecord.AnswerKey;

 AnswerRecord.AnswerDetails.Add(adItem);
 db.Entry(adItem).State = EntityState.Added;

 //This is called at the end of my method:
 db.SaveChanges();

 }//End of ForEach Loop

EvaluationVM

public class EvaluationVM
{
    public bool IsAdmin { get; set; }
    public bool IsLeader { get; set; }
    public int TeacherStatus { get; set; }
    public int LeaderStatus {get;set;}

    public bool IsPublicTeacher { get; set; }
    public bool IsFinalTeacher { get; set; }
    public bool IsPublicLeader { get; set; }
    public bool IsFinalLeader { get; set; }

    public List<EvaluationObject> ResultSet {get; set;}
    public TeacherInfo CurrentTeacher {get; set;}
    public LeaderInfo CurrentLeader { get; set; }

    public List<EvaluationRating> RatingSet {get;set;}
}

EvaluationObject:

    public class EvaluationObject : IEvaluationObject
    {
        public int? QuestionID { get; set; }
        public string IndicatorID { get; set; }
        public string QuestionDescription { get; set; }

        public string TeacherID { get; set; }
        public int? TeacherStatusKey { get; set; }
        public int? TeacherAnswerKey { get; set; }
        public int? TeacherAnswerDetailKey { get; set; }
        public int? TeacherAnswerOptionKey { get; set; }
        public string TeacherComment { get; set; }

        public string LeaderID { get; set; }
        public int? LeaderStatusKey { get; set; }
        public int? LeaderAnswerKey { get; set; }
        public int? LeaderAnswerDetailKey { get; set; }
        public int? LeaderAnswerOptionKey { get; set; }
        public string LeaderComment { get; set; }

    }

Answer 1

Separation of Concerns

I like that you're using EF's built-in unit-of-work implementation. However db (your DbContext class) looks like it's declared at instance level in your controller - make sure it's disposed properly at the end of the request (you could inject the context in the controller's constructor and use an IoC container to ensure per-request instantiation & disposal).

That doesn't mean the controller's [HttpPost] methods should be doing all the work!

Looking at the method's code, I think you can extract an entire service class that exposes at least 3 methods!

• Answer CreateNewAnswer(string userName)
• AnswerDetail FindByLeaderId(int leaderId)
• AnswerDetail CreateNewAnswerDetail(int answerId, AnswerDetailVM item)

Extracting these methods into their own class will make it much easier to follow your controller methods' code, by adding a level of abstraction - the controller shouldn't be dealing with minute details, rather should call into more specialized objects that do their specialized stuff. Shortly put, separate the concerns ;)

---

SaveChanges

You don't need to call db.SaveChanges() for every entity you create in the loop - EF's DbContext is a unit-of-work that encapsulates a transaction, so calling SaveChanges is like saying "I'm done, now commit all these pending changes!" - call it once (or, as sparingly as possible - e.g. the parent would typically needs to exist in the db before a child can be added), when you're done.

---

Naming & Other Nitpicks

Answer AnswerRecord;
AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID)).Where(y => y.LeaderID != null).FirstOrDefault<Answer>();

For readability, I prefer to split these across multiple lines:

AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID))
                           .Where(y => y.LeaderID != null)
                           .FirstOrDefault<Answer>();

This is confusing, because x and y refer to the same object. Also I'd prefer == over .Equals in most cases, so I'd write it like this instead:

var answer = db.Answers.Where(answer => answer.TeacherID == evaluation.CurrentTeacher.ID
                                     && answer.LeaderID != null)
                       .FirstOrDefault();

The type parameter for FirstOrDefault is inferred from usage, doesn't need to be specified ;)

---

Comments

If these comments are real, in-code comments...

//I use this block if I know the child record is new:

//I use this block if I am updated the child record

...then they're both lying - the code blocks under each comment seems to be doing what the other comment is saying! Remove these misleading comments, calling _service.CreateNewAnswerDetail should be clear enough that you're creating a new AnswerDetail entry ;)

As for this one:

//Add New Parent Record:

and this one:

//End of ForEach Loop

...they both say nothing that the code doesn't say already, remove them as well, thank yourself later ;)