6
\$\begingroup\$

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. Would love some rice and roses where is needed. This is the entire code.

First we have the main method.

private static void Main(string[] args)
{
    Sender.Errors.Clear();
    Sender.SendWelcomeEmails();
    SendingMail.SendComeBackEmail("Thisisavouchercode");
    if (sender.Errors.Any())
        Console.WriteLine("All mails are sent, I hope...");
}  

Following with the DataLayerClass

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 = "mail1@mail.com", CreatedDateTime = DateTime.Now.AddHours(-7)}, 
                       new Customer(){Email = "mail2@mail.com", CreatedDateTime = DateTime.Now.AddDays(-1)}, 
                       new Customer(){Email = "mail3@mail.com", CreatedDateTime = DateTime.Now.AddMonths(-6)}, 
                       new Customer(){Email = "mail4@mail.com", CreatedDateTime = DateTime.Now.AddMonths(-1)}, 
                       new Customer(){Email = "mail5@mail.com", CreatedDateTime = DateTime.Now.AddMonths(-2)},
                       new Customer(){Email = "mail6@mail.com", 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 = "mail3@mail.com", OrderDatetime = DateTime.Now.AddMonths(-6)}, 
                       new Order(){CustomerEmail = "mail5@mail.com", OrderDatetime = DateTime.Now.AddMonths(-2)},  
                       new Order(){CustomerEmail = "mail6@mail.com", OrderDatetime = DateTime.Now.AddDays(-2)}
                   };
    }
}

InterFace

public interface IMailSender
{
    void Send(string from, string to, string body);
}
sealed class NullMailSender : IMailSender
{
    void IMailSender.Send(string from, string to, string body)
    {

    }
}

sealed class SmtpMailSender : IMailSender
{
    void IMailSender.Send(string from, string to, string body)
    {
        System.Net.Mail.MailMessage mail = new System.Net.Mail.MailMessage();
        mail.From = new System.Net.Mail.MailAddress(from);
        System.Net.Mail.SmtpClient smtp = new System.Net.Mail.SmtpClient("yoursmtphost");
        mail.To.Add(to);
        mail.Body = body;
        smtp.Send(mail);

    }
}     

This is the mail sending part, In the SendWelcome Email and SendComeback Email I've connected them to a text file with the body of the emails to avoid html in code.

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

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

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

    public IMailSender Sender { get; set; }

    public void SendWelcomeEmails()
    {
        var template = Resources.WelcomeEmailTemplate;
        Send(GetNewCustomers(), Resources.WelcomeEmailTemplate);
    }

    public void SendComeBackEmail()
    {
        var template = Resources.WelcomeEmailTemplate;
        var emailContent = String.Format(template);
        Send(GetCustomersWithoutRecentOrders(), 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);

        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 (Customer customer in customers)
            Send(customer, body);
    }
    private void Send(Customer customer, string body)
    {
        MailMessage message = GenerateMessage(customer, body);
        int NumberOfRetriesOnError = 2;
        int DelayOnError = 10;
        for (int i = 0; i <= NumberOfRetriesOnError; ++i)
        {
            try
            {
                Sender.Send(GenerateMessage);
                return;
            }
            catch (SmtpException e)
            {
                if (i < NumberOfRetriesOnError)
                    Thread.Sleep((i + 1) * DelayOnError);
                else
                    Errors.Add(e.Message + customer.Email); // Include customeremail
            }
        }
    }
    private MailMessage GenerateMessage(Customer customer, string body)
    {
        string subject = "...";
        string from = "...";
        string to = customer.Email;
        MailMessage retVal = new MailMessage(from, to, subject, body);
        return retVal;
    }
}

}

