14
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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();
}  
\$\endgroup\$
5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ The idea behind this (using guard statements) was something new to me but nevertheless a fantastic way of building readable code. Flawless @janos :) \$\endgroup\$ – Kristofer Gisslén Dec 19 '14 at 15:42
4
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ it is correct english to use isEmailAndBarNumberValid as in "is the email and bar number valid?" rather than "Are the email and bar number valid?" if there were multiple emails or bar numbers then are would be the right form of the word be to use. otherwise I agree with the rest of the answer. \$\endgroup\$ – Malachi Dec 17 '14 at 21:51
  • \$\begingroup\$ @Malachi nope - "email and bar number are valid," not "email and bar number is valid." You still need the plural when referring to two distinct things, e.g. "Are Harry and Bob coming?" As opposed to "Is Harry and Bob coming?" Which is just plain wrong. \$\endgroup\$ – Ant P Dec 18 '14 at 15:54
  • \$\begingroup\$ @AntP Says who? you wouldn't say "the email are valid" or "the bar number are valid" you would say "the email is valid" or "the bar number is valid" so the inquisitive form of the sentence would be "is the bar number valid?" or "is the email valid?" to create "is the email and bar number valid?" \$\endgroup\$ – Malachi Dec 18 '14 at 17:49
  • \$\begingroup\$ That's not how it works. The email and bar number - together - are not an "it". They are a "they" (or should I say, "they is a they"?). To be honest, this is so basic and unambiguous that I am surprised it is even being contested. \$\endgroup\$ – Ant P Dec 18 '14 at 17:52
  • \$\begingroup\$ english.stackexchange.com/q/215683/48140 \$\endgroup\$ – Malachi Dec 18 '14 at 17:56
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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(...).
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.