Function to send email, refactored based on SOLID principles I have the below method that is responsible for generating and sending a MailMessage using a SmtpClient object:

public static void SendEmail(
    string subject, 
    string body, 
    bool isBodyHtml, 
    List&lt;string&gt; emails, 
    List&lt;string&gt; ccs = null)
{
    if (emails == null)
    {
        throw new ArgumentNullException("emails");
    }
    MailMessage message = new MailMessage
    {
        Subject = subject,
        Body = body,
        IsBodyHtml = isBodyHtml
    };
    foreach (string email in emails)
    {
        message.To.Add(email);
    }
    if (ccs != null)
    {
        foreach (string cc in ccs)
        {
            message.CC.Add(cc);
        }
    }
    SmtpClient client = new SmtpClient
    {
        EnableSsl = true,
        DeliveryMethod = SmtpDeliveryMethod.Network
    };
    client.Send(message);
}


But recently I've been reading more on S.O.L.I.D and principles of OOP design, so I thought to refactor this method, and compartmentalize it more.

Am I on the right track with my refactor? Are there ways to make my code more modular/testable?

Here's my refactored code:

private static MailMessage CreateMailMessage(
    string subject, 
    string body, 
    bool isBodyHtml, 
    List&lt;string&gt; emails, 
    List&lt;string&gt; ccs = null)
{
    if (emails == null)
    {
        throw new ArgumentNullException("emails");
    }
    MailMessage message = new MailMessage
    {
        Subject = subject,
        Body = body,
        IsBodyHtml = isBodyHtml
    };
    foreach (string email in emails)
    {
        message.To.Add(email);
    }
    if (ccs != null)
    {
        foreach (string cc in ccs)
        {
            message.CC.Add(cc);
        }
    }
    return message;
}

private static SmtpClient CreateSmtpClient(
    bool enableSsl, 
    SmtpDeliveryMethod deliveryMethod)
{
    SmtpClient client = new SmtpClient
    {
        EnableSsl = enableSsl,
        DeliveryMethod = deliveryMethod
    };
    return client;
}


And here's the method responsible for calling the above two and sending the email:

public static void SendEmail(string subject, string body, List&lt;string&gt; emails, List&lt;string&gt; ccs = null)
{
    using (MailMessage message = CreateMailMessage(subject, body, true, emails, ccs))
    {
        using (SmtpClient client = CreateSmtpClient(true, SmtpDeliveryMethod.Network))
        {
            client.Send(message);
        }
    }
}
I'd still say that you should break apart the mail message and SMTP client into separate classes for the "Single responsibility principle". In specific:


  A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.


This is usually where you have a MailFactory and a SmtpClientFactory that are injected into the class that actually does the sending (MailSender). The injection (by abstract, so interfaces) would be set into the MailSender to fulfill the "Dependency inversion principle"


  One should "depend upon abstractions, [not] concretions."


Side Note

This would also let you reduce MailFactory.CreateMailMessage to MailFactory.Create (and SmtpClientFactory.CreateSmtpClient to SmtpClientFactory.Create) but that isn't directly related to SOLID. You can get why in various API naming conventions, but the short answer is that it would be redundant.

Expansion

A reason for this is when you do unit testing. We have an abstract mail client that just sticks messages into an in-memory list in the order they are sent. That way, we can inject it into the sender as ISmtpClientFactory and the rest of the code still works, not knowing that we have taken out the entire implementation of the sending for testing.

Edit 2

You also want to avoid static in general. It is implied by "injecting" but I figured I'd clarify that. Create factory instances and pass them in. That way you can swap them out.