Common Implementation for Sending mail based on time

Question

I have created one Background service that execute after every 5 minutes and this service used for sending email after 6 hour,24 hour and 48 hour for user booking order is not confirmed.

for that i have created below method and enums.

public enum Enum_AbandonedCartEmailNotifyHour
    {
        Hour06 = 6,
        Hour24 = 24,
        Hour48 = 48
    }
    public enum Enum_AbandonedCartEmailTemplateSlug
    {
        Abandonedcart6hr,
        Abandonedcart24hr,
        Abandonedcart48hr
    }

and in service timer every time i have called below method.

 public void ProcessOrders()
        {             
            var abandoned6hrsOrders = Get6hrsOrders(); //get orders
            if(abandoned6hrsOrders.Count > 0)
                this.ProcessEmail(abandoned6hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour06, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart6hr, Enum_AbandonedCartEmailNotifyStatus.Hour06);

            var abandoned24hrsOrders = Get24hrsOrders();//get orders
            if (abandoned24hrsOrders.Count > 0)
                this.ProcessEmail(abandoned24hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour24, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart24hr, Enum_AbandonedCartEmailNotifyStatus.Hour24);

            var abandoned48hrsOrders = Get48hrsOrders();//get orders
            if (abandoned48hrsOrders.Count > 0)
                this.ProcessEmail(abandoned48hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour48, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart48hr, Enum_AbandonedCartEmailNotifyStatus.Hour48);
        }

in above method i have checked validation and send email if valid and change notification status in database table.

 private void ProcessEmail(List<Order> orders, Enum_AbandonedCartEmailNotifyHour hours, 
        Enum_AbandonedCartEmailTemplateSlug abandonedCartEmailSlug, Enum_AbandonedCartEmailNotifyStatus notifyStatus)
    {
        foreach (var item in orders)
        {
            bool isValid = false;
            isValid = CheckValidOrder(item);
            Logger.Debug($"check valid order : {item.ID}");
            if (isValid)
            {
                 SendEmail(item, abandonedCartEmailSlug);
                 item.NotifyStatus= notifyStatus;
            }
            else
            {
                item.NotifyStatus= Enum_AbandonedCartEmailNotifyStatus.InValid;
            }
            item.Modified = DateTime.Now;
            _orderService.Update(item);
            _unitOfWorkAsync.SaveChanges();
        }

    }

here is method for sendemail for order

  public void SendEmail(Order order, Enum_AbandonedCartEmailTemplateSlug abandonedCartEmailSlug)
    {
        if (order != null)
        {
            var domain = this.GetDomainForBackground();
            var emailSlug = abandonedCartEmailSlug.ToString().ToLower();
            var emailTemplateQuery = _emailTemplateService.Query(x => x.Slug.ToLower() == emailSlug).Select();
            var emailTemplate = emailTemplateQuery.FirstOrDefault();

            if (emailTemplate != null)
            {
                dynamic email = new Postal.Email("Email");
                email.To = order.AspNetUserReceiver.Email;
                email.From = CacheHelper.Settings.EmailAddress;
                email.Subject = emailTemplate.Subject;
                email.Body = emailTemplate.Body;
                email.SpaceTitle = order.Listing.Title;
                email.Description = order.Listing.Description;
                if (order.Listing.Description.Length > 100)
                {
                    email.Description = order.Listing.Description.Substring(0,100) + $"... <a href='{GetListingUrl(order.ListingID)}'>Show more</a>";
                }
                email.CallbackUrl = GetCallBackUrlForBooking(domain, order.ID, order.IsHourlyBooking);
                email.FirstName = order.AspNetUserReceiver.FirstName;
                email.Category = order.Listing.Category.Name;
                var suburb = GetSuburbName(order.Listing.Latitude, order.Listing.Longitude);
                email.Suburbname = suburb;
                if (string.IsNullOrEmpty(suburb))
                {
                    suburb = GetSuburbName(order.Listing.Location);
                }
                email.LocationSearchUrl = GetUrlForLocationSearch(domain, suburb, order.Listing.CategoryID);

                if (order.IsHourlyBooking)
                {
                    var str = order.Select(x => $"</br>Number Of Space:{x.Quantity} </br> Time: {x.GetBookedDateString()}, {x.GetBookedTimeString()}").ToList();
                    var orderInformation = string.Join("</br>", str);
                    email.OrderInformation = orderInformation;
                }
                else
                {
                    var orderInformation = $@"
                                             Number of spaces : {order.Quantity}<br />
                                             Start Date: {order.FromDate.Value.ToLongDateTimeStringFormat() }<br />
                                             End Date: {order.ToDate.Value.ToLongDateTimeStringFormat()}<br />
                                            ";
                    email.OrderInformation = orderInformation;
                }

                this.SendEmailNotLoggedInternal(email);
            }
        }
    }

