FluentEmail code review

Today, I would like to look within the lukencode/FluentEmail repository, written by Luke Lowrey.

Right now, it has about 439 stars. The first commit was at 10th April 2010. So it may contain many legacy code and the projects can be very huge. Unfortunately It hasn’t been developed that much in the first couple of years.

Major features

Using this library, you can upload files to any FTP server.

No, just kidding. As the name predicates, the framework is responsible for sending emails.

Sending an email dotNET isn’t really complicated. You are just need a SmtpClient and a MailMessage.

MailMessage mail = new MailMessage();
mail.From = new MailAddress("bill.gates@microsoft.com");
mail.To.Add("marc.zuckerberg@facebook.com");
mail.Subject = "URGENT!";
mail.Body = "Hey there, here is Bill";

SmtpClient client = new SmtpClient("smtp.microsoft.com", 25);
client.Credentials = new NetworkCredential("bill.gates@microsoft.com", "Top3ecret");
client.Send(mail);

This could be replaced by this couple lines:

Email.From("bill.gates@microsoft.com")
     .To("marc.zuckerberg@facebook.com", "Zucky")
     .Subject("URGENT")
     .Body("Hey there, here is Bill")
     .Send();

You can also use templates either as inline text (which can be stored within a resource file) or an html file and set only the variables.

Isn’t it awesome?

You can find a short documentation at the author’s website.

Let’s make a deep dive into the code!

Developing a fluent API is always a pain. The method names mostly aren’t clear enough and the code depends mostly on the previous expression. Take a look at NUnit. You can write:

Assert.That(actually, Is.Not.Null);
Assert.That(actually, Is.EqualsTo("expected"));

Based on the “Not” keyword, NUnit completely negate the constraint. Sure, it is the same constraint, but with another logic. I’ve also seen an option type library, where you could not write the .None() before the .Some().

However, Luke chooses a “simply” fluent pattern. Which only contain one major class with all functionalities and always return itself.

The result looks something like this:

public class Email : IFluentEmail
{
   /// <summary>
   /// Sets the subject of the email
   /// </summary>
   /// <param name="subject">email subject</param>
   /// <returns>Instance of the Email class</returns>
   public IFluentEmail Subject(string subject)
   {
      Data.Subject = subject;
      return this;
  }
}

Returning “this”, isn’t bad. It is OK. I do it also sometimes if I write similar functionalities (But I don’t call them Fluent 🙂 ).

It’s all about the names

While the class, interface and namespace names are good chosen and well structures, the method naming within the Email class are bad. Very bad. As the usage should be fluent, Luke hadn’t a chance to use better names. Otherwise, the usage would be a pain.

In case of doubt, you should always consider the opposite side. The best case would be, that you can read the expression as a sentence.

I think, you will surely agree, that the code above is more readable than that example.

EmailFactory.AddSender("bill.gates@microsoft.com")
   .AddReceiver("marc.zuckerberg@facebook.com", "Zucky")
   .SetSubject("URGENT")
   .SetMessage("Hey there, here is Bill")
   .Send();

There is an inline documentation …

I don’t see documentation in other code that much. And it is not a bad habit not to write some. If the method names are good chosen, you do not need to write documentation. If the method or class name is well named, you don’t need any documentation. It is everything clear enough.

But if you consider writing documentation, please write a good one.

It must have an additional benefit!

/// <summary>
/// Adds a Body to the Email
/// </summary>
/// <param name="body">The content of the body</param>
/// <param name="isHtml">True if Body is HTML, false for plain text (Optional)</param>
IFluentEmail Body(string body, bool isHtml = false);

Sorry Luke, I don’t see a real benefit within this documentation. You are trying to explain me one line of code with five lines, which are saying completely the same.

The implementation to that method definition is the snippet below:

/// <summary>
/// Adds a Body to the Email
/// </summary>
/// <param name="body">The content of the body</param>
/// <param name="isHtml">True if Body is HTML, false for plain text (default)</param>
public IFluentEmail Body(string body, bool isHtml = false)
{
   Data.IsHtml = isHtml;
   Data.Body = body;
   return this;
}

As you can see, the documentation part is completely the same.

If you have this scenario, please consider using the inhericdoc tag and you are good to go. This will save you also many lines of code within your class 🙂

How many lines of code should a file have?

Most of the files are clean and small. Except one file. I know, Luke want to put everything into a single class. But this is often not a good choice.

Classes should be small. From my point of view they should have at most 220 line of code. It is a good length. If you have more than 220 lines of code, you should consider splitting the file into several files. It is often a sign that the class is making too much.

Some here. Luke could use the Email class to generate the header and a body class to generate the email’s body.

Afterwards there would be two at least two interfaces:

public interface IFluentEmail: IHideObjectMembers
{
   EmailData Data { get; set; }
   ITemplateRenderer Renderer { get; set; }
   ISender Sender { get; set; }
   IFluentEmail To(string emailAddress, string name = null);
   IFluentEmail SetFrom(string emailAddress, string name = null);
   IFluentEmail To(string emailAddress);
   IFluentEmail To(IList<Address> mailAddresses);
   IFluentEmail CC(string emailAddress, string name = "");
   IFluentEmail CC(IList<Address> mailAddresses);
   IFluentEmail BCC(string emailAddress, string name = "");
   IFluentEmail BCC(IList<Address> mailAddresses);
   IFluentEmail ReplyTo(string address);
   IFluentEmail ReplyTo(string address, string name);
   IFluentEmail Subject(string subject);  

