C# + Linq - grouping objects by frames of time found in the data

Question

A bit of background. In the application I work on there are emails sent to notify users about articles they need to review. Each email+recipient pair is stored in database and may be assigned a different creation date ( called DateCreated ) due to emails being generated over some time.

I needed to calculate how many times the emails were sent and how many recipients were there on each turn. For this reason I grouped emails which come in quick succession into groups. I used a TimeSpan of 30 minutes after each "first" email in a group. In order to do it I needed to store the dates defining groups "somewhere". I used a separate method to wrap my grouping function. The TimeSpan I used is safe, mailing ever only takes up to 2 minutes. Also separate mailings usually happen on separate dates.

My questions here are - Is this easy enough to understand? Could I code it simpler? Any other advice is also welcome.

And yes - this should be fixed in the database, there should be a mailing table there but there isn't and I cannot achieve it easily at the moment.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace UserCommunication
{
    public class ArticleEmailSummary
    {
        public int ArticleId { get; }
        public int EmailTemplateId { get; }
        public DateTime DateCreated { get; }
        public int RecipientCount { get; }

        private ArticleEmailSummary(IGrouping<DateTime, ArticleEmail> grouping)
        {
            ArticleId = grouping.First().ArticleId;
            EmailTemplateId = grouping.First().EmailTemplateId;
            DateCreated = grouping.Key;
            RecipientCount = grouping.Count();
        }

        public static List<ArticleEmailSummary> List(int ArticleId, int emailTemplateId)
        {
            // get email list from db
            List<ArticleEmail> emails = ArticleEmail.List(ArticleId, emailTemplateId);

            // order it ascending by creation date
            IOrderedEnumerable<ArticleEmail> emailsWithOrder = emails.OrderBy(e => e.DateCreated);

            // group into sublists, each group cachet up to X minutes of emails occuring after the first one
            IEnumerable<IGrouping<DateTime, ArticleEmail>> groups = emailsWithOrder.GroupBy(GetGroupingFunction());

            // convert groupings to summaries and return
            return groups.Select(grouping => new ArticleEmailSummary(grouping)).ToList();
        }

        private static Func<ArticleEmail, DateTime> GetGroupingFunction()
        {
            // I am curious of other dev's opinion on this method :)

            TimeSpan maxMailingTime = TimeSpan.FromMinutes(30);
            DateTime lastDate = DateTime.MinValue;

            return delegate (ArticleEmail email)
            {
                if (email.DateCreated - lastDate > maxMailingTime)
                {
                    // the mailing was not sent within maxMailingTime from the start of first email in series
                    // we will update the lastDate and this will group the emails into a separate group
                    lastDate = email.DateCreated;
                }

                // now we return the value which groups our emails
                return lastDate;
            };
        }
    }
}

Edits:

• changed emailTemplateId to be of type int - it was just an enum ( with values such as "articleForApproval", "expiringArticle", etc. ), my bad, this must have been confusing indeed
• ArticleEmail.List(...) simply returns List<ArticleEmail> that match the specified type of email and specific article

• as requested here, is the ArticleEmail class, a bit simplified for the purpose of this question:

  public class ArticleEmail {
      public int ArticleId { get; }
      public int EmailTemplateId { get; }
      public DateTime DateCreated { get; }
      public int RecipientId { get; }
  
      public static List<ArticleEmail> List( int articleId, int emailTemplateId ) {
          // return instances from db
      }
  }
  

Answer 1

I would advise against the use of GroupBy in this case. It seems dangerous to do so and is very hard to understand. I would recommend something like this:

public static bool WasSendShortlyAfter(this ArticleEmail email, ArticleEmail start)
{
    var maxMailingTime = TimeSpan.FromMinutes(30);
    var diff = email.DateCreate - start.DateCreate;

    return diff >= TimeSpan.Zero && diff <= maxMailingTime;
}

and then you can do this:

var groupStarters = emails
    .Where(potentialStart => !emails
        .Any(email => potentialStart.WasSendShortlyAfter(email)));