now my question or doubt is what if in future required any changes or adding send email after 72hours or after 3 hours.

please suggest me to best way to design this code. i have no idea for this or i have written code is good or right or any required changes.

Answer 1

First of all you do not need to prefix all your enum with Enum_, the same way you do not do it with Class_ and Interface_ or Struct_ (or Delegate_). Just drop it. That said, do you really need that enum (which BTW has not a member with 0 value then when declared but not initialized is in an invalid state)?

---

I see that you have Get6hrsOrders(), Get24hrsOrders() and Get48hrsOrders(). I see two main problems here: name is not descriptive: you're not retrieving orders placed 6 hours ago but pending (or abandoned) orders. Make it clear in the name. Second problem is that, even if I do not see that code, it seems you have some duplication. Let's try to rewrite it to:

private IEnumerable<Order> FindAbandonedOrders(TimeSpan age) { }

Few things to note:

• I'm returning IEnumerable<Order> instead of concrete implementation (I do not see Get6hrsOrders() prototype but the fact that you access a Count property tells me that probably it's a concrete collection like List<Order>). In this way you do not need to materialize in memory all abandoned orders (they may be 1, 100 or 1,000,000).
• I'm using a TimeSpan to express the age. The type implicitly communicate the content of this parameter. You may also use an int (for example int ageInHours) if you really want to.
• Do not forget to validate your inputs.

---

ProcessOrders() does not communicate what it is doing. It should be something like SendEMailRemainderForAbandonedOrders(). Storage space for source code is cheap! I assume you have different e-mail templates and the thresholds are arbitrary (and maybe subject to changes), let's abstract this:

public ProcessAbandonedOrders()
{
    foreach (var age in Ages)
        SendEMailRemainderForAbandonedOrders(age);
} 

private ProcessAbandonedOrders(TimeSpan age)
{
    var abandonedOrders = FindAbandonedOrders(age);
    if (abandonedOrders.Any())
        SendEMailRemainder(age, abandonedOrders);
}

Where Ages, to begin with, might simply be:

private static readonly TimeSpan[] Ages = new TimeSpan[]
{
    TimeSpan.FromHours(6), 
    TimeSpan.FromHours(24),
    TimeSpan.FromHours(48) 
};

---

ProcessEMail() is doing much more than its name implies. It's not just sending e-mails but also checking if an order is valid or not. If this is part of the abandoned orders handling then you should move it to ProcessAbandonedOrders(). One word about logging:

 Logger.Debug($"check valid order : {item.ID}");

This is really little helpful during debugging (or tracing in production). If an error happens in CheckOrder() (and I do not see any error handling in your posted code) then you won't ever see this message. If everything works as expected you see that this order has been evaluated but you do not see the result.

