User registration Servlet

Question

I have a servlet that processes user registration to a website. It takes inputs from an HTML form like username, password, email, etc.

MySQL Table:

CREATE TABLE IF NOT EXISTS user(
    user_id VARCHAR(255),
    user_password VARCHAR(255) NOT NULL,
    user_last_name VARCHAR(255),
    user_first_name VARCHAR(255),
    user_email VARCHAR(255) UNIQUE NOT NULL,
    user_type TINYINT UNSIGNED NOT NULL, /* VALUES: 0 - Guest, 1 - Admin, 2 - User */   
    PRIMARY KEY(user_id)
);

Servlet:

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    String userId = request.getParameter("username");
    String userFirstName = request.getParameter("firstname");
    String userLastName = request.getParameter("lastname");
    String userEmail1 = request.getParameter("email1");
    String userEmail2 = request.getParameter("email2");
    String userPassword1 = request.getParameter("pass1");
    String userPassword2 = request.getParameter("pass2");
    String captchaAnswer = request.getParameter("answer");

    try {
        // simple captcha
        HttpSession session = request.getSession(true);
        Captcha captcha = (Captcha) session.getAttribute(Captcha.NAME);
        request.setCharacterEncoding("UTF-8");  

        boolean isCaptchaCorrect = captcha.isCorrect(captchaAnswer);
        session.setAttribute("isCaptchaCorrect", isCaptchaCorrect);

        session.setAttribute("userId", userId);
        session.setAttribute("userFirstName", userFirstName);
        session.setAttribute("userLastName", userLastName);
        session.setAttribute("userEmail1", userEmail1);
        session.setAttribute("userEmail2", userEmail2);

        if(isCaptchaCorrect) {
            // put database entries into a String[]
            DatabaseManipulator dm = new DatabaseManipulator();
            String[] usernameArray = dm.dbEntriesToArray("user_id");
            String[] emailArray = dm.dbEntriesToArray("user_email");

            // validate inputs
            RegistrationModule rm = new RegistrationModule();
            boolean hasDuplicateUsername = rm.hasDuplicate(usernameArray, userId);
            boolean hasDuplicateEmail = rm.hasDuplicate(emailArray, userEmail1);
            boolean isEmailMatch = rm.isMatch(userEmail1, userEmail2);
            boolean isPasswordMatch = rm.isMatch(userPassword1, userPassword2);

            // bind objects to session
            session.setAttribute("hasDuplicateUsername", hasDuplicateUsername);
            session.setAttribute("hasDuplicateEmail", hasDuplicateEmail);
            session.setAttribute("isEmailMatch", isEmailMatch);
            session.setAttribute("isPasswordMatch", isPasswordMatch);

            // throw user-defined exceptions
            if(hasDuplicateUsername) {
                try {
                    throw new UsernameAlreadyExistsException();

                } catch(UsernameAlreadyExistsException uaee) {
                    // redirect to result page
                    response.sendRedirect("register-result.jsp");
                }

            } else if(hasDuplicateEmail) {
                try {
                    throw new EmailAlreadyExistsException();

                } catch(EmailAlreadyExistsException eaee) {
                    response.sendRedirect("register-result.jsp");
                }

            } else if(!isEmailMatch) {
                try {
                    throw new MismatchedEmailsException();

                } catch(MismatchedEmailsException mee) {
                    response.sendRedirect("register-result.jsp");
                }

            } else if(!isPasswordMatch) {
                try {
                    throw new MismatchedPasswordsException();

                } catch(MismatchedPasswordsException mpe) {
                    response.sendRedirect("register-result.jsp");
                }

            // register success
            } else {
                // assign if match
                String userPassword = userPassword1;
                String userEmail = userEmail1;

                // assemble user bean object
                User user = UserAssembler.getInstance(
                    userId,
                    userPassword,
                    userLastName,
                    userFirstName,
                    userEmail,
                    2 // 2 = User
                );

                // insert user into database
                dm.registerUser(user);  

                response.sendRedirect("register-result.jsp");
            }

        // wrong captcha answer
        } else {
            response.sendRedirect("register-result.jsp");               
        }

    } catch(NullPointerException npe) {         
        // redirect when servlet is illegally accessed
        response.sendRedirect("index.jsp");
    }
}

