Mailsending application for e-store

Question

This is a fictional email sending program for a e-store I've done for practice purposes. EmailSenderProgram is a program sending emails to customers. Currently it sends two types of email: "welcome" and "please come back" email. It's supposed to run daily and write a debug log each day if it worked or not. Posted earlier, updated and now back for more. Revision: 2

namespace EmailSender
    {
        public class SendingMail
        {
            private static List<Customer> customers = DataLayer.ListCustomers();
            public static void SendEmail(string recipients, string from, string subject, string body)
            {

            foreach (Customer customer in customers)
            {
                var mailMessage = new MailMessage();
                mailMessage.To.Add(string.Join(",", recipients));
                mailMessage.From = new MailAddress(from);
                mailMessage.Subject = subject;
                mailMessage.Body = body;
            }
        }

        public static bool SendWelcomeMail()
        {
            string welcomeSubject = "Welcome as a new customer at Company!";
            string ourEmailAddress = "[email protected]";
            string bodyTemplate = "Hi {0}<br>We would like to welcome you as customer on our site!<br><br>Best Regards,<br>lcompany Team";

            try
            {
                foreach (var customer in DataLayer.ListCustomers())
                {
                    if (customer.CreatedDateTime >= DateTime.Now.AddDays(-1))
                    {
                        SendEmail(customer.Email, ourEmailAddress, welcomeSubject, string.Format(bodyTemplate, customer.Email));

#if DEBUG
                        // In debug mode, send email to 
                        Console.WriteLine("Send mail to:" + customer.Email);
#else
    //Create a SmtpClient to our smtphost: yoursmtphost
                    System.Net.Mail.SmtpClient smtp = new System.Net.Mail.SmtpClient("yoursmtphost");
                    Send mail
                    smtp.Send(mail);
#endif
                    }
                }
                return true;
                //All mails are sent! Success!
            }
            catch (Exception)
            {
                //Something went wrong :(
                return false;
            }
        }

        public static bool SendComeBackEmail(string voucherCode)
        {
            string comeBackSubject = "Welcome as a new customer at Company!";
            string ourEmailAddress = "[email protected]";
            string bodyTemplate = "Hi {0}" +
                                 "<br>We miss you as a customer. Our shop is filled with nice products. Here is a voucher that gives you 50 kr to shop for." +
                                 "<br>Voucher: " + voucherCode +
                                 "<br><br>Best Regards,<br>company Team";

            List<Customer> customers = DataLayer.ListCustomers();
            List<Order> customerOrders = DataLayer.ListOrders();

            try
            {
                foreach (var customer in DataLayer.ListCustomers())
                {
                    bool Send = false;
                    foreach (Order order in customerOrders)
                        if (customer.Email == order.CustomerEmail && order.OrderDatetime <= DateTime.Now.AddMonths(-1))
                        {
                            //We send email to that customer
                            Send = true;
                        }

                    //Send if customer hasn't put order
                    if (Send == true)
                    {
                        SendEmail(customer.Email, ourEmailAddress, comeBackSubject, string.Format(bodyTemplate, customer.Email));
#if DEBUG

                        Console.WriteLine("Send mail to:" + customer.Email);
#else
    //Create a SmtpClient to our smtphost: yoursmtphost
                    System.Net.Mail.SmtpClient smtp = new System.Net.Mail.SmtpClient("yoursmtphost");
                    Send mail
                    smtp.Send(m);
#endif
                    }
                }
                //All mails are sent! Success!
                return true;
            }
            catch (Exception)
            {
                //Something went wrong :(
                return false;
            }
        }
    }
}

Main class program

