2
\$\begingroup\$

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; }

    }
\$\endgroup\$
  • \$\begingroup\$ Are all code blocks in the same method? If so, it would be nice if you could add the method's signature; if not, splitting the code block into distinct methods would make it easier to review. \$\endgroup\$ – Mathieu Guindon Apr 28 '14 at 16:15
  • \$\begingroup\$ @Mat'sMug - The Method is a controller from a MVC application. So it is the form that got submitted. \$\endgroup\$ – webdad3 Apr 28 '14 at 16:17
  • \$\begingroup\$ Is there more to the method? I commented that, because right now it's not immediately apparent where userName, evaluation and item come from and what type they are - having the method's signature would help connect the dots ;) \$\endgroup\$ – Mathieu Guindon Apr 28 '14 at 16:21
  • \$\begingroup\$ @Mat'sMug - I updated the question with more information. Let me know if you need more? \$\endgroup\$ – webdad3 Apr 28 '14 at 16:27
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ With the db.Save() the way it currently is (inside the for loop), I have a similar method that does the same thing except it does deletes (finds a record then deletes it) and the inserts new records. Like I said the db.save() is currently inside the foreach loop like I have above. Would that possibly cause a deadlock on the table? Let me know if you need further info? \$\endgroup\$ – webdad3 May 5 '14 at 17:04
  • 1
    \$\begingroup\$ You cannot garantee the order of operations in db.Save, so if you need to first delete records, delete them, Save(), and then add the new ones. It would probably be best to update than delete+insert though. \$\endgroup\$ – Mathieu Guindon May 5 '14 at 17:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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