3
\$\begingroup\$

I've created an email sender with providers for the email content, that will be changed based on the email type. I need some help to enhance it.

These two models are used to send

public class EmailAddress
{
    public string Name { get; set; }
    public string Address { get; set; }
}

public class EmailMessage
{
    public EmailMessage()
    {
        ToAddresses = new List<EmailAddress>();
        CcAddresses = new List<EmailAddress>();
    }

    public List<EmailAddress> ToAddresses { get; set; }
    public List<EmailAddress> CcAddresses { get; set; }
    public string Subject { get; set; }
    public string Content { get; set; }
}

The content provider provides all the email information(subject, body, To and Cc)

public interface IEmailContentProvider
{
    EmailMessage Message { get; }
}

Then we have the abstracted email sender IEmailSender that has a single method Send which uses IEmailContentProvider parameter to get the email information

interface IEmailSender
    {
        Task Send(IEmailContentProvider provider);
    }

I have an example for the content provider WelcomEmailProvider

public class WelcomEmailProvider : IEmailProvider
{
        public EmailMessage Message { get; }

        public WelcomEmailProvider(string address, string name)
        {
            Message = new EmailMessage
        {
            Subject = $"Welcome {name}",
            Content = $"This is welcome email provider!",
            ToAddresses = new List<EmailAddress> { new EmailAddress { Address = address, Name = name} }
        };
    }
}

The IEmailSender implementation:

public class EmailSender : IEmailSender
{
    private readonly SmtpOptions _options;

    public EmailSender(IOptions<SmtpOptions> options)
    {
        _options = options.Value;
    }

    public async Task Send(IEmailContentProvider provider)
    {
        var emailMessage = provider.Message;
        var message = new MimeMessage();
        message.From.Add(new MailboxAddress(_options.Sender.Name, _options.Sender.Address));
        message.To.AddRange(emailMessage.ToAddresses.Select(x => new MailboxAddress(x.Name, x.Address)));
        message.Cc.AddRange(emailMessage.CcAddresses.Select(x => new MailboxAddress(x.Name, x.Address)));

        message.Subject = emailMessage.Subject;
        message.Body = new TextPart(TextFormat.Html) { Text = emailMessage.Content };

        using var emailClient = new SmtpClient();
        await emailClient.ConnectAsync(_options.Server, _options.Port, _options.EnableSsl);
        await AuthenticatedData(emailClient);

        await emailClient.SendAsync(message);
        await emailClient.DisconnectAsync(true);

    }

    private async Task AuthenticatedData(SmtpClient smtpClient)
    {
        if (string.IsNullOrWhiteSpace(_options.Username) || string.IsNullOrWhiteSpace(_options.Password))
            return;

        emailClient.AuthenticationMechanisms.Remove("XOAUTH2");
        await emailClient.AuthenticateAsync(_options.Username, _options.Password);
    }
}

And here is, how to use it and send an email:

class Sample
{
    private readonly IEmailSender _emailSender;
    public Samole(IEmailSender emailSender)
    {
        _emailSender = emailSender;
    }

    public async Task DoSomethingThenSendEmail()
    {
        await _emailSender.Send(new WelcomEmailProvider("someone@example.com", "Someone"));
    }
}
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$
public EmailMessage()
{
    ToAddresses = new List<EmailAddress>();
    CcAddresses = new List<EmailAddress>();
}

Not necessary, I can understand if you just initiate the ToAddress, however, initiating the lists like this might consume a lot of memory (imagine you have large amount of EmailMessage instances! So, I would suggest to keep them as null, and use null validation to force initiating them when it's needed (like in sender).

your overall design is good enough, however, you could do this directly :

public class EmailMessage 
{
    public EmailAddress From { get; set; }
    public IEnumerable<EmailAddress> To { get; set; }
    public IEnumerable<EmailAddress> Cc { get; set; }
    public IEnumerable<EmailAddress> Bcc { get; set; }
    public string Subject { get; set; }
    public string Body { get; set; }
}

public interface IEmailProvider
{
    IEmailServerSetting ServerSettings { get; set; }
}

EmailMessage should contain From(required), To(required), CC(optional),BCC(optional), Subject, and Body as a full model of the message. The reason behind that is that it will be always paired together as requirement to any message, later on, it would be also easier to implement on the database side. using IEnumerable<EmailAddress> would open to use any collection that implements IEnumerable. So, it's not restricted to List.

For IEmailProvider As an email provider should also contain its own settings as requirement. This would make things easier for you, to have each email message with it's own settings. This way, you can send multiple emails from different providers with no extra coding. IOptions is treated independently, but in reality, IOptions won't be useful by itself, and it would only used by EmailProvider, so if we add it to the contract, it'll always be implemented with the interface, this way you forced the provider to always have IOptions. Besides, IOptions should be renamed to something like EmailServerSetting or EmailProviderSetting to relate it to the main implementation. As these are settings and not options. Use IOptions to handle the email optional settings and features that can be managed like sending as text or html, disable/enable attachments, pictures ..etc.

Also, you need a middle-ware class to wrap things up, and uses SmtpClient. This would give you the advantage of increasing the flexibility of your current work and wrap them under one roof along with ease things up for reusing code (such as MimeMessage, TextPart ..etc.) instead of reimplement it on each new sender. It also would give you the ability to create a collection to store multiple providers if you're going that far. also, you will be able to add new providers, handle them, handle messages, and keep your work scoped.

Here is how I imagine the final usage :

Creating a new provider :

// creating a new email provider 
    public class SomeEmailProvider : IEmailProvider
    {
        // only set the settings internally but it's exposed to be readonly
        public EmailServerSetting ServerSettings { get; private set; }
        
        public SomeEmailProvider()
        {
            //set up the server settings
            ServerSettings = new EmailServerSetting 
            {
                ServerType = EmailServerType.POP, 
                Server = "pop.mail.com",
                Port = 995, 
                Encryption = EmailServerEncryption.SSLOrTLS, 
                EnableSsl = true
            };      
        }
       // some other related code 
    }

now, creating a new mail message and sending it :

// can be single object or collection 
var messages = new EmailMessage
{
    From = new EmailAddress("Test", "test@mail.com"), 
    To = new EmailAddress [] {
         new EmailAddress("Test1", "test1@mail.com"), 
         new EmailAddress("Test2", "test2@mail.com")
        }, 
    Subject = "Testing Subject", 
    Body = "Normal Text Body"           
};

using(var client = new EmailClient(new SomeEmailProvider()))
{
    client.Options = new EmailClientOptions 
    {
        // would send it as plain text 
        EnableHtml = false 
    };
    
    client.Messages.Add(messages); 
    
    client.Send();  
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Initializing the lists as empty is much more expressive, and introducing null as a possible value complicates the other code. There could be a case where the memory usage is the limiting factor, but this shouldn't be assumed. By introducing the null-checks, the CPU must spend extra cycles when working with an EmailMessage. \$\endgroup\$ Sep 1, 2020 at 1:45

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.