internal class Program
    {
        private static void Main(string[] args)
        {
            Console.WriteLine("Send Welcomemail");
            bool welcomeMailSuccess = SendingMail.SendWelcomeMail();
#if DEBUG
            //Debug mode, always send Comeback mail
            Console.WriteLine("Send Comebackmail");
            bool comeBackEmailSuccess = SendingMail.SendComeBackEmail("Thisisavouchercode");
#else
            //Every Sunday run Comeback mail
            if (DateTime.Now.DayOfWeek.Equals(DayOfWeek.Monday))
            {
                Console.WriteLine("Send Comebackmail");
                comebackEmailSuccess = SendComeBackEmail("Thisisavouchercode");
            }
#endif

            //Check if the sending went OK
            if (comeBackEmailSuccess == true)
            {
                Console.WriteLine("All mails are sent, I hope...");
            }
            //Check if the sending was not going well...
            if (comeBackEmailSuccess == false)
            {
                Console.WriteLine("Oops, something went wrong when sending mail (I think...)");
            }
            Console.ReadKey();
        }
    }

Lastly a datalayer class

public class Customer
    {
        public string Email { get; set; }
        public DateTime CreatedDateTime { get; set; }
    }

    public class Order
    {
        public string CustomerEmail { get; set; }
        public DateTime OrderDatetime { get; set; }
    }

    class DataLayer
    {
        /// <summary>
        /// Mockup method for all customers
        /// </summary>
        public static List<Customer> ListCustomers()
        {
            return new List<Customer>()
                       {
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddHours(-7)}, 
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddDays(-1)}, 
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddMonths(-6)}, 
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddMonths(-1)}, 
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddMonths(-2)},
                           new Customer(){Email = "[email protected]", CreatedDateTime = DateTime.Now.AddDays(-5)}
                       };
        }

        /// <summary>
        /// Mockup method for listing all orders
        /// </summary>
        public static List<Order> ListOrders()
        {
            return new List<Order>()
                       {
                           new Order(){CustomerEmail = "[email protected]", OrderDatetime = DateTime.Now.AddMonths(-6)}, 
                           new Order(){CustomerEmail = "[email protected]", OrderDatetime = DateTime.Now.AddMonths(-2)},  
                           new Order(){CustomerEmail = "[email protected]", OrderDatetime = DateTime.Now.AddDays(-2)}
                       };
        }

This question is a follow-up to: E-store automatic email send, to be run daily

Answer 1

First of all you should remove code duplication. SendWelcomeMail() and SendComeBackEmail() do almost the same task, you shouldn't repeat that big bunch of code. Let's move the common implementation to a separate private method.

private void Send(IEnumerable<Customer> customers, string body) { }

Now you can simplify those functions:

public void SendWelcomeEmails()
{
    Send(GetNewCustomers(),
        Properties.Resources.WelcomeEmailTemplate);
}

public void SendComeBackEmail()
{
    Send(GetCustomersWithourRecentOrders(),
        Properties.Resources.ComeBackEmailTemplate);
}

Few things to note:

• Each method is as small as possible and descriptive method names help me to understand what it is doing reading as little as possible. This is important when you come back to your code after few months and you need to learn what it is actually doing (not to mention that plain English also helps to spot out bugs).
• I do not use return flags which may be ignored by the caller (as you do in your code, BTW). Also do not catch exception you can't handle (if something went wrong I'd like to know WHAT).
• Avoid to hard-code HTML in your C# code. Move it to a resource file (by now).

We now need to move that preprocessor logic out of our code. Why? Because it makes unit testing a pain. First of all let's introduce an interface IMailSender:

interface IMailSender
{
    void Send(string from, string to, string body);
}

Now let's implement two concrete classes:

sealed class NullMailSender : IMailSender
{
    void Send(string from, string to, string body)
    {
        // Empty
    }
}

sealed class SmtpMailSender : IMailSender
{
    void Send(string from, string to, string body) 
    {
         var smtp = new SmtpClient("yoursmtphost");
         // ...  
    }
}