\$\endgroup\$
  • 2
    \$\begingroup\$ What was wrong with the other question? You could have edited it. \$\endgroup\$ – t3chb0t Aug 16 '17 at 7:19
  • 6
    \$\begingroup\$ It was only 16 hours old... give it a few days at least ;-) \$\endgroup\$ – t3chb0t Aug 16 '17 at 7:22
  • 2
    \$\begingroup\$ There is no overload of SendComeBackEmail which takes a string. What is Sender? Your Main doesn't seem to match the rest of your code. \$\endgroup\$ – RobH Aug 16 '17 at 9:39
  • 2
    \$\begingroup\$ This is tagged "unit-testing"... did you mean to include some unit tests too? \$\endgroup\$ – Ben Aaronson Aug 16 '17 at 10:16
  • 4
    \$\begingroup\$ Also, don't use Thread.Sleep for retry please \$\endgroup\$ – Milney Aug 16 '17 at 13:21
4
\$\begingroup\$

DataLayer.ListCustomers() should not be static for the usual reasons, not it's a mock used for testing purposes but you want to, ideally, switch between implementations without changing your model code. The same is true for DataLayer.ListOrders(). You can use a base abstract class or an interface. Also those methods shouldn't contain List, the data structure you use to accomodate them is an implementation detail which may (and should) change (you will see this when you will add a true ORM to your application). Class itself has a little bit weird name. Probably Customer, Order and DataLayer should be sealed.

