1
\$\begingroup\$

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);
            }
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Is this entity framework? You can use .Include to load the foreign rows immediately. Alternatively, if that's not possible, you can create a list of absence.BranchCode, query Branches.Where(b => branchCodes.Contains(b.EPRId)) and then resolve the matches in memory once you've got them all. A fast way would be to put the results in a dictionary \$\endgroup\$
    – Rob
    Jan 12, 2016 at 10:24

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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