3
\$\begingroup\$

I'm using Linq-to-Entities for querying, and MVC.NET and C# for coding. I'm trying to generate monthly donor registry report which needs donor count, donors who signed through web,donors who registered in current month and all this is to be display per county.

private MonthlySummaryViewModel
BuildMonthlySummaryViewModel(MonthlySummarySubModel model){

// Get the bounding datetimes for the previous and lookup month

DateTime curr = new DateTime(model.Year, model.Month, 1),                  
prev = curr.AddMonths(-1),    
next = curr.AddMonths(1);      
var monthlySummary = (from county in context.Counties.AsEnumerable()    
                      let query = context.DonorStatusActions.Where(x => x.Donor.CountyId == county.CountyId && x.ActionTakenOn >= prev && x.ActionTakenOn < next)

let data = query.GroupBy(x => x.Donor, x => x, (key, grp) => new {Item1 = key, Item2 = grp.OrderByDescending(y => y.ActionTakenOn).FirstOrDefault()})                                 
                                  .Where(x => x.Item2.IsDonor && ((model.WebSignUp != "2" && x.Item2.Donor.WebSignUp == (model.WebSignUp == "1" ? true : false)) || model.WebSignUp == "2"))  
                                  let registrations = data.Select(x => x.Item2)   
                                  let previous = registrations.Count(x => x.ActionTakenOn >= prev && x.ActionTakenOn < curr)                                                                                let current = registrations.Count(x => x.ActionTakenOn >= curr && x.ActionTakenOn < next)    
select Tuple.Create(county.Name, previous, current)).ToList();

 return new MonthlySummaryViewModel()
 {   
 Month = new System.Globalization.DateTimeFormatInfo().GetMonthName(model.Month),  
Year = model.Year,  
WebSignUp=DropDownLists.SignUpMethodList.First(m => m.Value==model.WebSignUp).Text,CountyData = monthlySummary,  
TotalPrevious = monthlySummary.Sum(x => x.Item2),  
TotalReport = monthlySummary.Sum(x => x.Item3), };}
\$\endgroup\$

closed as unclear what you're asking by Simon Forsberg, Evan Bechtol, Hosch250, Ethan Bierlein, Quill Jul 9 '15 at 3:57

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ @Neha Have you managed to test the code? Do you know whether or not it works? \$\endgroup\$ – Simon Forsberg Jul 7 '15 at 23:44
  • \$\begingroup\$ Yes i'm able to test. it does work when i have select month which has less records . but it doesn't seem to be working for huge number of records. Thanks \$\endgroup\$ – Neha Jul 7 '15 at 23:48
  • 1
    \$\begingroup\$ @Neha Are the database indexes good? As in, if you were to run the full query in plain SQL instead of Linq, would it take the same large amount of time? Slowness in a query is most often due to a bad query plan or poor indexing. \$\endgroup\$ – Phrancis Jul 8 '15 at 0:00
  • \$\begingroup\$ I'd recommend adding your database schema to your question, so that we can see what the indexes are. \$\endgroup\$ – Simon Forsberg Jul 8 '15 at 0:11
5
\$\begingroup\$

First thing to do, is to layout the query so that you can actually read it. Optimizing something you can't read is not really possible.

var monthlySummary = 
    (from county in context.Counties.AsEnumerable()
     let query = context.DonorStatusActions
                        .Where(x => x.Donor.CountyId == county.CountyId 
                                 && x.ActionTakenOn >= prev 
                                 && x.ActionTakenOn < next)
     let data = query
         .GroupBy(x => x.Donor, x => x, (key, grp) => new 
             { 
                 Item1 = key, 
                 Item2 = grp.OrderByDescending(y => y.ActionTakenOn)
                            .FirstOrDefault()
             })
         .Where(x => x.Item2.IsDonor 
                 && ((model.WebSignUp != "2" 
                     && x.Item2.Donor.WebSignUp == (model.WebSignUp == "1" ? true : false)) 
                 || model.WebSignUp == "2"))
     let registrations = data.Select(x => x.Item2)   
     let previous = registrations.Count(x => 
         x.ActionTakenOn >= prev && x.ActionTakenOn < curr)
     let current = registrations.Count(x => 
         x.ActionTakenOn >= curr && x.ActionTakenOn < next)
     select Tuple.Create(county.Name, previous, current)
    ).ToList();