Now we can introduce a property in our EmailSending class (which can't and should be static any more):

public IMailSender Sender { get; set; }

That ugly #if DEBUG logic is now gone from our business logic and at least we can test it.

Let's do exactly the same thing for the list of customers. You probably don't want to hit the database during your tests (because to prepare database before/after each test is slow and, for small applications, you'd better do unit testing without DB (which, of course, must be tested in your integration/system tests).

Where exception handling is? In the outer Main() code because you actually can't do anything but ignore the error.

Can we do better? As you should know to send marketing e-mails isn't an easy task and I won't repeat the reasons here. We may, at least, make it little bit more robust. Pseudo-code here:

private void Send(IEnumerable<Customer> customers, string body)
{
    for (int i=0; i <= NumberOfRetriesOnError; ++i)
    {
        try
        {
            // Your code...

            return; // Break the loop
        }
        catch (SmtpException) when (i < NumberOfRetriesOnError)
        {
            Thread.Sleep((i + 1) * DelayOnError);
        }
    }
}

Mail server may temporary refuse to accept an e-mail or network-related errors may occur, at least we wait few seconds and try again few times. What's bad there? We still stop sending e-mails if sending of one of them failed. We should keep a list of errors:

public List<string> Errors { get; } = new List<string>();

And in method:

private void Send(IEnumerable<Customer> customers, string body)
{
    foreach (var customer in customers)
    {
         for (int i=0; i <= NumberOfRetriesOnError; ++i)
         {
             try
             {
                 Sender.Send(...);

                 return;
             }
             catch (SmtpException e)
             {
                 if (i < NumberOfRetriesOnError)
                     Thread.Sleep((i + 1) * DelayOnError);
                 else
                     Errors.Add(e.Message); // Include customer name...
             }
        }
    }
}

Note that this code is already a good candidate for more refactoring, just leave it there for now. Calling code will then be:

sender.Errors.Clear();
sender.SendComeBackEmail("Thisisavouchercode");
if (sender.Errors.Any())
    Console.WriteLine("All mails are sent, I hope...");

Of course there is MUCH MUCH more that can be done here but I guess you see what I mean.

---

Let me stress once again: do not use preprocessor logic #if DEBUG to alter your program flow unless you're leaving out debug code (for example to print the e-mail before sending). This makes your code hard to test and it's hard to see what it is doing (not to mention that if you run your application in release mode during development...you will send few thousands e-mail to your customers...)

---

To put some suggestions all together, the skeleton of you class may be:

sealed class SendingMail
{
    public List<string> Errors { get; } = new List<string>();

    public IEnumerable<Customer> Customers { get; set; }

    public IEnumerable<Order> Orders { get; set; }

    public IEMailSender { get; set; }

    public void SendWelcomeEmails()
    {
        Send(GetNewCustomers(),
            Properties.Resources.WelcomeEmailTemplate);
    }

    public void SendComeBackEmail()
    {
        Send(GetCustomersWithoutRecentOrders(),
            Properties.Resources.ComeBackEmailTemplate);
    }

    private IEnumerable<Customer> GetNewCustomers()
    {
        var yesterday = DateTime.Now.Date.AddDays(-1);
        return Customers.Where(x => x.CreatedDateTime >= yesterday);
    }

    private IEnumerable<Customer> GetCustomersWithoutRecentOrders()
    {
        var oneMonthAgo = DateTime.Now.Date.AddMonths(-1);

        // Note that this code may be greatly simplified and
        // probably a JOIN made by the ORM you're using will also
        // make it much faster and less resource consuming.
        return Customers.Where(c => {
            var latestOrder = Orders
                .Where(o => o.CustomerEmail == c.EMail)
                .OrderByDescending(o => o.OrderDatetime)
                .FirstOrDefault();

            return latestOrder != null
                && latestOrder.OrderDateTime < oneMonthAgo;
        });
    }

    private void Send(IEnumerable<Customer> customers, string body)
    {
        foreach (var customer in customers)
        {
             for (int i=0; i <= NumberOfRetriesOnError; ++i)
             {
                 try
                 {
                     Sender.Send(...);

                     return;
                 }
                 catch (SmtpException e)
                 {
                     if (i < NumberOfRetriesOnError)
                         Thread.Sleep((i + 1) * DelayOnError);
                     else
                         Errors.Add(e.Message); // Include customer name...
                 }
            }
        }
    }
}