3
\$\begingroup\$

I wrote a method that will confirm email with token send to user emails. It will also send different email depending on the condition for the registered user. If anyone would write the same method, how would you write this code snippet?

[AllowAnonymous, Route("confirm-email", Name = AccountControllerRoute.GetConfirmEmail)]
public async Task<ActionResult> ConfirmEmail(int accountId, string token)
{
    if (accountId == 0 || token == null)
        return HttpNotFound();

    var account = await AccountManager.FindByIdAsync(accountId);

    if (account == null)
        return HttpNotFound();

    if (account.EmailConfirmed)
        return View("ConfirmEmail"); //return page that will say the token has expired

    var result = await AccountManager.ConfirmEmailAsync(accountId, token);

    var emailBody = string.Empty;
    if (result.Succeeded)
    {
        if(account.Institution == null)
        {
            // send message notifying admin that a new institution needs review and setup
            emailBody = $"{account.UserName}|{account.profile.contact}|{account.profile.institutionName}|{account.profile.institutionAddress}";
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewInstitutionRequest, "");

            return View("ConfirmEmail");
        }

         // Pass Name, Username
        emailBody = $"{account.Name}|{account.UserName}";

        if (account.Institution.IsClient)
        {
            if (account.Institution.Licence.AllowedAccounts == 0)
            {
                await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
                return View("ConfirmEmail"); // send a different message notifying admin about exceeding allow accounts
            }

            // send message notifying admin that a new user request for access
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
            return View("ConfirmEmail"); 
        }
        else
        {
            // send message notifying admin that a new user has registered
            await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserNotificationForSA, emailBody);

            return View("ConfirmEmail");
        }
    }

    return View("Error");
}

Also, could there be a better way to just pass an object rather than a string with a separator that I have to use to parse them, once passed to a method where it send an email to the user?

public class PostalEmailService : IIdentityMessageService
{
    public Task SendAsync(IdentityMessage message)
    {
        if (message == null)
            throw new ArgumentNullException("message");

        EmailBase email;
        switch (message.Subject)
        {
            case EmailNames.ResetPassword:
                email = new ResetPasswordEmail {To = message.Destination, Link = message.Body};
                break;

            case EmailNames.NewUserRequest:
                string name = message.Body.Split('|')[0];
                string userName = message.Body.Split('|')[1];
                email = new NewUserRequestEmail { To = "[email protected]", Name = name, UserName = userName };
                break;
            default:
                throw new InvalidOperationException("Unknown email '{0}'.".FormatWith(message.Subject));
        }

        return email.SendAsync();
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$
    – Mast
    Aug 22, 2016 at 13:41
  • \$\begingroup\$ @Mast apology for that =) \$\endgroup\$
    – rpmansion
    Aug 22, 2016 at 13:42

1 Answer 1

2
\$\begingroup\$

I'm a bit confused by this:

if (account.EmailConfirmed) return View("ConfirmEmail"); //return page that will say the token has expired

Your comment doesn't correspond with the actual code, especially when that same view is returned elsewhere.


The if (result.Succeeded) should be inverted to avoid 30 lines of indentation. Why not do if (!result.Succeeded) and return View("Error"); immediately afterwards?

Also, result is a fairly meaningless name.


I don't know what you're doing with emailBody, but judging from its format and the comments it looks like you're using it to pass arguments in a delimiter-separated string. That's odd, to say the least.

I also don't see the point of compiling it inside if(account.Institution == null) and then not using its value.


What happens inside if (account.Institution.Licence.AllowedAccounts == 0) looks to be exactly the same as what happens if that isn't the case.


What is EmailNames? An enum? I don't think that class should have a plural name, and it doesn't look like it is an "email name".

\$\endgroup\$
8
  • \$\begingroup\$ I don't agree with the last statement, classes with contants-only, may have plural names. I do this all the time ie forColumnNames \$\endgroup\$
    – t3chb0t
    Aug 22, 2016 at 13:19
  • \$\begingroup\$ @t3chb0t Even in such cases I don't see why. To me ColumnNames.ID is an odd construct, since it will return the name of one column. But there are no hard rules, it's just an opinion (except for collection classes, certain types of enum,...). \$\endgroup\$
    – BCdotWEB
    Aug 22, 2016 at 13:27
  • \$\begingroup\$ haha, on the contrary I would never give a collection class a plural name - so we're even? ;-P \$\endgroup\$
    – t3chb0t
    Aug 22, 2016 at 13:29
  • \$\begingroup\$ EmailsNames is a class with constant properties. defining which type of emails will be used. \$\endgroup\$
    – rpmansion
    Aug 22, 2016 at 13:30
  • \$\begingroup\$ if (account.Institution.Licence.AllowedAccounts == 0) it will look the same but it has different email template that will be sent to admin. \$\endgroup\$
    – rpmansion
    Aug 22, 2016 at 13:31

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.