HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Sending activation email for sqlmembership

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
emailsqlmembershipsendingforactivation

Problem

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("", user.UserName);
    message.Body = message.Body.Replace("", 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?

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#74112, answer score: 4

Revisions (0)

No revisions yet.