3
\$\begingroup\$

I have the following working code to send search results via e-mail. Since we have segregated domains that cannot talk to each other, the program needs to support multiple SMTP servers.

private static int[] _SMTPport = new int[2] { 25, 587 };
private static string[] _SMTPserver = new string[4]
{
  "smtp.domain1.local",
  "smtp.domain2.local",
  "smtp.domain3.local",
  "smtp.domain4.local" 
};

switch (sUserDomain) 
{
  case: "domain1":
  case: "DOMAIN1":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
  case: "domain2":
  case: "DOMAIN2":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
  case: "domain3":
  case: "DOMAIN3":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
  case: "domain4":
  case: "DOMAIN4":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
  default:
    Send_Email(_SMTPserver[3], _SMTPport[0], false);
    break;
}

private static void Send_Email(string _server, int _port, bool _ssl)
{
  StringBuilder emailString = new StringBuilder();
  MailMessage mMessage = new MailMessage();

  SmtpClient smtpClient = new SmtpClient(_server);
  smtpClient.Port = _port;
  smtpClient.DeliveryMethod = SmtpDeliveryMethod.Network;

  if (_ssl.Equals(true)) 
  { smtpClient.EnableSsl = true; }

  ServicePointManager.ServerCertificateValidationCallback = delegate(object s, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
  { return true; };

  mMessage.From = new MailAddress("from-address");
  mMessage.To.Add(sEmailAddress);

  mMessage.Subject = "Logs from " + Environment.GetEnvironmentVariable("COMPUTERNAME") + " searched " + sDate + " for " + sSearchTerm;
  emailString.Append("<p style='font-family:arial,helvetica,sans-serif;'>Here are the file(s) on <b>" + Environment.GetEnvironmentVariable("COMPUTERNAME") + "</b> under <b>" + sDate + "</b> that contain <b><i>" + sSearchTerm + "</i><b></p><br/><br/>");
  emailString.AppendLine("<table width='100%' border='0' align='center' cellpadding='5' cellspacing='0' style='font-family:arial,helvetica,sans-serif;'><tbody><tr><td style='padding:5px;background-color:rgb(169, 169, 169);color:white;'>Filenames</td></tr>");
  int count = 0;
  foreach (var item in logList)
 {
   if (count % 2 == 0)
   {
     emailString.AppendLine("<tr><td style='background-color:#bada55;border-collapse:collapse;'>" + Path.GetFileName(item) + "</td></tr>");
   }
   else
  {
    emailString.AppendLine("<tr><td style='background-color:#55bada;border-collapse:collapse;'>" + Path.GetFileName(item) + "</td></tr>");
  }
  count++;
}
emailString.Append("</tr></tbody></table>");
mMessage.IsBodyHtml = true;

mMessage.Body = emailString.ToString();

I'm working on converting it to be object-oriented. My concern is that I need to add multiple items to the message before sending. Here is what I have so far — am I on the correct path?

public class EmailClass
{
  private string _domain;
  private string _emailaddress;
  private string _fromaddress;
  private string _subject;
  private List<string> _message = new List<string>();

  public string Domain
  {
    get { return _domain; }
    set { _domain = value; }
  }

  public string EmailAddress
  {
    get { return _emailaddress; }
    set { _emailaddress = value; }
  }

  public string FromAddress
  {
    get { return _fromaddress; }
    set { _fromaddress = value; }
  }

  public string Subject
  {
    get { return _subject; }
    set { _subject = value; }
  }
  public List<string> Message
  {
    get { return _message; }
    set { _message = value; }
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. Can you describe some more about what your code does? How much do you know about C# classes? I would recommend trying to write a class implementation and then coming here and ask for a review on it. It sounds like you already know what you need to do. \$\endgroup\$ – Simon Forsberg Nov 5 '14 at 19:47
  • \$\begingroup\$ @SimonAndréForsberg My code uses our companies smtp relays to send email messages based on intuit. I don't know very much about C# classes \$\endgroup\$ – ondrovic Nov 5 '14 at 19:48
  • \$\begingroup\$ Perhaps what you might need is a tutorial on C# classes? \$\endgroup\$ – Simon Forsberg Nov 5 '14 at 19:49
  • \$\begingroup\$ I have been looking at this one blackwasp.co.uk/CSharpSimpleClass.aspx \$\endgroup\$ – ondrovic Nov 5 '14 at 19:50
4
\$\begingroup\$

SRP - single responsibility principle

The Send_Email() method is responsible for to many things.
The method is

  1. creating and configuring a SmtpClient
  2. creating and configuring a MailMessage
  3. composing the body of the MailMessage

This should be extracted to separate methods.

So let us refactor this method.

First we write a method to create a SmtpClient.

private SmtpClient GetSmtpClient(string serverAddress, int serverPort, bool enableSsl)
{
    SmtpClient smtpClient = new SmtpClient(serverAddress);
    smtpClient.Port = serverPort;
    smtpClient.DeliveryMethod = SmtpDeliveryMethod.Network;
    smtpClient.EnableSsl = enableSsl;

    return smtpClient;
}

This had been pretty easy, hadn't it ? So, next we add a method composing the Subject by using the String.Format() method.

private String ComposeSubject(LogResult logResult) 
{
    return String.Format("Logs from {0} searched {1} for {2}", logResult.ComputerName, logResult.SearchDate, logResult.SearchTerm);
}

But wait, what is LogResult ? That is a class we need to create, so let us do it. As we see it should have at least the properties ComputerName, SearchDate and SearchTerm. We add also a ReadOnlyCollection<String> FileNames property.

public class LogResult
{
    public String ComputerName { get; private set; }
    public DateTime SearchDate { get; private set; }
    public String SearchTerm { get; private set; }
    public ReadOnlyCollection<String> FileNames { get; private set; }

    public LogResult(String computerName, DateTime searchDate, String searchTerm, List<String> logList)
    {
        ComputerName = computerName;
        SearchDate = searchDate;
        SearchTerm = searchTerm;
        FileNames = ToFileNames(logList);
    }

    public LogResult(DateTime searchDate, String searchTerm, List<String> logList)
        : this(Environment.GetEnvironmentVariable("COMPUTERNAME"), searchDate, searchTerm, logList)
    { }

    private ReadOnlyCollection<String> ToFileNames(List<String> logList)
    {
        IList<String> fileNames = new List<String>(logList.Count);
        foreach (String completeFileName in logList)
        {
            fileNames.Add(Path.GetFileName(completeFileName));
        }
        return new ReadOnlyCollection<String>(fileNames);
    }
}

So now we will use this class to also compose the email body. Here we are using the feature of the StringBuilder that it is returning a StringBuilder for calling the AppendXX() methods. So we can call StringBuilder.Append().Append()...we are using also the StringBuilder.AppendFormat() method.

private String ComposeMailBody(LogResult logResult)
{
    StringBuilder bodyBuilder = new StringBuilder(1024);

    bodyBuilder.Append("<p style='font-family:arial,helvetica,sans-serif;'>")
        .AppendFormat("Here are the file(s) on <b> {0}", logResult.ComputerName)
        .AppendFormat("</b> under <b>{0}", logResult.SearchDate)
        .AppendFormat("</b> that contain <b><i>{0}</i><b></p><br/><br/>",logResult.SearchTerm)
        .AppendLine("<table width='100%' border='0' align='center' cellpadding='5' cellspacing='0' style='font-family:arial,helvetica,sans-serif;'><tbody><tr><td style='padding:5px;background-color:rgb(169, 169, 169);color:white;'>Filenames</td></tr>");

    int count = 0;
    foreach (String fileName in logResult.FileNames)
    {
        if (count % 2 == 0)
        {
            bodyBuilder.AppendFormat("<tr><td style='background-color:#bada55;border-collapse:collapse;'>{0}</td></tr>",fileName);
        }
        else
        {
            bodyBuilder.AppendFormat("<tr><td style='background-color:#55bada;border-collapse:collapse;'>{0}</td></tr>",fileName);
        }
        count++;
    }
    bodyBuilder.Append("</tr></tbody></table>");

    return bodyBuilder.ToString();
}

The last method we introduce is for creating a MailMessage.

private MailMessage CreateMessage(LogResult logResult, String receiver)
{
    MailMessage message = new MailMessage("from-address", receiver);
    message.Subject = ComposeSubkject(logResult);
    message.Body = ComposeMailBody(logResult);

    return message;
}

Now the former Send_Email() method would look like

private static void SendEmail(string server, int port, bool ssl)
{
    LogResult logResult = new LogResult(sDate ,sSearchTerm, logList);
    SmtpClient smtpClient = GetSmtpClient(server, port, ssl);
    MailMessage message = CreateMailMessage(sEmailAddress);

    smtpClient.Send(message);
}

EmailClass

Using auto properties will reduce your code a lot. Auto properties are properties with a hidden backing field generated by the compiler.

public class EmailClass
{
    public string Domain { get; set; }

    public string EmailAddress { get; set; }

    public string FromAddress { get; set; }

    public string Subject { get; set; }

    public List<string> Message { get; set; }

}

Naming

Based on the naming guidlines method names should be named using PascalCase casing. So Send_Email should be renamed to SendEmail.
Based on the same guidlines method parameters should be named using camelCase casing. the signature of the former Send_Email() method should look like

private static void SendEmail(string server, int port, bool ssl) 
\$\endgroup\$
2
\$\begingroup\$

This is in addition to Heslacher's great review.

I'm not a big fan of the arrays you define at the start, since a line like this sin't really telling me anything:

Send_Email(_SMTPserver[0], _SMTPport[0], false); 

I'd put the values of those arrays in a static class and expose them as const strings/int, e.g.

internal static class SmtpPort
{
    public const int TwentyFive = 25;
}

internal static class SmtpServer
{
    public const string Domain1 = "smtp.domain1.local";
}

That way your call becomes far more readable:

Send_Email(SmtpServer.Domain1, SmtpPort.TwentyFive, false); 

Note that SMTP shouldn't be all capitals:

Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier.

A class named XmlWriter is an example of a long acronym used as the first word of a Pascal-cased identifier. A parameter named htmlReader is an example of a long acronym used as the first word of a camel-cased identifier.

I'd also eliminate the need for multiple cases in your switch by simply converting sUserDomain first:

switch (sUserDomain.ToLower()) 
{
  case: "domain1":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
  case: "domain2":
    Send_Email(_SMTPserver[0], _SMTPport[0], false); 
    break;
}

Note that sUserDomain is a bad variable name: Hungarian notation is frowned upon.

\$\endgroup\$

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.