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?

• abuzittin gillifirca: Short answer: You have new DatabaseConnectionImpl() and catch (SQLException e) in your servlet class, therefore your web layer is not separated from data access layer.
• Koray Tugay: @abuzittingillifirca I am not using any dependency injection container or anything else so how can I avoid new DatabaseConnectionImpl()?
• abuzittin gillifirca: You define a ServletContextListener in the web.xml. You create your services, factories etc in its contextInitialized method, and stick them into the ServletContext with setAttribute("some.prefix.xyzService", xyzService) etc. You can then get them back in the servlet with XyzService xyzService = (XyzService)(getServletContext().getAttribute("some.prefix.xyzService"));
• Koray Tugay: @abuzittingillifirca Thanks. I will try this approach.
• abuzittin gillifirca: Of course, if you are doing this not to learn workings of the Servlet API, but to develop production systems; you can use a framework such as Spring and save yourself a lot of time. Also, after moving service etc out of your web layer, next in order is to move any passwords etc out of your application code, for example if they exist in DatabaseConnectionImpl`. Standard way to do it is using JNDI. See here for how it is done for Tomcat. Each servlet container has a way of doing this.

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?"));

• Koray Tugay: If the fields are empty, the getParameter returns "". How can they be null? Thanks for the feedback however.
• Eddie Curtis: The result will be null if the parameter you are looking for doesn't exist. If it's not currently causing you problems then it's not an issue, however it's worth noting and possibly adding in the null check anyway, because you never know how the functionality of your program might change in the future and those parameters could be missing from the request, so it's much better to handle the null case than allow for a NPE
• David Harkness: Bots and hackers are free to omit these parameters in the query, causing a (probably inocuous) NPE. Guave and Apache Commons both provide forms of Strings.isNullOrEmpty(str) for happy inlining.

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.

• Koray Tugay: Thank you for this great response. The passwords, well I was too lazy as it is only a local project and for learning purposes only. But one question: UserRegistirationService#registerAppUser method has no checks. Should I put the null checks in this layer? or in this layer AS WELL?
• JvR: I've edited an addition to the answer for that question.