CODEWITHSUNDEEP
c#linqlinq-to-sqlrefactoring
A Salcedo
A Salcedo

Reputation: 6468

How can I make this LINQ query cleaner?

I have recently written a LINQ query to get a Dictionary containing the last 6 month's placement amounts.

It is returning a Dictionary of Month string - Decimal Amount pairs.

It seems kind of cludgey. Any of you LINQ masters out there able to help me refactor this to make a bit cleaner?

/// <summary>
/// Gets the last 6 months of Placement History totalled by Month 
/// for all Agencies
/// </summary>
/// <returns></returns>
public Dictionary<string, decimal> getRecentPlacementHistory()
{
    var placementHistoryByMonth = new Dictionary<string, decimal>();

    using (DemoLinqDataContext db = new DemoLinqDataContext())
    {
        for (int i = 0; i < 6; i++)
        {
            Decimal monthTotal = 
              (from a in db.Accounts
               where 
                 (a.Date_Assigned.Value.Month == DateTime.Now.AddMonths(-i).Month &&
                  a.Date_Assigned.Value.Year == DateTime.Now.AddMonths(-i).Month)
               select a.Amount_Assigned).Sum();
            String currentMonth = DateTime.Now.AddMonths(-i).ToString("MMM");

            placementHistoryByMonth.Add(currentMonth, monthTotal);
        }
        return placementHistoryByMonth;
    }
}

Upvotes: 1

Views: 715

Answers (3)

tghw
tghw

Reputation: 25303

Use Group By

DateTime now = DateTime.Now;
DateTime thisMonth = new DateTime(now.Year, now.Month, 1);

Dictionary<string, decimal> dict;
using (DemoLinqDataContext db = new DemoLinqDataContext())
{
    var monthlyTotal = from a in db.Accounts
        where a.Date_Assigned > thisMonth.AddMonths(-6)
        group a by new {a.Date_Assigned.Year, a.Date_Assigned.Month} into g
        select new {Month = new DateTime(g.Key.Year, g.Key.Month, 1),
                    Total = g.Sum(a=>a.Amount_Assigned)};

    dict = monthlyTotal.OrderBy(p => p.Month).ToDictionary(n => n.Month.ToString("MMM"), n => n.Total);
}

No loop needed!

Upvotes: 2

sgmoore
sgmoore

Reputation: 16067

If you are not worried about missing months with no data,then I had a similar problem where I did the following : (translated to your variables)

  DateTime startPeriod = 
     new DateTime(DateTime.Now.Year, DateTime.Now.Month, 1).AddMonths(-6);

  var query1 = from a in db.Accounts where a.Date_Assigned >= startPeriod
 group a by new { a.Date_Assigned.Year  ,a.Date_Assigned.Month  } into result
 select new
 {
     dt = new DateTime( result.Key.Year, result.Key.Month , 1),
     MonthTotal = result.Sum(i => i.Amount_Assigned)
 } ;             

  var dict = query1.OrderBy(p=> p.dt).ToDictionary(n => n.Dt.ToString("MMM") , n => n.MonthTotal );

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1500055

First problem:

where (a.Date_Assigned.Value.Month == DateTime.Now.AddMonths(-i).Month &&
       a.Date_Assigned.Value.Year == DateTime.Now.AddMonths(-i).Month)

Shouldn't the latter expression end with .Year rather than .Month? Surely you'll rarely get a year with a value of 1-12...

I would extract the idea of the "current month" as you're using it a lot. Note that you're also taking the current time multiple times, which could give odd results if it runs at midnight at the end of a month...

public Dictionary<string, decimal> getRecentPlacementHistory()
{
    var placementHistoryByMonth = new Dictionary<string, decimal>();
    using (DemoLinqDataContext db = new DemoLinqDataContext())
    {
        DateTime now = DateTime.Now;

        for (int i = 0; i < 6; i++)
        {
            DateTime selectedDate = now.AddMonths(-i);

            Decimal monthTotal = 
               (from a in db.Accounts
                where (a.Date_Assigned.Value.Month == selectedDate.Month &&
                       a.Date_Assigned.Value.Year == selectedDate.Year)
                select a.Amount_Assigned).Sum();

            placementHistoryByMonth.Add(selectedDate.ToString("MMM"),
                                        monthTotal);
        }
        return placementHistoryByMonth;
    }
}

I realise it's probably the loop that you were trying to get rid of. You could try working out the upper and lower bounds of the dates for the whole lot, then grouping by the year/month of a.Date_Assigned within the relevant bounds. It won't be much prettier though, to be honest. Mind you, that would only be one query to the database, if you could pull it off.

Upvotes: 5

Related Questions