4
\$\begingroup\$

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
        }
    }
    
\$\endgroup\$
3
  • 1
    \$\begingroup\$ When you reference custom objects, it's hard to fully appreciate what you're doing without seeing the code for those objects. \$\endgroup\$ – user33306 Dec 10 '19 at 3:07
  • 1
    \$\begingroup\$ you'll need to provide the ArticleEmail and EmailTemplateId classes as well. So, we can have a full understanding on how your code logic works. \$\endgroup\$ – iSR5 Dec 10 '19 at 16:48
  • \$\begingroup\$ I added the ArticleEmail class as requested and updated EmailTemplateId to use int type consistently. Sorry for confusing you guys and thanks for your time and effort. \$\endgroup\$ – jakubiszon Dec 10 '19 at 19:04
3
\$\begingroup\$

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.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Yes, one especially curious effect occured when I returned IEnumerable<ArticleEmailSummary> instead ot List<ArticleEmailSummary>. It was iterated multiple times and used same grouping function. The function was not resetting its state at any point. After iterating once, which returned correct groups it kept returning one group with all emails ( because lastDate was set to the last group ). I like your idea of groupStarters - I'll experiment around it. \$\endgroup\$ – jakubiszon Dec 11 '19 at 13:52
3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ GetGroupingFunction declares lastDate in order to make it persist between invocations of the returned delegate. The delegate updates this value when it finds current item diverges from the last stored value too much ( the assignment lastDate = email.DateCreated; ) This way the lastDate is moved forward in time as the call to emailsWithOrder.GroupBy progresses over items. Your answer kind of proves my code is indeed somewhat hard to understand despite my efforts to make it clean and readable. Now I need to figure out how to make it easier. \$\endgroup\$ – jakubiszon Dec 10 '19 at 18:27
  • \$\begingroup\$ I see, my mistake. In this case it's just not useable for grouping. You have to have a function that is returning always the same, if you call it twice. And this does not, it depends on how often it was called before. It's the same rules as for a Hashcode. However, just to make this more readable, make this thing a class, with properties and methods, and take this Method as your delegate. In this way you also have access to your last date. Actually the compiler is doing exactly the same, that is called a closure (around local variables). \$\endgroup\$ – Holger Dec 10 '19 at 18:38
  • \$\begingroup\$ It is working code (actually tested) which correctly groups emails into frames of time. When we find an email which does not fit into a 30 minute frame of time starting at lastDate we update this date. The subsequent records which fall into the time frame use the lastDate as their grouping value without updating it. \$\endgroup\$ – jakubiszon Dec 10 '19 at 18:43
  • \$\begingroup\$ if you group (2001,2002,2003) it will result in 3 groups, if you group(2003,2002,2001) it will result in 1 group, if it's (2002, 2003, 2001) it will be 2 groups. Maybe this is desired behavior, but this is not what people usually consider as grouping. I'm assuming the algorithm reads the data in a linear way, but since we shall not make assumptions on the implementation of GroupBy, the result is unpredictable. It depends on the order of elements. You have ascending sorted data and want to form clusters, it's a creative idea - but it's kind of manual obfuscation to hide this behind a GroupBy. \$\endgroup\$ – Holger Dec 10 '19 at 21:01
  • \$\begingroup\$ Don't worry about the orders of dates I sort the records right before grouping. \$\endgroup\$ – jakubiszon Dec 11 '19 at 0:29
3
\$\begingroup\$

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); 
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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