Problem
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;
}
}
}
Solution
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:
NumberOfRetriesOnErrorandDelayOnErrorshould beprivate constclass fields, not local variables.Send()method has unneeded knowledge of the actualIMailSenderimplementation, it renders the interface itself half useless. Callers ofIMailSender.Send()should have no knowledge at all about it! No error handling in that method and the message itself shouldn’t be created insideMailSender.
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.
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:
- It stores errors as a public list
- 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);