CheckOrder() is again a poor choice for a name, at least you should use IsOrderValid() (or even IsValid(Order) if applicable and clear in your context), even better if you can express the condition that makes an order valid or not (assuming that it won't change), something like IsExpired().

HOWEVER there is a VERY big BUT. Your logic seems complex because you're storing in DB a flag to determine if abandoned order has been handled or not (for the 6, 24 and 48 hours reminders). This is a bad idea (IMO) because this is business logic and you're storing an implementation detail (the enum value) of this business logic inside your database. Changes (for example if thresholds will ever change or more conditions will be added) will be hard. What I'd do? Just store when latest reminder has been sent:

private void SendEMailRemainder(TimeSpan age, IEnumerable<Order> orders)
{
    foreach (var order in orders)
    {
        SendEMailRemainder(age, order);

        order.LatestReminderSentAfter = age;
        order.Modified = DateTime.Now;
        _orderService.Update(order);
    }

    _unitOfWorkAsync.SaveChanges();    
}

Few more notes:

• You should consider to use UTC time instead of server local time (and not only in this case!) time changes (summer time) and you may send the reminder too early (or too late or no at all, according to the logic in FindAbandonedOrders()).
• I moved SaveChanges() outside the loop, I don't know the ORM you're using but there are chances that you may get better performance. Consider to do it or not.
• Order does not contain the implementation details of your business logic (all those enums) but a field used to determine when last reminder has been sent. Now your're free to change this logic without impacting database.
• I omit the second implementation for SendEMailRemainder(TimeSpan, Order) because it should be trivial (and anyway it's not visible in your original code).
• A field named _unitOfWorkAsync is highly suspicious.

---

Consider to refactor your code for testing. How do you test all this logic? Here I assume (because I do not see surrounding code) that:

• Code for sending an e-mail is separate from code to read and build the content, in this way you can mock the service for sending for testing purposes. You do not want to send 1000 e-mails each time your test suite runs.
• Code for retrieving abandoned orders (and to update DB) can be mocked, in this way you can test this logic in isolation, without the need to setup a database (which is slow but viable in your development machine but a pain in your build machine where all unit tests will also run). Database integration is a separate issue you will tests in integrations tests (not unit testing).

Answer 2

Comments On Code

1. Enum_AbandonedCartEmailNotifyHour hours is passed into ProcessEmail but it is not actually used. Get rid of the paramters that you don't actually need!

2. Personally I like to avoid keeping the state of a class if I can avoid it. You have a bool isValid (looks like a field or a property) within the Order class, but you are using the CheckValidOrder() method. This CheckValidOrder method should be a method within the Order class (IMO) - so unless you have a very good reason, you don't need to expose the isValid bool/property to the outside world?

3. If using a polymorphic approach you will not need to pass in the abandonedCartEmailSlug - you can simply put in the code you have in your current SendEmail method (which depends on the slug enum) into the relevant sub class of the order. I hope this is making sense.

4. You'll have to create different subclasses of orders: e.g. 24HourOrder and 48HourOrder etc (the class can't start with a number of course) and it will have to be a subclass of the abstract Order class.

The Goal

I think that the entire code can be turned into this - so we shall try for it:

    public void ProcessOrders()
    {
        List<Order> orders = Order.getAllOrders();

        foreach (Order order in orders)
        {
            order.SendEmailIfValid();
        }           
    }

The Order Class:

    public abstract class Order
    {
        public static List<Order> getAllOrders()
        {
            return new List<Order>() { };
        }

        public DateTime Modified { get; set; }

        private bool IsValid() // i.e. the CheckValidOrder method.
        {
            throw new NotImplementedException();
        }

        protected abstract void SendEmail();

        public void SendEmailIfValid()
        {
            if (IsValid())
            {
                SendEmail();
                //  there's no need to notify the status
            }
        }

        protected class SixHourOrder : Order
        {
            protected override void SendEmail()
            {
                // you don't need to pass in the abnadonedcartemailslug enum in here
                // anymore. Simply copy the relevant portions of your current SendEmail method
                // into here. The other portions you need to create another subclass of Order
                // in order to handle that.

                throw new NotImplementedException();
            }
        }
    }

Comments on your code:

    ///// <summary>
    /////  // Don't need this anymore because you have polymorphic classes which handle this.
    ///// </summary>
    //public enum Enum_AbandonedCartEmailNotifyHour
    //{
    //    Hour06 = 6,
    //    Hour24 = 24,
    //    Hour48 = 48
    //}

    ///// <summary>
    /////  // Don't need this anymore because you have different polymorphic classes which handle this.
    ///// </summary>
    //public enum Enum_AbandonedCartEmailTemplateSlug
    //{
    //    Abandonedcart6hr,
    //    Abandonedcart24hr,
    //    Abandonedcart48hr
    //}

    /// <summary>
    /// This class can basically turn into
    /// var orders = getOrders();
    /// for each order in orders ---> order.processEmail()
    /// </summary>
    //public void ProcessOrders()
    //{
    //    var abandoned6hrsOrders = Get6hrsOrders(); //get orders
    //    if (abandoned6hrsOrders.Count > 0)
    //        this.ProcessEmail(abandoned6hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour06, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart6hr, Enum_AbandonedCartEmailNotifyStatus.Hour06);

    //    var abandoned24hrsOrders = Get24hrsOrders();//get orders
    //    if (abandoned24hrsOrders.Count > 0)
    //        this.ProcessEmail(abandoned24hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour24, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart24hr, Enum_AbandonedCartEmailNotifyStatus.Hour24);

    //    var abandoned48hrsOrders = Get48hrsOrders();//get orders
    //    if (abandoned48hrsOrders.Count > 0)
    //        this.ProcessEmail(abandoned48hrsOrders, Enum_AbandonedCartEmailNotifyHour.Hour48, Enum_AbandonedCartEmailTemplateSlug.Abandonedcart48hr, Enum_AbandonedCartEmailNotifyStatus.Hour48);
    //}

    //private void ProcessEmail(List<Mailer> orders, Enum_AbandonedCartEmailNotifyHour hours, Enum_AbandonedCartEmailTemplateSlug abandonedCartEmailSlug, Enum_AbandonedCartEmailNotifyStatus notifyStatus)
    //{
    //    foreach (var item in orders)
    //    {
    //        bool isValid = false;
    //        isValid = CheckValidOrder(item);

    //        if (isValid)
    //        {
    //            SendEmail(item, abandonedCartEmailSlug);
    //            item.NotifyStatus = notifyStatus;
    //        }
    //        else
    //        {
    //            item.NotifyStatus = Enum_AbandonedCartEmailNotifyStatus.InValid;
    //        }
    //        item.Modified = DateTime.Now;
    //        _orderService.Update(item);
    //        _unitOfWorkAsync.SaveChanges();
    //    }
    //}

I hope this is making some sort of sense, or at the very least giving you some ideas.