var groups = groupStarters
    .Select(start => emails.Where(email => email.WasSendShortlyAfter(start));

Remarks:

• Alternatively WasSendShortlyAfter could be a member method of ArticleEmail.
• It would be nice if maxMailingTime would be configurable in some way.
• Not sure how performant my solution is. If the operation is performance critical there might be better options.

Answer 2

This

private static Func<ArticleEmail, DateTime> GetGroupingFunction()
    {
        // I am curious of other dev's opinion on this method :)

        TimeSpan maxMailingTime = TimeSpan.FromMinutes(30);
        DateTime lastDate = DateTime.MinValue;

        return delegate (ArticleEmail email)
        {
            if (email.DateCreated - lastDate > maxMailingTime)
            {
                // the mailing was not sent within maxMailingTime from the start of first email in series
                // we will update the lastDate and this will group the emails into a separate group
                lastDate = email.DateCreated;
            }

            // now we return the value which groups our emails
            return lastDate;
        };
    }

I would just write as:

 private static readonly TimeSpan maxMailingTime = TimeSpan.FromMinutes(30);
 private static readonly DateTime lastDate = DateTime.MinValue;
 private static readonly Func<ArticleEmail, DateTime> GroupingFunction = 
    email => (email.DateCreated - lastDate > maxMailingTime) ? email.DateCreated : lastDate; 

To initialize a constant TimeSpan you can just use

new TimeSpan(30, 0, 0, 0);

that is a constant of 30 days. Your way with FromMinutes is intended to compute an floating point, like 3.76 days into a TimeSpan and is much slower and inaccurate, since it's floating point operations, not integer.

While I'm not sure what you want with subtracting DateTime.MinValue, which is actually 0 or 1/1/1. Do you just want to convert a DateTime to a TimeSpan by computing the TimeSpan from 1/1/1 up to the Date ? Creative, but actually useless.

You can get the same with

  const long maxMailingTicks = 30*60*1000000L; // 30minutes
  if (email.DateCreated.Ticks > maxMailingTicks)

Let's think on. How can a Date ever be smaller than 30minutes ? Do you really want to deal or Check on Dates in the first 30minutes of the life of Jesus Christ ?

So where do we end up ?

 private static readonly Func<ArticleEmail, DateTime> = email => email.DateCreated;

If we assume, the email was not send to notify of the birth of Jesus, there is nothing left to do.

But if you want to keep your test:

 private static readonly Func<ArticleEmail, DateTime> GroupingFunction = email => 
    email.DateCreated.Ticks > maxMailingTicks ? email.DateCreated : DateTime.MinValue;

To Have this possibly working on Linq to SQL also, you rather use Expression<Func<ArticleEMail, DateTime>> as the Type of the function. There is nothing else to do than exchanging the return type/type of my field. When dealing with Dates and SQL you have to use some special functions sometimes, there could be a revision necessary, but with just "Func", its definitly impossible ever to become SQL at all. This would definitly work on SQL:

 private static readonly Expression<Func<ArticleEmail, DateTime>> = email => email.DateCreated;

Actually your code was too long. I just dealed with the last 10 lines.

Answer 3

This combines the GroupBy and the following Select into Once. It should be easier to understand and much more performant, cause it does not involve the LINQ-GroupBy algorithm. It's your algorithm that does the grouping.

private static IEnumerable<ArticleEmailSummary> 
MakeGroups(IOrderedEnumerable<ArticleEmail> emails)
{
    TimeSpan maxMailingTime = new TimeSpan(30, 0, 0, 0); // could be a static readonly field also
    DateTime lastDate = DateTime.MinValue;

   var list = new List<ArticleEMail>();

    foreach(var email in emails)
    {
        if (email.DateCreated - lastDate > maxMailingTime)
        {
        // the mailing was not sent within maxMailingTime from the start of first email in series
        // we will update the lastDate and this will group the emails into a separate group

            if (list.Count > 0)
            {
                yield return new ArticleEMailSummary(list); 
                list = new List<ArticleEMail>();
            }

            lastDate = email.DateCreated;
        }
        list.Add(email);
    }
    Debug.Assert(list.Count > 0);
    yield return new ArticleEMailSummary(list); 
}