How to Improve Entity Framework and Javascript Interaction

Question

This is a pretty vague/subjective question. I want to know if this is the best way to send/retrieve data to/from the browser using ajax calls. On the back end webservice, I want to use the entity framework. Below are two example functions.

The criteria for "best" is speed of writing code, readable code, and robust architecture.

Thanks for any feedback and suggestions and comments.

Get Function

[WebMethod]
public AjaxEmployee EmployeeGetById(int employeeID, bool getTimeOff)
{
    using (Time_TrackerEntities ctx = new Time_TrackerEntities())
    {
        var results = from item in ctx.Employees
                      where item.ID == employeeID
                      orderby item.Last_Name
                      select new AjaxEmployee
                      {
                          ID = item.ID,
                          Employee_ID = item.Employee_ID,
                          First_Name = item.First_Name,
                          Middle_Name = item.Middle_Name,
                          Last_Name = item.Last_Name,
                          Supervisor_ID = item.Supervisor_ID,
                          Active = item.Active,
                          Is_Supervisor = item.Is_Supervisor
                      };
        var emp = results.FirstOrDefault();
        if (getTimeOff)
        {
            var results2 = from item2 in ctx.Time_Off
                           where item2.Employee_ID == emp.Employee_ID
                           select new AjaxTime_Off
                           {
                               ID = item2.ID,
                               Employee_ID = item2.Employee_ID,
                               Date_Off = item2.Date_Off,
                               Hours = item2.Hours
                           };
            emp.Time_Off = results2.ToList<AjaxTime_Off>();
        }

        return emp;
    }
}

Save Function

[WebMethod]
public bool EmployeeSave(AjaxEmployee emp)
{
    using (Time_TrackerEntities ctx = new Time_TrackerEntities())
    {
        var results = from item in ctx.Employees
                      where item.ID == emp.ID
                      select item;

        var myEmp = results.FirstOrDefault();
        if (myEmp == null)
        {
            myEmp = new Employee();
            ctx.Employees.AddObject(myEmp);
        }

        myEmp.Employee_ID = emp.Employee_ID;
        myEmp.First_Name = emp.First_Name;
        myEmp.Middle_Name = emp.Middle_Name;
        myEmp.Last_Name = emp.Last_Name;
        myEmp.Supervisor_ID = emp.Supervisor_ID;
        myEmp.Active = emp.Active;
        myEmp.Is_Supervisor = emp.Is_Supervisor;

        return ctx.SaveChanges() > 0;
    }
}

Answer 1

There are a few improvements to be made.

Save() Method - Don't left-to-right copy, use EF built in logic

Instead of this:

myEmp.Employee_ID = emp.Employee_ID;
myEmp.First_Name = emp.First_Name;
myEmp.Middle_Name = emp.Middle_Name;
myEmp.Last_Name = emp.Last_Name;
myEmp.Supervisor_ID = emp.Supervisor_ID;
myEmp.Active = emp.Active;
myEmp.Is_Supervisor = emp.Is_Supervisor;

You can do this:

ctx.Employees.ApplyCurrentValues(emp).

What this does, is look for an entity with the same key in the graph (which there is, since you have just retrieved it with FirstOrDefault()), and override the scalar values with the entity you pass in - which is exactly what your doing.

So your 7 lines becomes 1, plus if you add any extra scalar properties - you won't have to refactor your code. Just remember - only works for scalar properties, not navigational properties.

Why build query for primary key retrieval? Just use predicate to SingleOrDefault()

Instead of this:

var results = from item in ctx.Employees
              where item.ID == emp.ID
              select item;

var myEmp = results.FirstOrDefault();

Do this:

var myEmp = ctx.Employees.SingleOrDefault(x => x.ID == emp.Id);

Or even better, use a pipe/filter technique:

var myEmp = ctx.Employees.WithId(emp.Id).SingleOrDefault();

Where WithId is an IQueryable<Employee> extension method, which filters the query based on the supplied employee ID. This allows de-coupling of filtering/business logic from your repository/DAL. It should go in your domain model, so you can have a nice fluent API for query your domain entities via your ORM.

When your retrieving an entity via the primary key, you should always use SingleOrDefault() or Single(), never FirstOrDefault() or First(). If it's a primary key - there should only be one of them, so you should throw an exception if more than one exists, which is what SingleOrDefault() does. And as @Shiraz mentions - your FirstOrDefault() will crash the query below. You always need null checking when you use <First/Single>OrDefault().

The same improvements can be made to your Get method.

Overall, there is nothing functionally wrong with your code - it just needs subtle improvements, null checking and exception handling.

The only functional improvement i highly recommend is refacting your web service code into a Generic Repository. As the code is very trivial and can be re-used across any entity. The web service shouldn't be concerned with transactions, primary key's or EF logic whatsoever. It shouldn't even have a reference to the EF DLL. Encapsulate this logic behind a repository and delegate the persistence logic to there (via an interface of course).

After making the changes i've mentioned above, your web service methods should have no more than 5-7 lines of code each.

You have far too much intelligence in your web service - it should be dumb and persistent ignorant.