Everything works as it should, however during a quick code review from my instructor, he commented I should not be catching NPEs. I am using the catch clause to redirect the user to index.jsp if they try to jump to the Servlet URL without going through the required pages. My other servlets are formatted similarly as well. What is the best practice if catching NPE is not encouraged?

Answer 1

What's even worse is all this:

try {
     throw new SomeException();
} catch (SomeException uaee) {
     response.sendRedirect("some-result.jsp");
}

It would be better to just do

response.sendRedirect("some-result.jsp");

Directly. There is really no need to throw an exception just to catch the same exception on the next line.

if(hasDuplicateUsername) {
    response.sendRedirect("register-result.jsp");
} else if(hasDuplicateEmail) {
    response.sendRedirect("register-result.jsp");
} else if(!isEmailMatch) {
    response.sendRedirect("register-result.jsp");
} else if(!isPasswordMatch) {
    response.sendRedirect("register-result.jsp");
}

As for the NullPointerException, I assume that it is one of these that are null:

String userId = request.getParameter("username");
String userFirstName = request.getParameter("firstname");
String userLastName = request.getParameter("lastname");
String userEmail1 = request.getParameter("email1");
String userEmail2 = request.getParameter("email2");
String userPassword1 = request.getParameter("pass1");
String userPassword2 = request.getParameter("pass2");
String captchaAnswer = request.getParameter("answer");

The fix for this is easy, check if they are null before using them:

if (userId == null || userFirstName == null || userLastName == null ||
  userEmail1 == null || ...) {
    response.sendRedirect("index.jsp");
    return;
}

Answer 2

As already stated, this is pretty bad:

try {
    throw new UsernameAlreadyExistsException();
} catch(UsernameAlreadyExistsException uaee) {
    // redirect to result page
    response.sendRedirect("register-result.jsp");
}

Moreover, do you really think, there's anyone who could make use of the comment?

What's worse: Your dedicated UsernameAlreadyExistsException is completely useless, and so are the others. The above snippet is about as good as to

try {
    throw new SQLException();
} catch(SQLException uaee) {
    response.sendRedirect("register-result.jsp");
}

Directly redirecting as suggested makes sense. However, throwing an exception is good, too, assuming you replace your blob by a few methods and don't catch the exception in the method which threw it.

---

Cacthing NPE or even catching Exception at the top level is fine, as you want to catch all problems there. Doing it anywhere else is (nearly always) wrong. But as servlets provide an error page, you should use this mechanism.

Calling a method and letting it produce an NullPointerException is wrong, when you can do a check instead.

---

If you feel like writing comments like this

// wrong captcha answer
} else {

then your method is clearly way too long.

---

user_type TINYINT UNSIGNED NOT NULL, /* VALUES: 0 - Guest, 1 - Admin, 2 - User */

That's possibly OK in a database (although even MySql has enums), but

2 // 2 = User

clearly shows that you should use an

enum UserType {GUEST, ADMIN, USER}

For the numbering, ordinal() can be used, and the numbers should appear nowhere.

Answer 3

This is just to show my approach, based on Simon's answer (which I accepted). Since some of the Strings are allowed to be null (i.e. userFirstName and userLastName), I put the required details in a String[]. Each element is then checked for a null value. If all elements are not null, proceed, else, redirect.

public boolean areElementsNull(String[] requiredUserDetails) {
    for(String detail : requiredUserDetails) {
        if(detail == null) {
            return true;
        }
    }

    return false;
}

Servlet:

String[] requiredUserDetails = {
    userId,
    userEmail1,
    userEmail2,
    userPassword1,
    userPassword2,
    captchaAnswer,          
};

RegistrationModule rm = new RegistrationModule();
boolean areElementsNull = rm.areElementsNull(requiredUserDetails);

if(!areElementsNull) {
    // code here

} else {
    response.sendRedirect("index.jsp");
}

I have also removed the user-defined exceptions. Thank you for everyone's critique.