patterncsharpMinor
Sending activation email for sqlmembership
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:
If the method contained only the above,
it would be perfectly reasonable.
But it contains some extra stuff that don't seem to belong:
Lastly, as a minor remark, the variable naming is not consistent.
Most of the local variables are
It would be better to use
These are good and belong to this method:
- Creating and filling a
MailMessageobject
- The constant strings for the subject
- The
SmtpClientobject 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
UserNameandActivationURLwould 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.