In SendWelcomeEmail() (note that name is singular because you're sending a single e-mail) you're not customizing the template. The same is true for SendComeBackEmail() where you use it as parameter for String.Format() but you do not provide any...usable argument. You're also mixing welcome/come back content there...

For simplicity let me assume we're working with plain text templates.

Let's say you have a resource file with this text:

Welcome <CLIENT NAME>, please buy something from <COMPANY NAME>

Text between < and > is obviously a placeholder for something external to the template. It's now obvious that template is determined by SendWelcomeEmail() and SendComeBackEmail() but substitution must be performed by Send() because it's the one working with a specific customer (which we need to extract specific information).

Let's go with a first quick and very naive implementation using String.Format(). Use your template as formatting string:

Welcome {0}, please buy something from {1}

Code to use it:

private void SendWelcomeEmail()
{
    Send(GetNewCustomers(), Resources.WelcomeEmailTemplate);
}

And:

private void Send(Customer customer, string template)
{
    var body = String.Format(template, customer.FullName, "ACME Corporation");

    // ...
}

Now it works, it's not exactly an extensible approach and it's terrible when you will need to add anything else but...it's simple. Can we do better? Of course we can use and handmade text templating solution, we can use T4 text transformations or...use, for example, Handlebars.NET:

private void Send(Customer customer, string template)
{
        var compiledTemplate = 
        HandlebarsDotNet.Handlebars.Compile(template);
        var body = template(new {
        {
            CompanyName = "ACME Corporation",
            Customer = customer
        });

    // ...
}

Your text template will then be:

Welcome {{Customer.FullName}}, please buy something from {{CompanyName}}

That's much easier to extend, less error-prone and localization-friendly. We can do even better but we don't need to go that far now. Few more notes:

  • NumberOfRetriesOnError and DelayOnError should be private const class fields, not local variables.
  • Send() method has unneeded knowledge of the actual IMailSender implementation, it renders the interface itself half useless. Callers of IMailSender.Send() should have no knowledge at all about it! No error handling in that method and the message itself shouldn't be created inside MailSender.

What does it mean? Send() should probably be something like this (for now):

private void Send(IEnumerable<Customer> customers, string title, string template)
{
    if (Sender == null)
        throw new InvalidOperationException("Cannot send...");

    Errors.Clear();

    foreach (var customer in customers)
    {
        string body = String.Format(template, customer.Name);
        Sender.Send(Errors, OurEmailAddress, customer.Email, title, body);
    }
}

And one implementation of IMailSender.Send():

public void Send(IList<string> errors, string from, string to, string title, string body)
{
    for (int i = 0; i <= NumberOfRetriesOnError; ++i)
    {
        try
        {
            using (var smtp = new SmtpClient("yoursmtphost"))
            {
                var mail = new MailMessage();
                mail.From = new MailAddress(from);
                mail.To.Add(to);
                mail.Body = body;

                smtp.Send(mail);
            }

            continue;
        }
        catch (SmtpException e)
        {
            if (i < NumberOfRetriesOnError)
                Thread.Sleep((i + 1) * DelayOnError);
            else
                errors.Add($"{to}: {e.Message}");
        }
    }

}

Also do not forget to check that Orders and Customers have a value before you use them (again you might throw InvalidOperationException). To pass a IList isn't an optimal solution but I think you may start with it, maybe in future you will need to create Issue, IssueCollection and pass a pass a reference to IMailSender but now I think it is only making things more complicate.

\$\endgroup\$
  • 2
    \$\begingroup\$ Another option is antaris.github.io/RazorEngine for people coming from an ASP.Net MVC background. \$\endgroup\$ – RobH Aug 16 '17 at 10:54
  • 2
    \$\begingroup\$ @RobH absolutely! \$\endgroup\$ – Adriano Repetti Aug 16 '17 at 11:00
4
\$\begingroup\$

As I mentioned in a comment, your main method isn't really helpful to see how you intend to use this because it's using an undeclared Sender and SendingMail is the name of a type which makes it look like you're calling a static method. I assume you have some fields or properties with these names.

I'm mostly going to limit myself to your SendingMail class.

It has very confusing behaviour in at least 2 ways:

  1. It stores errors as a public list
  2. You have to know the implementation to create it correctly

For point 1, why are your public methods void? You want to know the results of sending the emails (that's why you've added the list). Why not return the list directly. You could even add a MailSendingResult class which encapsulates both successes and failures as well as other interesting data to be logged somewhere.

For point 2, consider this:

var mailer = new SendingMail();
mailer.SendComeBackEmail(); // Boom. Because `Sender` property will be `null`.

I haven't set the IMailSender so the class can't work. Add a constructor so the class can be instantiated correctly and obviously.


You have a bunch of unused code, for example:

var template = Resources.WelcomeEmailTemplate;

Try to avoid DateTime.Now and prefer DateTime.UtcNow. The former is local time which can jump around when there are things like Daylight Savings Transitions or even this sort of thing: Samoa and Tokelau skip a day for dateline change.


Why have you implemented your interface explicitly? What benefit is that giving you?

Edit

You have this:

sealed class NullMailSender : IMailSender
{
    void IMailSender.Send(string from, string to, string body)
    {

    }
}

When you could have this:

sealed class NullMailSender : IMailSender
{
    public void Send(string from, string to, string body)
    {

    }
}

The explicit interface implementation means you have to use an instance of the interface not the class. So with explicit interface implementation I can't do this:

NullMailSender sender = new NullMailSender();
sender.Send(null, null, null);

I would have to do this:

IMailSender sender = new NullMailSender();
sender.Send(null, null, null);
\$\endgroup\$
  • 1
    \$\begingroup\$ Will try to implement most of your suggestion, but I got confused at "I haven't set the IMailSender so the class can't work. Add a constructor so the class can be instantiated correctly and obviously." How do you mean with that? Also "Why have you implemented your interface explicitly?" I'm a bit new to implementing interfaces and thought that was the only way. \$\endgroup\$ – JsonDork Aug 16 '17 at 12:02
  • 2
    \$\begingroup\$ I've added some extra comments for you @JsonDork \$\endgroup\$ – RobH Aug 16 '17 at 13:30
  • 1
    \$\begingroup\$ Sorry, but I'm still unsure what you're meaning. And what did you mean with the constructor part, I'm assuming making a constructor of the interface in the sendingmail class? Or? \$\endgroup\$ – JsonDork Aug 16 '17 at 13:45
  • 2
    \$\begingroup\$ @JsonDork Create a constructor for SendingEmail class which populates the Sender property. If you add the code you're actually using to the question that I can compile, I can hopefully explain further. See here for interface msdn.microsoft.com/en-us/library/aa288461%28v=vs.71%29.aspx \$\endgroup\$ – RobH Aug 16 '17 at 14:12
  • 2
    \$\begingroup\$ @JsonDork - You need to show how you're creating your SendingEmail instance because at the moment, Sender, Orders, and Customers will all be null and your methods will throw NullReferenceExceptions. \$\endgroup\$ – RobH Aug 16 '17 at 14:15

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.