Java web application MVC approach needs critique

I have a servlet that invokes generic actions passing in a form and object (depending on what the action needs)

CommitmentServlet.java

CommitmentListDAO clDAO = new CommitmentListDAO();
CommitmentItemForm form = new CommitmentItemForm(clDAO);
CommitmentItem obj = new CommitmentItem();

actionMap.put(null, new ListAction(form);
actionMap.put("list", new ListAction(form);
actionMap.put("view", new ViewAction(form, obj)
actionMap.put("delete", new DeleteAction(form, obj);
actionMap.put("edit", new EditAction(form, obj);

ControllerAction action = (ControllerAction) actionMap.get(request.getParameter("method"));
action.service(request, response);

EditAction.java

public class EditAction implements ControllerAction {
  private Form form;
  private Object obj;

public EditAction(Form form, Object obj) {
    this.form = form;
    this.obj = obj;
}

public void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

    obj = form.edit(request);

    request.setAttribute("obj", obj);
    request.setAttribute("form", form);

    if (form.isSucces()) {
        RequestDispatcher view = request.getRequestDispatcher(success page);
        view.forward(request, response);
    }
    else {
        RequestDispatcher view = request.getRequestDispatcher(failure page);
        view.forward(request, response);
    }
}
}

The actual business logic is located in the form object passed in to the generic action.

The generic actions allow me to quickly get CRUD Controller functionality for any new objects. I just have to code the business logic form such as here

CommitmentItemForm.java

public Object edit(HttpServletRequest request) {
    CommitmentItem commitmentItem = null;
    STKUser authenticatedUser = (STKUser) request.getSession().getAttribute("STKUserSession");
    String ownedByBadge = null;
    List deptSupervisorList = null;

    try {
        deptSupervisorList = STKUserDAO.getList(authenticatedUser.getDepartment()); //<--- Static call is it OK??
        commitmentItem = CommitmentListDAO.retrei开发者_如何学运维ve(request.getParameter("commitment_id"), authenticatedUser);
        ownedByBadge = commitmentItem.getOwned_by();
    }
    catch (DAOException e) {
        setError(FORM_RESULTS, e.getMessage());
    }
    catch (ValidatorException e) {
        // ValidatorExceptions are thrown when the DAO can not find a record
        setError(FORM_RESULTS, e.getMessage());
        LOGGER.log(Level.INFO, e.getMessage(), authenticatedUser);
    }

    if (ownedByBadge != null) {
        if (ownedByBadge.equals(authenticatedUser.getBadge()) || ownedByBadge.equals(authenticatedUser.getAtaBadge())) {
        }
        else {
            setError(FORM_RESULTS, "You are not authorized to edit this data.");
            LOGGER.log(Level.INFO, "Error - You are not authorized to edit this data '" + commitmentItem.getCommitment_id() + "'", authenticatedUser);
        }
    }
    request.setAttribute("deptSupervisorList", deptSupervisorList);  // <--- Is this acceptable to do???
    return commitmentItem;
}

1) is my approach of setting a request attribute and returning an object in method un orthodox?

2) I'm making a static call to get the deptSupervisorList. Is this asking for trouble??

3) Does my servlet, generic action, business form seem like an acceptable approach to develop a java web application without using a framework?

EDIT: What is the difference?

Static
deptSupervisorList = STKUserDAO.getList(authenticatedUser.getDepartment()); 

vs

non-static
STKUserDAO userDAO = new STKUserDAO();
deptSupervisorList = userDAO.getList(authenticatedUser.getDepartment()); 


public static List getList(String dept) throws DAOException {
   ...
}

First some caveats:

• This is subjective
• I agree with SidCool that the answer is to take a look at some of the existing web application frameworks out there. If anything, just to find out how they do it.
• I'm a massive fan of dependency injection

To answer your questions:

• It's not great to pass data around in request attributes because: it's not type safe; it's a bit of an invisible bag of things -- always better if you can see output objects in the type signature; at some point you'll find yourself wanting to store two things in the request attributes under the same name
• Dependency injection is the way of the future. Making a static call is bad because: you've now tightly coupled the two objects making reuse harder as well as making it harder to test
• I'd definitely have a look at some other frameworks here. Most of them tend to have a single dispatch servlet, I think you'll end up writing a lot of very similar looking servlets. A lot of frameworks will also use reflection to try and get the transformation between request and POJO done as early and as easily as possible.

Other:

• All of your actions are off parameters i.e. ?method=[list,view,delete,edit]. Often it is preferable to use routes (e.g. index.html is usually used for 'list').

To answer your feedback / questions from the comments:

Running on older version of Java

Wow, that sucks. There are frameworks that run on Java 1.4, though. Spring MVC would be my recommendation but there are more here. That said, the reason that I suggested looking at other frameworks wasn't just to use them but more to be inspired by them. Writing your own Web Application Framework is practically a rite of passage and can be pretty fun. Writing it in such a constrained environment just adds to the challenge.

What I'd suggest:

• Try out a recent Java framework or even a non Java one (e.g. Ruby on Rails), just to see what's possible
• When writing your own only framework, just use 1 servlet and dispatch down to your various 'controllers'. The reason for this is that Servlets are not great at letting you put your whole application together (what Spring MVC does, is loads up the 'application' using a ContextListener and then servlets and filters look up the 'application' from the ServletContext)

The tight coupling of static

Tight coupling is when two objects can't be used without each other, ever. Why is this bad, you ask? Because you can never reuse the code for something else (say, if you decided to load some data from a file, introduced a caching layer, used it in a different project etc.). Most importantly, some would say, is that it is difficult to test it. This is because you can't just replace the object that you statically call with another one. Interfaces are usually used to decouple objects but realistically, you can do it just by setting the object in via dependency injection (which is a complicated way of saying: put it in the constructor or as a setter).

OO and being a civil engineer

It's all good. Some of the best programmers I know didn't start out that way. For me, using the Dependency Injection pattern is an awesome way to write 'good' code by default. Note: if you look at Dependency Injection, you don't need a framework for it. You just need to construct all of your objects in one place and all of your objects should get all of their dependencies either in the constructor or in a setter. Not static methods or singletons allowed.

What's the difference

An alternative 'what's the difference' that better illustrates what I mean would look like this:

// code in your application builder
// assuming an interface called UserDAO
UserDAO userDAO = new STKUserDAO();
CommitmentItemForm form = new CommitmentItemForm(userDao);


public class CommitmentItemForm {

  private final UserDAO userDao;
  public CommitmentItemForm(UserDAO userDao) { this.userDao = userDao; }

  public Object edit(HttpServletRequest request) {
    ...
    deptSupervisorList = userDao.getList(authenticatedUser.getDepartment());
    ...
  }

}

vs

public class CommitmentItemForm {

  public CommitmentItemForm()

  public Object edit(HttpServletRequest request) {
    ...
    deptSupervisorList = STKUserDAO.getList(authenticatedUser.getDepartment());
    ...
  }

}

The static method definitely looks like it's less work, so why is it so bad? Essentially, it's because in the static version, you can never look up the deptSupervisorList from anything but an STKUserDAO. In the non static version, you can supply any implementation of the UserDAO interface. This means that you could use the same CommitmentItemForm code regardless of whether:

• You were doing it in a test and you were creating a Mock version of UserDAO that returned an exception every time so that you could test that
• You found out that you needed to retrieve your departments list from a JSON HTTP REST web service, or from a file

It's also immediately obvious from the signature of CommitmentItemForm that it needs a UserDAO to function (because it's required in the constructor).

This is one of those little things that if you do it with all of your code, you will find that your code is not only more flexible, it's more testable, more reusable and the parts that you suddenly find you need to change in the future are better isolated.