Turnleaf Design Ramblings of a junior developer

Web developers should hate http

I don't mean that literally, what I do mean is a web developer should hate when Java's http specification is directly accessed in the code. This is a topic I briefly touched on in my 8 signs your code sucks post awhile back. I decided to return to this topic because it touches on a very important and fundamental programming practice which is limiting dependencies in code. This article represents the initial entry into a new series I plan on covering called "Java don't care." As aforementioned this series will focus on reducing dependencies in code, as well as making your code less aware of the environment it is in, and how following these practices will help you as a developer. But back to the task at hand; why referencing Java's http specification is bad.

The problems

A long list likely could be made as to why you should not directly reference http in your code. Here are four of the more important:

Testing

Probably the most readily noticable and debately the most important; referencing http in code greatly increases the difficultly of writing tests. To even begin writing a unit test you will need a tool to mock the http request/session. This increase in the difficulty of writing unit tests leads to two problems; decreased unit test coverage and increased unit test complexity.

Contract

A somewhat overlooked issue, but important none the less, is the lack of a contract between the view and the backend code that supports it. There is no central location to figure out what is contained in the request/session. The HttpServletRequest does not (and could not) provide this information. This lack of a contract can make it difficult to understand what is happening in the application as well as what the developer has access to.

Coupling

Directly accessing the http specification makes the code more aware of a view's internal implementations than it should be. The controller, and certainly the model, should not know the names of the various fields and variables in the view. It creates a mud ball of an application where changes to one level unduly necessitate changes to other levels.

You also introduce an unnecessary requirement into the application. Obviously requiring a web based application to include the httpservlet library in its classpath isn't particularly burdesom, but in other areas it could represent an issue. In general it is a bad programming practice to force requirements on an application.

Maintainability

Fundamentally this is more of a meta-issue, it is the inevitable outcome of the above three issues (as well as others). When you have an application that is difficult to write unit test for, hard to figure out what is accessible where, and has unnecessary requirements to work, you have an application that is difficult to maintain. I would argue that writing a maintainable application is the second most important requirement of an application (the first of course being one that it meets business requirements).

How to fix it

There are many differnet ways to fix and/or prevent this issue from occurring. Many frameworks, like Spring, (can) handle the session and request for you. This would also typically be the best way of addressing the issue. Let's assume, for the purpose of this article, that such an option is not available. Below is the controller of a simple application that I wrote that makes extensive use of the http request.

public class NewUserFormController extends HttpServlet {

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
String nextPage = createUser(req);
req.getRequestDispatcher(nextPage).forward(req, resp);
}

private String createUser(HttpServletRequest req) {
List errorMsg = new ArrayList();
UserDao dao = new UserDaoImpl();
validateUserInput(req,
errorMsg);
if(!dao.doesUserExist(req, errorMsg)){
dao.insertUser(req, errorMsg);
}
if(errorMsg.isEmpty()){
return "Complete.jsp";
} else{
req.setAttribute("errorMsg", errorMsg);
setRequestAttributes(req);
return "Form.jsp";
}
}

private void setRequestAttributes(HttpServletRequest req) {
String fName = req.getParameter("fName");
String lName = req.getParameter("lName");
String eMail = req.getParameter("eMail");
req.setAttribute("fName", fName);
req.setAttribute("lName", lName);
req.setAttribute("eMail", eMail);
}

private void validateUserInput(HttpServletRequest req, List errorMsg) {
String fName = req.getParameter("fName");
String lName = req.getParameter("lName");
String eMail = req.getParameter("eMail");
String password = req.getParameter("password");
String confirmPassword = req.getParameter("confirmPassword");
if (isEmpty(fName)) {
errorMsg.add("Must enter a first name!");
}
if (isEmpty(lName)) {
errorMsg.add("Must enter a last mame!");
}
if (isEmpty(eMail)) {
errorMsg.add("Must enter an email address!");
}
if (isEmpty(password)) {
errorMsg.add("Must enter a password!");
}
if (isEmpty(confirmPassword)) {
errorMsg.add("Must confirm password!");
}
if(!password.equals(confirmPassword)){
errorMsg.add("Passwords must match!");
}
}

private boolean isEmpty(String string) {
return string == null || string.trim().equals("");
}
}

Entire source of the project can be viewed here

As you can see the access to the HttpServletRequest permeates near the entireity of the code base. As a result many of the issues I brought up are problems in this code (coincedence?). It would be difficult to write unit tests for this code, it's somewhat difficult to know what information is on the request, and I am dependant on the httpservlet in my code.

The fix

So clearly this code needs help. Our refacotring of the code has two major goals:
1. Create a contract between the view and the rest of the application
2. Reduce the depence on the http specification in the code

Typically in this situation I create a bean that holds all the fields in the request/session as well as a few additional meta fields (such as in this example the next page). This creates the previously mentioned contract. A developer can look at the bean class and know what fields are accessible in the application.

The usage of this bean will help accomplish goal number two. Instead of accessing the http request to get the data we need, instead I will dump all the data into the bean and access the bean and then reference it instead.

So here is what the new controller class looks like

public final class NewUserFormController {

private NewUserFormController(){

}

protected static String createUser(UserFormBean bean) {
UserDao dao = new UserDaoImpl();
validateUserInput(bean);
if(!dao.doesUserExist(bean)){
dao.insertUser(bean);
}
if(bean.getErrorMsg().isEmpty()){
return FormConstants.COMPLETE_PAGE;
} else{
return FormConstants.FORM_PAGE;
}
}

private static void validateUserInput(UserFormBean bean) {
if (isEmpty(bean.getfName())) {
bean.addErrorMsg("Must enter a first name!");
}
if (isEmpty(bean.getlName())) {
bean.addErrorMsg("Must enter a last mame!");
}
if (isEmpty(bean.geteMail())) {
bean.addErrorMsg("Must enter an email address!");
}
if (isEmpty(bean.getPassword())) {
bean.addErrorMsg("Must enter a password!");
}
if (isEmpty(bean.getConfirmPassword())) {
bean.addErrorMsg("Must confirm password!");
}
if(!bean.getPassword().equals(bean.getConfirmPassword())){
bean.addErrorMsg("Passwords must match!");
}
}

private static boolean isEmpty(String string) {
return string == null || string.trim().equals("");
}
}

As can be seen the HttpServlet dependency has been completely removed from this code. The code is a bit simpler as I removed pretty much all local variables and use the bean instead. However the best part is I can much more easily write unit tests (which I did).

So what does this mean to me?

Well these changes fix much of the four problems I mentioned above. However there is a couple other addtional benefits:

Increased modularity

If a new form was created that had a lot the same fields, but a few additional ones (say city and address), instead of writing a bunch of new code to handle this form, instead these classes; the bean and the controller (removing the final modifier) could be reused. The developer then only needs to write code and test for the new fields.

Separation of responsibilities

With a contract in place there is less of a requirement for the developer writing the backend to be involved in writing the view portion of the application (or vice versa). This allows developers (or designers) to better play to their respective strengths and makes parallel development easier.

You can view all the source code for the entire project here (revision 13 is the last revision relevant to this article). The project also requires the Java servlet package (2.3+)

Technorati Tags: Best practices, Java, Programming, Unit testing

Related posts:

1. An Intro into Test Driven Development with JUnit4
2. 8 Signs your code sucks