10
\$\begingroup\$
private void SendActivationEmail(MembershipUser user)
{
    var message = new MailMessage();
    message.To.Add(user.Email);
    message.Subject = "Attorney Event Notification Activation";

    string urlBase = Request.Url.GetLeftPart(UriPartial.Authority) + Request.ApplicationPath;
    string ActivationUrl = "/ActivateUser.aspx?ID=" + GetUserID(user.Email);
    var fullPath = urlBase + ActivationUrl;            

    using (StreamReader sr = new StreamReader(Request.PhysicalApplicationPath + "ActivationTemplate.txt"))
    {
        message.Body = sr.ReadToEnd();
    }

    message.Body = message.Body.Replace("<%UserName%>", user.UserName);
    message.Body = message.Body.Replace("<%ActivationURL%>", fullPath);
    message.IsBodyHtml = true;

    using (var smtpClient = new SmtpClient())
    {
        smtpClient.Send(message);
    }
}

How can I clean up this activation email sender?

And is it relatively safe?

\$\endgroup\$
2
  • \$\begingroup\$ If the user id is a simple integer then the recipient simply try the next number in order to activate another user. One way to prevent this is to generate a GUID to prevent end user from guessing the next number and also have a time window for the GUID to be valid only, say 24 hours from when the email is sent. \$\endgroup\$
    – Tien Dinh
    Dec 27, 2014 at 17:06
  • \$\begingroup\$ The userID is a GUID. I am using the SQL Membership Provider package in SQL and ASP.NET \$\endgroup\$
    – Malachi
    Dec 28, 2014 at 2:54

2 Answers 2

4
\$\begingroup\$

Decoupling

If we extract the composition of the body to a separate method

private String ComposeEmailBody(String userName, String userEmail)
{
    String bodyTemplate = ReadBodyTemplate(Request.PhysicalApplicationPath, "ActivationTemplate.txt");
    bodyTemplate  = bodyTemplate.Replace("<%UserName%>", userName);

    return bodyTemplate.Replace("<%ActivationURL%>", GetUrlFor(userEmail));
}

private String ReadBodyTemplate(String path, String fileName)
{
    using (StreamReader sr = new StreamReader(Path.Combine(path, fileName))
    {
        return sr.ReadToEnd();
    }
}

private String GetUrlFor(String userEmail)
{
    return Request.Url.GetLeftPart(UriPartial.Authority) 
         + Request.ApplicationPath  
         + "/ActivateUser.aspx?ID="  
         + GetUserID(userEmail);     
}  

after introducing an overloaded SendActivationEmail() method

private void SendActivationEmail(String userName, String userEmail)
{
    var message = new MailMessage();
    message.To.Add(userEmail);
    message.Subject = "Attorney Event Notification Activation";
    message.Body = ComposeEmailBody(userName, userEmail);
    message.IsBodyHtml = true;
    using (var smtpClient = new SmtpClient())
    {
        smtpClient.Send(message);
    }
} 

the former method can be refactored to a call to the overloaded method, which decouples the overloaded one from MembershipUser

private void SendActivationEmail(MembershipUser user)
{
    SendActivationEmail(user.UserName, user.Email);
}  

Nitpicking

You are mixing the useage of the concrete type and var

string urlBase = Request.Url.GetLeftPart(UriPartial.Authority) + Request.ApplicationPath;
string ActivationUrl = "/ActivateUser.aspx?ID=" + GetUserID(user.Email);
var fullPath = urlBase + ActivationUrl;  

here it should be either all var or all string.

\$\endgroup\$
4
\$\begingroup\$

I think there is a problem with encapsulation and the single responsibility principle here. The method depends on too many external factors, it makes too many external references, and it includes logic of multiple independent responsibilities that would make sense to extract elsewhere.

These are good and belong to this method:

  • Creating and filling a MailMessage object
  • The constant strings for the subject
  • The SmtpClient object that sends the message

If the method contained only the above, it would be perfectly reasonable.

But it contains some extra stuff that don't seem to belong:

  • The construction logic of fullPath. It seems this logic should be somewhere else, for example a centralized router component
  • The construction logic of the template file path to read
  • The replacement of the template parameters UserName and ActivationURL would also be good to extract to a component. There should be a component in charge of user templates and the template parameters, which naturally takes a user template and returns properly formatted text. That way, if the template changes, the logic of the template parameters will be closer to the logic that fills those parameters, making the code more cohesive.

Lastly, as a minor remark, the variable naming is not consistent. Most of the local variables are camelCase, except ActivationUrl:

string urlBase = Request.Url.GetLeftPart(UriPartial.Authority) + Request.ApplicationPath;
string ActivationUrl = "/ActivateUser.aspx?ID=" + GetUserID(user.Email);

It would be better to use camelCase consistently.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I don't know how that variable name slipped through, I was trying my best to be consistent on the naming too. thanks for the review. \$\endgroup\$
    – Malachi
    Dec 18, 2014 at 22:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.