Many CRUD operations for a list of technicians' shifts

Question

The following code snippet is causing me a performance problem in my application. The first 5 db queries are not high time consuming, the real problem lays on the foreach loop. I considered to use Dictionary structures instead of List to improve the lookups, but improving the loop will make the real difference.

public void ProcessShifts(IList<NavTechAbsenceDto> navShifts)
{

    var branches = _iagiBranchModelFactory.GetAll().ToList();

    var technicians = _iagiTechnicianModelFactory.GetAll().Where(it => it.IsActive).ToList();

    var activeTechCodes = technicians.Select(t => t.ERPId).ToList();

    var absencesForActiveTech = navShifts.Where(a => activeTechCodes.Contains(a.TechCode)).ToList();

    var absences = _iagiTechnicianModelFactory.GetAllWithAbsences().SelectMany(it => it.Absences);

    IEnumerable<Absence> mwmAbsenceList = absences == null ? null : absences.Where(a => a.Start >= DateTime.Today);

    foreach (NavTechAbsenceDto absence in absencesForActiveTech)
    {
        var mwmAbsence = mwmAbsenceList == null ? null : mwmAbsenceList.FirstOrDefault(it => it.AGIErpId == absence.AGIErpId);

        AGIBranchModel branch = branches.FirstOrDefault(b => b.ERPId == absence.BranchCode);
        AGITechnicianModel tech = technicians.FirstOrDefault(t => t.ERPId == absence.TechCode);

        if (branch == null || tech == null)
        {
            Logger.Trace("NOT AVAILABLE: Shift Branch: {0}, Shift Technician: {1}", absence.BranchCode, absence.TechCode);
            continue;
        }

        if (mwmAbsence == null)
        {
            if (absence.AGILogicallyDeleted == 1)
            {

                Logger.Trace(string.Format("DELETED absence:{0} from Navision, but it does not exist in Scheduler", absence.AGIErpId));
            }
            else
            {
                Logger.Trace(string.Format("ADD absence:{0}", absence.AGIErpId));

                tech.AddAbsence(absence, branch);
            }
        }
        else
        {

            if (absence.AGILogicallyDeleted == 1)
            {
                Logger.Trace(string.Format("DELETE absence:{0}", absence.AGIErpId));
                tech.DeleteAbsence(mwmAbsence);
            }
            else
            {
                Logger.Trace(string.Format("UPDATE absence:{0}", absence.AGIErpId));

                mwmAbsence.Start = absence.Start;
                mwmAbsence.End = absence.End;
                mwmAbsence.AGIBranchId = branch.Id;
                mwmAbsence.TechnicianId = tech.Id;
                mwmAbsence.Technician = tech;
                mwmAbsence.AGIType = absence.AGIType;
                mwmAbsence.AGIMobile = absence.AGIMobile;
                mwmAbsence.AGIErpId = absence.AGIErpId;

                tech.UpdateAbsence(mwmAbsence);
            }
        }
    }
}

Answer 1

For what I see, it should suffice - in order to increase time performance of the cycle - to transform the branches, technicians, and mwmAbsenceList into dictionaries with the respective ids as keys. In this way the following lines:

        var mwmAbsence = mwmAbsenceList == null ? null : mwmAbsenceList.FirstOrDefault(it => it.AGIErpId == absence.AGIErpId);
        AGIBranchModel branch = branches.FirstOrDefault(b => b.ERPId == absence.BranchCode);
        AGITechnicianModel tech = technicians.FirstOrDefault(t => t.ERPId == absence.TechCode);

can be executed in \$O(1)\$ time instead of the current \$O(n)\$. The resulting code after the change should be something in the lines of:

        Absence mwmAbsence = null; 
        if(mwmAbsenceList != null)
        {
            mwmAbsenceList.TryGetValue(absence.AGIErpId, out mwmAbsence);
        }
        AGIBranchModel branch; 
        branches.TryGetValue(absence.BranchCode, out branch);
        AGITechnicianModel tech; 
        technicians.TryGetValue(absence.TechCode, out tech);

---

Another optimization I would suggest is to try and change the method into a set operation (try to get all items via a LINQ query). Just keep in mind that doing it in this way passes the burden to the database.

---

As for a review of the code I'd suggest three things:

1. Reduce the space between the lines of code. Right now is a bit too much.
2. Reduce indentation.
3. Reduce width.

The end result - after applying the changes in the first solution and the changes for these 3 points - should be something like the following:

public void ProcessShifts(IList<NavTechAbsenceDto> navShifts)
{
    var branches = _iagiBranchModelFactory.GetAll()
        .ToDictionary(/* insert key and value expressions here */);
    var technicians = _iagiTechnicianModelFactory.GetAll()
        .Where(it => it.IsActive)
        .ToDictionary(/* insert key and value expressions here */);
    var activeTechCodes = technicians.Select(t => t.ERPId).ToList();
    var absencesForActiveTech = navShifts
        .Where(a => activeTechCodes.Contains(a.TechCode)).ToList();
    var absences = _iagiTechnicianModelFactory.GetAllWithAbsences()
        .SelectMany(it => it.Absences);
    Dictionary</*TKey*/, /*TVal*/> mwmAbsenceList = absences == null 
        ? null 
        : absences.Where(a => a.Start >= DateTime.Today)
                  .ToDictionary(/* Insert key and value expressions here */);

    foreach (NavTechAbsenceDto absence in absencesForActiveTech)
    {
        Absence mwmAbsence = null; 
        if(mwmAbsenceList != null)
        {
            mwmAbsenceList.TryGetValue(absence.AGIErpId, out mwmAbsence);
        }

        AGIBranchModel branch; 
        branches.TryGetValue(absence.BranchCode, out branch);
        AGITechnicianModel tech; 
        technicians.TryGetValue(absence.TechCode, out tech);

        if (branch == null || tech == null)
        {
            Logger.Trace("NOT AVAILABLE: Shift Branch: {0}, Shift Technician: {1}", 
                absence.BranchCode, 
                absence.TechCode);
            continue;
        }

        if (mwmAbsence == null)
        {
            if (absence.AGILogicallyDeleted == 1)
            {
                // is string.Format() necessary?
                Logger.Trace(string.Format(
                    "DELETED absence:{0} from Navision, but it does not exist in Scheduler", 
                    absence.AGIErpId));
            }
            else
            {
                Logger.Trace(string.Format("ADD absence:{0}", absence.AGIErpId));
                tech.AddAbsence(absence, branch);
            }

            continue;
        }

        if (absence.AGILogicallyDeleted == 1)
        {
            Logger.Trace(string.Format("DELETE absence:{0}", absence.AGIErpId));
            tech.DeleteAbsence(mwmAbsence);
        }
        else
        {
            Logger.Trace(string.Format("UPDATE absence:{0}", 
                absence.AGIErpId));

            mwmAbsence.Start = absence.Start;
            mwmAbsence.End = absence.End;
            mwmAbsence.AGIBranchId = branch.Id;
            mwmAbsence.TechnicianId = tech.Id;
            mwmAbsence.Technician = tech;
            mwmAbsence.AGIType = absence.AGIType;
            mwmAbsence.AGIMobile = absence.AGIMobile;
            mwmAbsence.AGIErpId = absence.AGIErpId;

            tech.UpdateAbsence(mwmAbsence);
        }
    }
}

---

Let me know if anything's unclear.