Validating email confirmation and sending different emails to admins

Question

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();
    }
}

Answer 1

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".