A simple web-app code - user registration. Is the layering ok?

Question

So knowing the little pieces is something different and putting them together is something different.

Here is the code where I try to follow good oop practices and 3 - layered structure.

Any review is greatly appreciated:

The form is located at registirationform.jsp.

The view layer:

public class RegistirationServlet extends HttpServlet {

    @Override
    protected void doPost(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse)
            throws ServletException, IOException {

        httpServletRequest.setCharacterEncoding("utf-8");
        httpServletResponse.setContentType("text/html;charset=utf-8");

        try {
            UserRegistirationService userRegistirationService
                    = new UserRegistirationService(new AppUserAccessObject(new DatabaseConnectionImpl()));

            String username = httpServletRequest.getParameter("username");
            String password = httpServletRequest.getParameter("password");
            String email = httpServletRequest.getParameter("email");

            if(username.equals("")
                    || password.equals("")
                        || email.equals("")){
                List<String> errors = new ArrayList<String>();
                errors.add("Can you make sure you fill all the fields?");
                httpServletRequest.setAttribute("errors",errors);

                if(!username.equals("")){
                    httpServletRequest.setAttribute("username",username);
                }

                if(!email.equals("")){
                    httpServletRequest.setAttribute("email",email);
                }

                httpServletRequest.getRequestDispatcher("/registirationform.jsp").forward(httpServletRequest, httpServletResponse);
            }

            else {
                userRegistirationService.registerAppUser(username, password, email);
            }

        } catch (SQLException e) {
            e.printStackTrace();
            List<String> errors = new ArrayList<String>();
            errors.add("Registiration was not successful. Maybe username/email was already taken?");
            httpServletRequest.setAttribute("errors",errors);
            httpServletRequest.getRequestDispatcher("/registirationform.jsp").forward(httpServletRequest, httpServletResponse);
        } catch (ClassNotFoundException e) {
            e.printStackTrace();
        }

    }
}

and the service layer:

public class UserRegistirationService {

    private AppUserAccessObject appUserAccessObject;

    public UserRegistirationService(AppUserAccessObject appUserAccessObject) {
        this.appUserAccessObject = appUserAccessObject;
    }

    public void registerAppUser(String username, String password, String email) throws SQLException {
        appUserAccessObject.insertUser(username, password, email);
    }
}

and the db layer:

public class AppUserAccessObject extends DataAccessObjectImpl implements DataAccessObject {

    public AppUserAccessObject(DatabaseConnectionImpl databaseConnection)
            throws IOException, SQLException, ClassNotFoundException {
        super(databaseConnection);
        setTableName("app_user");
    }

    public void insertUser(String username, String email, String password) throws SQLException {
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO " + tableName + "(username,email,password) VALUES(?,?,?)");
        preparedStatement.setString(1, username);
        preparedStatement.setString(2, email);
        preparedStatement.setString(3, password);
        preparedStatement.execute();
    }

}

Is all the code at correct layers?

Answer 1

I know you're asking strictly for feedback about the layers (which look fine to me), but in case you're looking for other feedback too:

if(username.equals("")
        || password.equals("")
            || email.equals("")){

You need to null check these values. getParameter can return null so you will get a NullPointerException if they don't exist.

List<String> errors = new ArrayList<String>();
errors.add("Registiration was not successful. Maybe username/email was already taken?");
httpServletRequest.setAttribute("errors",errors);

Can be done in one line if you want:

errors.add(Arrays.asList("Registiration was not successful. Maybe username/email was already taken?"));

Answer 2

I'm going to pass by the setup issue that abuzittin gillifirca mentioned in his comment, and the input sanitizing that jimmycarr pointed to, because I have little to add to that. And since you asked about the layering, I'm going to focus on the layering.

First things first: the layers are well divided. You separate the different layers without going overboard and, aside from initialisation, each layer has at most to contend with the one below and/or the one above. That's the very essence, and you have that basic idea right.

Because the basic idea is right, I'll focus on a smaller issue that may or may not trip you up later on.

The important thing to keep in mind about layering is that it is all about isolation. You require the minimum that is needed to function correctly and, in turn, you expose the minimum that is needed to provide a reliable, consistent, and useful service. This makes it easier to change bits in layer X without endangering layers Y and Z.

• UserRegistirationService (typo there, should be "registration") has a field AppUserAccessObject appUserAccessObject that can probably be a DataAccessObject instead. Unless you require specific functionality that only AppUserAccessObject provides, go through the interface instead (require less).

• AppUserAccessObject.insertUser and UserRegistirationService.registerAppUser are declared to throw SQLException, but shouldn't (expose less).

  In casu the access objects, if your project is small, this is acceptable, but still not recommended. It is the DAO implementation's job to sort out any issues it may encounter, and to translate or at least wrap exceptions it can't handle.

  For the service, though, this is a no-no. You don't want to force users of your service to deal with database protocol exceptions. What can your web layer or my console app really do at this point?

The exceptions you (declare to) throw are as much a part of your API as your return and parameters types. If returning a ResultSet would look out of place, throwing SQLException probably will be, too.

There are three main ways to deal with exceptions:

1. Log and ignore. If you don't know what to do with it, and the other two options can't help you, this is a viable option. You'll want to be careful not to ignore out of convenience, though.

2. Wrap and rethrow. The quick way out, and one many people choose when faced with checked exceptions. Wrap in a RuntimeException and bubble away. When done right, it simplifies APIs and speeds up your development. (I don't see it done right often.)

3. Analyse and decide. The hard way out. Analyse the exception you caught, check for type and error codes, and decide what to do with it. You can throw another exception type (NameTooLongException, InvalidEmailAddressException), try another route, or wait a bit and try again. This takes the most work and effort, and may be overkill for small projects.

No size fits all. Pick what you feel is appropriate in a given situation and the effort you can afford to put into it.

---

Two points not related to layering:

1. Instead of writing passwords plaintext into your database, store a salted hash of them.

2. Remember to close your PreparedStatement after use!

---

Edit:

But one question: UserRegistirationService#registerAppUser method has no checks. Should I put the null checks in this layer? or in this layer AS WELL?

Your service layer should check its input at any rate, and it should be your baseline. It is best positioned to know what input it accepts, so it should be the authority on checking it. When you write services, not all input may come from sources that you control, so you need to be defensive in checking. And even if you do control the sources, checking anyway will catch bugs quickly.

For the presentation layer, often some quick & dirty checks are also made. This lets you provide feedback to users without contacting the service layer (potentially on another physical machine). It can check that required fields are filled out, basic syntax followed, and so on. Null checks and length checks are a good choice here, but others are outside its possibilities (such as checking that a user name is not already taken).

There is some balance to be sought, though. Checking in multiple layers also gives you some duplicated code. Usually, it's not so bad, but it's something you'll want to document.