Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am building a new system that is using some tables from an old system.

For each user on this new system, I need to go to the old system and total up their sales. Currently it takes between 2-5 minutes to load.

public static List<DailyTeamGoal> GetListDailyTeamGoals(int teamId)
{
    string teamGoal = "";
    List<ProPit_User> lstProPit_User = new List<ProPit_User>();
    using (ProsPitEntities db = new ProsPitEntities())
    {
        // Find the team
        Team team = db.Teams.Where(x => x.teamID == teamId).FirstOrDefault();

        if (team != null)
        {
            // Grab team goal
            teamGoal = Convert.ToString(team.goal);
        }
        // Make a list of all users who are on the team
        lstProPit_User = db.ProPit_User.Where(x => x.teamID == teamId).ToList();
    }

    List<DailyTeamGoal> lstDailyTeamGoal = new List<DailyTeamGoal>();
    using (TEntities db = new TEntities())
    {
        //have to get every day of the month
        DateTime dt = DateTime.Now;
        int days = DateTime.DaysInMonth(dt.Year, dt.Month);
        decimal orderTotal = 0m;
        for (int day = 1; day <= days; day++)
        {
            // For every day in the month total the sales
            DailyTeamGoal dtg = new DailyTeamGoal();
            dtg.Date = day.ToString(); //dt.Month + "/" + day; + "/" + dt.Year;
            dtg.TeamGoal = teamGoal;
            decimal orderTotalRep = 0m;
            foreach (var propit_User in lstProPit_User)
            {
                DateTime dtStartDate = Convert.ToDateTime(dt.Month + "/" + day + "/" + dt.Year);
                DateTime dtEndDate = dtStartDate.AddDays(1);
                var lstorderTotalRep = (from o in db.Orders
                                    where o.DateCompleted >= dtStartDate
                                    where o.DateCompleted <= dtEndDate
                                    where (o.Status == 1 || o.Status == 2)
                                    where o.Kiosk != 0
                                    where o.SalesRepID == propit_User.SalesRepID
                                    orderby o.OrderTotal descending
                                    select o.OrderTotal).ToList();

                foreach (var item in lstorderTotalRep)
                {
                    //orderTotalRep =+ item;
                    orderTotalRep += item;
                }
            }

            orderTotal += orderTotalRep;
            dtg.DailyTotal = orderTotal;

            lstDailyTeamGoal.Add(dtg);
        }
    }

    return lstDailyTeamGoal;
}

The above code will use my site to find the teamID the logged in person is on. It will then find all members who are on that team. For each member it finds it will calculate the total sales and then spit me back a list. Any way to speed this up?

share|improve this question
2  
Is there an index on Orders including columns DateCompleted, Status, Kiosk and SalesRepID? How many rows are we talking about? Have you profiled anything to see where the bottleneck might be? –  Mat's Mug Apr 23 at 19:20
    
I have not profiles it yet. I found out the bottle neck, it isn't the query itself. The problem was for each day of the month I was running a single query to sum the total sales per user found on the team. Which came out to be around 900+ queries being ran. I've knocked it down to about 30 and is currently at least somewhat acceptable on speed around 30 seconds. Still trying to improve. –  James Wilson Apr 23 at 21:12
add comment

3 Answers 3

up vote 3 down vote accepted

I won't comment about the fact that this is a static method in what's possibly a static "helper" class and that it would be much prettier in a service class instance... but I just did.

The method is doing way too many things.

Thing one:

// Make a list of all users who are on the team

That's one method. But there's a twist: you not only need all users on the team, but also the teamGoal figure. I'd encapsulate that in its own class:

public class TeamGoalAndUsersResult // todo: rename this class
{
    public IEnumerable<ProPit_User> Users { get; private set; }
    public string Goal { get; private set; }

    public TeamGoalAndUsersResult(string goal, IEnumerable<ProPit_User> users)
    {
        Users = users;
        Goal = goal;
    }
}

And then you can write a method/function that will return that type:

private TeamGoalAndUsersResult GetTeamGoalAndUsers(int teamId)
{
    using (ProsPitEntities db = new ProsPitEntities())
    {
        var goal = string.Empty;
        var team = db.Teams.SingleOrDefault(x => x.teamID == teamId);
        if (team != null)
        {
            goal = Convert.ToString(team.goal);
        }

        var users = db.ProPit_User.Where(x => x.teamID == teamId).ToList();
        return new TeamGoalAndUsersResult(goal, users);
    }
}

Performance Issue:

            var lstorderTotalRep = (from o in db.Orders
                                where o.DateCompleted >= dtStartDate
                                where o.DateCompleted <= dtEndDate
                                where (o.Status == 1 || o.Status == 2)
                                where o.Kiosk != 0
                                where o.SalesRepID == propit_User.SalesRepID
                                orderby o.OrderTotal descending
                                select o.OrderTotal).ToList();

            foreach (var item in lstorderTotalRep)
            {
                //orderTotalRep =+ item;
                orderTotalRep += item;
            }

