Getting list of daily team goals

Question

The old code I had before was atrociously slow but after some advice and research I was able to take a 2-5 minutes run time down to about 5-30 seconds. That is acceptable, but still looking to have it run as quickly as possible.

I'm still cleaning it up, like there shouldn't be two seperate PP entities calls.

public static List<DailyTeamGoal> GetListDailyTeamGoals(int teamId)
{
 string teamGoal = "";
 List<ProPit_User> lstProPit_User = new List<ProPit_User>();
 using (PPEntities db = new PPEntities())
 {
 Team team = db.Teams.Where(x => x.teamID == teamId).FirstOrDefault();
 if (team != null)
 {
 teamGoal = Convert.ToString(team.goal);
 }
 lstProPit_User = db.ProPit_User.Where(x => x.teamID == teamId).ToList();
 }
 List<DailyTeamGoal> lstDailyTeamGoal = new List<DailyTeamGoal>();
 using (TEntities db = new TEntities())
 using (PPEntities ppdb = new PPEntities())
 {
 //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 <= DateTime.Now.Day; day++)
 {
 DailyTeamGoal dtg = new DailyTeamGoal();
 dtg.Date = day.ToString(); //dt.Month + "/" + day; + "/" + dt.Year;
 dtg.TeamGoal = teamGoal;
 //decimal orderTotalRep = 0m;
 DateTime dtStartDate = Convert.ToDateTime(dt.Month + "/" + day + "/" + dt.Year);
 DateTime dtEndDate = dtStartDate.AddDays(1);
 var ordersQuery = db.Orders.Where(o => o.DateCompleted >= dtStartDate
 && o.DateCompleted <= dtEndDate
 && (o.Status == 1 || o.Status == 2)
 && o.Kiosk != 0).ToList();
 var lstSalesRepIDs = ppdb.ProPit_User
 .Where(x => x.teamID == teamId).Select(v => v.SalesRepID).ToList();
 var total = (from o in ordersQuery
 where lstSalesRepIDs.Contains(o.SalesRepID)
 select o.OrderTotal).Sum();
 orderTotal += total;
 dtg.DailyTotal = orderTotal;
 lstDailyTeamGoal.Add(dtg);
 }
 }
 return lstDailyTeamGoal;
}

• user41269: Is this faster? abs( o.DateCompleted - (dtStartDate + 0.5) ) < 0.5

Answer 1

You are calling queries in a for-loop for each day. Database queries are slow compared to C# code execution. Send only one query to the DB for the whole range of days.

You can do the grouping by day either as DB query or query the ungrouped records and group using LINQ-to-Objects later.

Also you are querying the lstSalesRepIDs within the loop. Why? The teamId seems not to change between the loop iterations. Query it before the loop.

Also your where-clause tests the days like this: o => o.DateCompleted >= dtStartDate && o.DateCompleted <= dtEndDate. Where dtEndDate is one day more than dtStartDate. It looks to me as if you were taking two days at a time. Is this intended? If you want to include all times of a single day, change the comparison to o.DateCompleted >= dtStartDate && o.DateCompleted < dtEndDate. (Note: I used < instead of <= for the upper bound.)

Don't make date calculations by converting from DateTime to string and back! DateTime contains all the functionality you need, without using any dirty tricks.

---

var salesRepIDs = new HashSet<int>(ppdb.ProPit_User
 .Where(ppu => ppu.teamID == teamId)
 .Select(ppu => ppu.SalesRepID)
 .AsEnumerable()); // HashSet.Contains is faster than List.Contains.
DateTime today = DateTime.Today;
DateTime startDate = new DateTime(today.Year, today.Month, 1);
DateTime endDate = today.AddDays(1);
// Note: If the DB contains no entries with future dates, 
// you can drop the test for the upper date!
var orders = db.Orders
 .Where(o => o.DateCompleted >= startDate
 && o.DateCompleted < endDate
 && (o.Status == 1 || o.Status == 2)
 && o.Kiosk != 0)
 .AsEnumerable();
// Because of AsEnumerable() we are working with LINQ-to-Objects now.
var ordersByCompletionDate = orders
 .Where(o => salesRepIDs.Contains(o.SalesRepID))
 .GroupBy(o => o.DateCompleted.Date); // .Date in order to drop the time part.
foreach (var dayGroup in ordersByCompletionDate) {
 decimal daylyTotal = dayGroup.Sum(o => o.OrderTotal);
 // ...
}

.AsEnumerable() has the advantage over .ToList() that the source is consumed on the fly, item by item. No in-memory list needs to be created.

The salesRepIDs.Contains(..) part requires special attention. It could be performed as part of the DB query; however depending on the implementation and the number of salesRepIDs this could create queries with long IN lists: WHERE id IN (1, 2, 3, ..). There is a limit for the length of these lists. This can lead to "Query too complex" exceptions. If this is not the case, i.e., if you know that you will always have a reasonable number of ID's, could can simplify the whole code and do everything (filtering, grouping and summing) with only one single DB query.

• James Wilson: I'm definitely interested in simplifying the query calls and have adopted a lot of the suggestions here. The .contains() portion could 'technically' be unlimited. But realistically it's around 30 users per team. Still working on changing my method over to these suggestions and then will begin to test it. Thanks for the advice.
• James Wilson: o.DateCompleted.Date returns an error for me. System.Nullable<System.DateTime> does not contain a definition for 'Date' and no extension method 'Date' accepting a first argument of type 'System.Nullable<Sustem.DateTime>' could be found.
• Olivier Jacot-Descombes: If DateCompleted is a Nullable<DateTime> then change the expression to o.DateCompleted.Value.Date.

Answer 2

I agree with Olivier Jacot-Descombes on the functional aspects of this code, but it also lacks organization, meaningful naming, and consistency, which will make it harder for other readers (or yourself in future years) to follow. Some examples:

public static List<DailyTeamGoal> GetListDailyTeamGoals(int teamId)
...
List<DailyTeamGoal> lstDailyTeamGoal = new List<DailyTeamGoal>();
...
DateTime dt = DateTime.Now;
...
int days = DateTime.DaysInMonth(dt.Year, dt.Month);

With modern strongly typed languages like C#, adding the Type to a method or variable name is considered unnecessary clutter. Furthermore, variables with unclear names get lost in translation when used elsewhere. In the last example above, what does dt mean other than the Type of the variable? Consider these improvements:

public static IEnumerable<DailyTeamGoal> GetDailyTeamGoals(int teamId)
...
List<DailyTeamGoal> dailyTeamGoals = new List<DailyTeamGoal>();
...
DateTime today = DateTime.Now.Date;
...
int daysThisMonth = DateTime.DaysInMonth(today.Year, today.Month);

There's also a lot going on in this one method. Separating it out into several methods with clear names will make the code easier to understand and can break logic out into distinct testable pieces. Right now, your single method about getting the Daily Team Goals encompasses a lot of business logic that will be hard to split out when the time comes (and it will).

• Olivier Jacot-Descombes: These are good points. I changed my identifiers accordingly.
• James Wilson: Yeah I've began renaming them, think hard habits to break with the type naming.
• Olivier Jacot-Descombes: The number of public types in the .NET Framework 3.5 was over 10,000! And this number is growing with each release. This makes it impractical to find an appropriate type prefix for each type.