Create a new Member

Question

I was tasked with member creation on a site that was already developed for Active Directory, so the log in functionality was there and most of the site was all ready to go (after minor major tweaks).

Here is what I came up with for my create user method (Button_Click Event).

Please let me know what you think.

protected void createUser_Click(object sender, EventArgs e)
{
    var createStatus = new MembershipCreateStatus();

    if (IsBarNumberAndEmailValid(email.Text, barNumber.Text))
    {
        if (password.Text == confirmPassword.Text)
        {
            var newUser = Membership.CreateUser(username.Text, password.Text, email.Text, securityQuestion.Text, securityAnswer.Text, false, out createStatus);

            switch (createStatus)
            {
                case MembershipCreateStatus.Success:
                    newUser.Comment = barNumber.Text;
                    userCreationResults.Text = "User account was created successfully. \n Please check your Email for an activation link";
                    SendActivationEmail(newUser);
                    //Roles.AddUserToRole(newUser.ToString(), "Attorney");
                    Membership.UpdateUser(newUser);
                    break;
                case MembershipCreateStatus.DuplicateUserName:
                    userCreationResults.Text = "That username is already taken, please choose another username.";
                    break;
                case MembershipCreateStatus.DuplicateEmail:
                    userCreationResults.Text = "A user with that E-mail address already exists.";
                    break;
                case MembershipCreateStatus.InvalidEmail:
                    userCreationResults.Text = "Please Enter a valid E-mail address.";
                    break;
                case MembershipCreateStatus.InvalidAnswer:
                    userCreationResults.Text = "The security answer is not valid.";
                    break;
                case MembershipCreateStatus.InvalidPassword:
                    userCreationResults.Text = "Please enter a valid Password. please enter a password with at least 7 characters, 1 number and one non-alphanumeric";
                    break;
                default:
                    userCreationResults.Text = "Unknown Error: Account NOT created.";
                    break;
            }
        }
        else
        {
            userCreationResults.Text = "Confirmation Password must match the Password";
        }
    }
    else
    {
        userCreationResults.Text = "Your Email and/or Bar Number doesn't match UJS Records.";
    }
}

the following are Text Boxes

• password
• barNumber
• email
• confirmPassword
• securityQuestion
• securityAnswer

userCreationResults is a label.

IsBarNumberAndEmailValid is obviously a Boolean method that checks to make sure the bar number and email match a attorney record in a 3rd-party application.

Answer 1

You shouldn't place this inside the button click eventhandler, but to a separate method. Assume you want to create a user at some other place, what will you do? Refactoring so the code in question is placed in a separate method or misusing the eventhandler ?

First I would add a method which returns a String based on a MembershipCreateStatus enum value.

private string GetMessage(MembershipCreateStatus status)
{
    switch (createStatus)
    {
        case MembershipCreateStatus.Success:
           return "User account was created successfully. \n Please check your Email for an activation link";
        case MembershipCreateStatus.DuplicateUserName:
            return "That username is already taken, please choose another username.";
        case MembershipCreateStatus.DuplicateEmail:
            return "A user with that E-mail address already exists.";
        case MembershipCreateStatus.InvalidEmail:
            return "Please Enter a valid E-mail address.";
        case MembershipCreateStatus.InvalidAnswer:
            return "The security answer is not valid.";
        case MembershipCreateStatus.InvalidPassword:
           return "Please enter a valid Password. please enter a password with at least 7 characters, 1 number and one non-alphanumeric";
        default:
            return "Unknown Error: Account NOT created.";
    }
}  

now let us add a method

private string CreateUser()
{
    if (!IsBarNumberAndEmailValid(email.Text, barNumber.Text)) 
    { 
        return "Your Email and/or Bar Number doesn't match UJS Records.";
    }

    if (password.Text != confirmPassword.Text)
    {
        return "Confirmation Password must match the Password";
    }

    var createStatus = new MembershipCreateStatus();

    var newUser = Membership.CreateUser(username.Text, password.Text, email.Text, securityQuestion.Text, securityAnswer.Text, false, out createStatus);

    if(createStatus == MembershipCreateStatus.Success)
    {
        newUser.Comment = barNumber.Text;
        SendActivationEmail(newUser);
        Membership.UpdateUser(newUser);
    }

    return GetMessage(createStatus);
}  