You're doing all the computation on the client. I'd take a wild guess and say that this is where your bottleneck is. Do you really need to select, sort and materialize all orders just to sum up OrderTotal?

var total = db.Orders.Where(o => o.DateCompleted >= dtStartDate
                              && o.DateCompleted <= dtEndDate
                              && (o.Status == 1 || o.Status == 2)
                              && o.Kiosk != 0
                              && o.SalesRepID == user.SalesRepID)
                     .Sum(o => o.OrderTotal);

orderTotal += total;
dtg.DailyTotal = orderTotal;
dailyTeamGoals.Add(dtg);

The above doesn't sort anything, has the same criterion and performs everything on the server side, without materializing every order in the way. Also I believe if the table has all the fields used in the WHERE, in an index (guts tell me "especially DateCompleted"), that would further help the server-side processing, but that's something you're probably better off finding out with a profiler.

share|improve this answer
1  
+1 I definitely consider the ordering and ToList() the first point of call for bottle necking here –  dreza Apr 23 at 20:15
    
All that extra stuff was not necessary so I removed it and just summed it up now. I also just sum up the complete teams sales instead of each users sales. It also ran each day of the month, even if it was the first day of the month so I forced it to only run for the days that have passed + current day which sped it up to very fast speed still around 30 seconds toward the end of the month however. –  James Wilson Apr 24 at 14:27
    
@JamesWilson feel free to post your updated code as a new question, I for one, would be happy to review it and see what else can be fine-tuned ;) –  Mat's Mug Apr 24 at 14:56
1  
@Mat'sMug I posted the new code under a new question for review if interested. codereview.stackexchange.com/questions/48082/… –  James Wilson Apr 24 at 19:58
add comment

I can't shake this feeling that you could use grouping to get the same result of totaling up the sales of the month hence effectively doing all of the hard work in SQL and limiting the amount of queries required.

Utitlising Matt's excellent answer here is something I've come up with.

public static List<DailyTeamGoal> GetListDailyTeamGoals(int teamId)
{    
    // utitilising the method suggested by Mats Mug below
    var team = GetTeamGoalAndUsers(teamId); 
    var dailySales = GetTeamDailySalesTotal(team.Users);

    return dailySales.Select(p => new DailyTeamGoal
     {
        Date = p.Key,
        TeamGogal = team.Goal,
        DailyTotal = p.Value
     });
}

private static Dictionary<int, int> GetTeamDailySalesTotal(IEnumerable<ProPit_User> teamMembers)
{
    // Date ranges
    DateTime now = DateTime.Now;
    DateTime dtStartDate = new DateTime(now.Year,now.Month,1);
    int numberOfDaysInMonth = dtStartDate.DaysInMonth();
    DateTime dtEndDate = new DateTime(now.Year,now.Month,numberOfDaysInMonth);  

    // ensure every day has at least an entry for it even if there are 0 sales on that day
    var sales = InitialiseSalesPerMonth(numberOfDaysInMonth);

    // Get the total sum of the daily sales for each rep
    var dailySalesTotal = (from o in orders
      where o.DateCompleted >= dtStartDate
      && o.DateCompleted <= dtEndDate
      && (o.Status == 1 || o.Status == 2)
      && o.Kiosk != 0
      && teamMemberIds.Any(m =>m == o.SalesRepID)
      group o by o.DateCompleted.Day into s
      select new {
            Day = s.Key,
            OrderTotal = s.Sum(p => p.OrderTotal)
      }).ToList();

    dailySalesTotal.Foreach(p => sales[p.Day] = p.OrderTotal);      

    return sales;
}

 private Dictionary<int, int> InitialiseSalesPerMonth(int numberOfDays)
 {
    var sales = new Dictionary<int, int>();

    for(int i = 1; i <= numberOfDays; i++)
    {
        sales.Add(i, 0);
    }

    return sales;
 }
share|improve this answer
add comment

As a rough rule of thumb, I assume that performance is as follows:

  • Fastest: calling methods in my own process
  • Fastish: calling methods in my run-time library (e.g. the .NET framework)
  • Slowish: calling O/S methods for I/O
  • Slowest: using the network to call methods on another machine

I assume that your function is slow because you are making hundreds of SQL calls:

  • For each team member
  • For each day in the month

There may be two ways to speed this up:

  1. For each day, select the orders for all team members.

    The culprit is the where o.SalesRepID == propit_User.SalesRepID clause which selects for one team member.

    I guess that translates to a SQL expression like WHERE Orders.SalesRepID = 3.

    Instead you want something which translates to a WHERE ... IN expression (where you pass in the IDs of all team members); or, do a JOIN against the teams table.

  2. The other thing you could try to do is select all data for the whole month (using one SQL SELECT statement) into a cache of the data in your own process.

    This is possible using e.g. the DataSet and DataTable classes in ADO.NET (I don't know whether/how you can do it using the Entity Framework).

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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