Yikes!

Ok let's remove the fluff.

from county in context.Counties.AsEnumerable()

Counties is already an Enumerable - it's an IQueryable<County>, and by soft-casting it like this, you've told LINQ "don't bother translating that to T-SQL, give me all counties and let LINQ-to-Objects deal with it instead" (see this answer).

If you ran a SQL profiler when your code runs, I bet you're not going to see the T-SQL you'd expect.

Just do this instead:

from county in context.Counties

This part needs a cleanup:

 .Where(x => x.Item2.IsDonor 
         && ((model.WebSignUp != "2" 
             && x.Item2.Donor.WebSignUp == (model.WebSignUp == "1" ? true : false)) 
         || model.WebSignUp == "2"))

I'm scratching my head over this one:

x.Item2.Donor.WebSignUp == (model.WebSignUp == "1" ? true : false)

Why not just do this instead?

x.Item2.Donor.WebSignUp == (model.WebSignUp == "1")

I have to say that Donor.WebSignUp being a bool while model.WebSignUp is a string, is pretty confusing.

These magic "1" and "2" values are annoying. They have a special meaning, but nobody knows what it is. Looks like a misused string that should be an int, or even better, an enum.

Regardless, the condition is redundant and could be simplified to this:

 .Where(x => x.Item2.IsDonor && (
     model.WebSignUp == "2" || x.Item2.Donor.WebSignUp == (model.WebSignUp == "1")))

Be sure to profile the generated T-SQL and show execution plan to have SQL Server tell you if any index could speed things up; I suspect you might need one on Donors.WebSignUp, too.

\$\endgroup\$
  • \$\begingroup\$ FirstOrDefault in the context of an EF query is translated to SQL (I think basically the same SQL as Take(1)). \$\endgroup\$ – RobH Jul 8 '15 at 7:54
  • \$\begingroup\$ @RobH edited, thanks. It seems to be in the context of an EF subquery (let) though. \$\endgroup\$ – Mathieu Guindon Jul 8 '15 at 13:58
  • 1
    \$\begingroup\$ Entity Framework will translate the subquery into a SQL subquery with a Select Top (1) ... It's all magic really ;) \$\endgroup\$ – RobH Jul 8 '15 at 14:50
  • \$\begingroup\$ Thanks @Mat's Mug. I've installed Resharper which simplified my code way better. The way you told above. Per your suggestion did formatting and proper naming convention. \$\endgroup\$ – Neha Jul 9 '15 at 18:13
2
\$\begingroup\$

Naming

Naming things in programming is a hard task if one wants to do it right. Good naming can lead to many things but foremost to readable, understandable and maintainable code.

How to do the naming right ?

  • names should be meaningful so Sam the maintainer will see at first glance what the whole thing is about.
  • names shouldn't be to short nor to long.
  • names should use some guidelines either the C# naming guidelines or your own. The most important thing is to stick to the guidelines you use.
  • names should be as descriptive to the task at hand as possible.
  • names shouldn't need a comment for describing what the meaning of the is.

Now let us focus on your code, keeping the said in mind.

curr
prev
next  

neither of these names will tell you what it is about, what the purpose of the field is without wildly guessing. They could just be items in a linked list, or positions in an array ....

So you should better name the right. You could name the fields like so

DateTime currentMonth = new DateTime(model.Year, model.Month, 1);
DateTime previousMonth = currentMonth.AddMonths(-1);
DateTime nextMonth = currentMonth.AddMonths(1);  

but this wouldn't be right either, because it doesn't have to be the current month which should be queried.

A name like currentSummaryMonth would better reflect that the month the summary is for is meant.

\$\endgroup\$

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