which will reduce the button click handler code to

protected void createUser_Click(object sender, EventArgs e)
{
    userCreationResults.Text = CreateUser();
}  

Answer 2

It would be slightly more readable to use early returns, with so-called guard statements, for example:

protected void createUser_Click(object sender, EventArgs e)
{
    var createStatus = new MembershipCreateStatus();

    if (!IsBarNumberAndEmailValid(email.Text, barNumber.Text))
    {
        userCreationResults.Text = "Your Email and/or Bar Number doesn't match UJS Records.";
        return;
    }
    if (password.Text != confirmPassword.Text)
    {
        userCreationResults.Text = "Confirmation Password must match the Password";
        return;
    }
    var newUser = Membership.CreateUser(username.Text, password.Text, email.Text, securityQuestion.Text, securityAnswer.Text, false, out createStatus);

    switch (createStatus)
    {
        case MembershipCreateStatus.Success:
            newUser.Comment = barNumber.Text;
            userCreationResults.Text = "User account was created successfully. \n Please check your Email for an activation link";
            SendActivationEmail(newUser);
            //Roles.AddUserToRole(newUser.ToString(), "Attorney");
            Membership.UpdateUser(newUser);
            break;
        case MembershipCreateStatus.DuplicateUserName:
            userCreationResults.Text = "That username is already taken, please choose another username.";
            break;
        case MembershipCreateStatus.DuplicateEmail:
            userCreationResults.Text = "A user with that E-mail address already exists.";
            break;
        case MembershipCreateStatus.InvalidEmail:
            userCreationResults.Text = "Please Enter a valid E-mail address.";
            break;
        case MembershipCreateStatus.InvalidAnswer:
            userCreationResults.Text = "The security answer is not valid.";
            break;
        case MembershipCreateStatus.InvalidPassword:
            userCreationResults.Text = "Please enter a valid Password. please enter a password with at least 7 characters, 1 number and one non-alphanumeric";
            break;
        default:
            userCreationResults.Text = "Unknown Error: Account NOT created.";
            break;
    }
}

Answer 3

Normally I would say that method names should be Pascal Case, meaning that createUser_Click should be CreateUser_Click

and the Comment for adding the user to the attorney role in the MembershipCreateStatus.Success case should be deleted, or it should be a valid comment like

//TODO: Add User to Attorney Role

Answer 4

I think it's too nested. A switch inside an if block should leave you with a funny smell and an urge to pull things out of there.

Also the method seems to me like it's doing too many things - I'd extract that switch block to some SetCreationResultText method, and then look into ways to get rid of the switch altogether.

Answer 5

As far as I can see, createStatus is only used as an out parameter in MemberShip.createUser(). You don't need to explicitly create a new instance for this so I wouldn't do it either. I prefer to leave this to the method itself since it has to take care of that anyway. This would result in the following code:

MembershipCreateStatus createStatus;
var newUser = Membership.CreateUser(/* params */, out createStatus);

---

IsBarNumberAndEmailValid(email.Text, barNumber.Text)

I would change this to

AreEmailAndBarNumberValid(email.Text, barNumber.Text)

to indicate a multiple and to correspond with the order of the arguments.

---

Your Membership.CreateUser contains an argument false. This doesn't tell me what it means, so I would suggest using a named argument.

Answer 6

I'm no C# expert, but because each case is so similar, wouldn't it make more sense instead to store MembershipCreateStatus codes and resulting text? Then one could use a nice, fast data structure such as a tree to generate the appropriate text from the given status.

Answer 7

You should never use variable names like newUser. This is too easy to confuse with new User.

While it is tempting to use such things, and it is the same reason the language designers chose to use new, I much prefer names like:

User userToCreate = new User(...).

Answer 8

You should create a resource file to store your strings. Then they could be changed without recompiling your program.

You also have a typo here.

"Please enter a valid Password. please enter a password with at least 7 characters, 1 number and one non-alphanumeric";

Sentences begin with a capital letter.