   IFluentEmail Body(string body, bool isHtml = false);
   IFluentEmail Body(IFluentEmailBody body);

   IFluentEmail HighPriority();
   IFluentEmail LowPriority();

   IFluentEmail Attach(Attachment attachment);
   IFluentEmail Attach(IList<Attachment> attachments);

   SendResponse Send(CancellationToken? token = null);
   Task<SendResponse> SendAsync(CancellationToken? token = null);
   IFluentEmail AttachFromFilename(string filename, string contentType = null);
}

public interface IFluentEmailBody
{
   IFluentEmailBody UsingTemplateEngine(ITemplateRenderer renderer);
   IFluentEmailBody UsingTemplateFromEmbedded<T>(string path, T model, Assembly assembly, bool isHtml = true);
   IFluentEmailBody UsingTemplateFromFile<T>(string filename, T model, bool isHtml = true);
   IFluentEmailBody UsingCultureTemplateFromFile<T>(string filename, T model, CultureInfo culture, bool isHtml = true);
   IFluentEmailBody UsingTemplate<T>(string template, T model, bool isHtml = true);
   IFluentEmailBody PlaintextAlternativeBody(string body);
   IFluentEmailBody PlaintextAlternativeUsingTemplateFromEmbedded<T>(string path, T model, Assembly assembly);
   IFluentEmailBody PlaintextAlternativeUsingTemplateFromFile<T>(string filename, T model);
   IFluentEmailBody PlaintextAlternativeUsingCultureTemplateFromFile<T>(string  filename, T model, CultureInfo culture);
   IFluentEmailBody PlaintextAlternativeUsingTemplate<T>(string template, T model);
}

But not only a class should be small. A method should be short as well.

I’ve looked into one of the senders. The StmpSender contain a private method, called CreateMessage. I have a 28-inch monitor and the method doesn’t fit my screen.

A good method should have at most 20 lines of code. I am pretty sure, this method can be split into several methods.

By only introducing some new methods, the code would be much more readable and testable. The result could look somehow like that:

private MailMessage CreateMailMessage(IFluentEmail email)
{
   MailMessage message = !string.IsNullOrEmpty(email.Data.PlaintextAlternativeBody)
      ? CreateAlternativeMessage(email)
      : CreateSimpleMessage(email);

   SetReceivers(messagem email);
   SetPriority(message, email);
   SetAttachments(message, email);

   return message;
}

private MailMessage CreateSimpleMessage(IFluentEmail email)
{
   return new MailMessage
   {
      Subject = email.Data.Subject,
      Body = email.Data.Body,
      IsBodyHtml = email.Data.IsHtml,
      From = GetSender(email)
   };
}

private MailMessage CreateAlternativeMessage(IFluentEmail email)
{
   var message = new MailMessage
   {
      Subject = email.Data.Subject,
      Body = email.Data.PlaintextAlternativeBody,
      IsBodyHtml = false,
      From = GetSender(email)
   };
 
   var mimeType = new System.Net.Mime.ContentType("text/html");
   AlternateView alternate = AlternateView.CreateAlternateViewFromString(email.Data.Body, mimeType);
   message.AlternateViews.Add(alternate);
   return message;
}

private MailAddress GetSender(IFluentEmail email)
{
   return new MailAddress(email.Data.FromAddress.EmailAddress, email.Data.FromAddress.Name);
}

private void SetReceivers(MailMessage message, IFluentEmail email)
{
   email.data.ToAddresses.ForEach(x =>
   {
      message.To.Add(new MailAddress(x.EmailAddress, x.Name));
   });

   email.data.CcAddresses.ForEach(x =>
   {
      message.CC.Add(new MailAddress(x.EmailAddress, x.Name));
   });

   email.data.BccAddresses.ForEach(x =>
   {
      message.Bcc.Add(new MailAddress(x.EmailAddress, x.Name));
   });

   email.data.ReplyToAddresses.ForEach(x =>
   {
      message.ReplyToList.Add(new MailAddress(x.EmailAddress, x.Name));
   });
}

private void SetPriority(MailMessage message, IFluentEmail email)
{
   switch (email.Data.Priority)
   {
      case Priority.Low:
         message.Priority = MailPriority.Low;
         break;
      case Priority.Normal:
         message.Priority = MailPriority.Normal;
         break;
      case Priority.High:
         message.Priority = MailPriority.High;
         break;
   }
}

private void SetAttachments(MailMessage message, IFluentEmail email)
{
   email.Data.Attachments.ForEach(x =>
   {
      message.Attachments.Add(new System.Net.Mail.Attachment(x.Data, x.Filename, x.ContentType));
   });
}

I totally agree, there code is a little bit longer than the original. But as this code does not have any dependencies to the rest of the class, it can be moved into a separate class as well.

Conclusion – FluentEmail

I love it 🙂 No, really. I love it!

The implementation is great. The idea is awesome. Everything I said, is only “lament on of high-level”. Every code can be improved. Source code cannot be perfect like some shiny gems. Most of the time the developer is forced on a problem. If the problem is solved, the code will be left as it is. Without refactoring.

We (software developer) tending to solve an issue as easy as we can. Just to make sure, we know what we do.

Luke did it. He solved that issue 🙂

Leave a Reply

Your email address will not be published.