2
\$\begingroup\$

I'm building a prototype MVC web application as a learning exercise so I'm avoiding existing frameworks. I'm interested in feedback on the following approach

The view consists of JSPs which use EL to present data stored in the appropriate scope (mostly session scope).

There is a single servlet called Controller. The job of Controller is

  1. Decide which action to take based on the input
  2. Forward to the appropriate view based on the outcome of the action taken

The following code should give the general idea

@WebServlet(name = "Controller", urlPatterns = {"/Controller"})
public class Controller extends HttpServlet {

    protected void processRequest(HttpServletRequest request, HttpServletResponse response) {
        response.setContentType("text/html;charset=UTF-8");

        ServletParams params = new ServletParams(request);
        String action = params.getString("action", "");

        try {
            switch (action) {
                case "listaddress":
                    new ListAddressAction(request, response).execute();
                    destination = "addresses.jsp";
                    break;
                case "editaddress":
                    new GetAddressAction(request, response).execute();
                    destination = "addressform.jsp";
                    break;
                case "saveaddress":
                    new SaveAddressAction(request, response).execute();
                    new ListAddressAction(request, response).execute();
                    destination = "addresses.jsp";
                    break;
            }

            request.getRequestDispatcher(destination).forward(request, response);

        } catch (ActionException ax) {
            log("Error processing action -->" + action + "<--", ex);
            throw new ServletException(ex);
        }
    }

    @Override
    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        processRequest(request, response);
    }

    @Override
    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        processRequest(request, response);
    }
}

The actions are responsible for retrieving the input parameters from the request and storing any data relevant to the view in the appropriate scope. For example, the ListAddressAction might look as follows:

public class ListAddressAction extends AbstractAction<Void> {

    public ListAddressAction(HttpServletRequest request, HttpServletResponse response) {
        super(request, response);
    }

    @Override
    protected void handleAction() throws ActionException {

        try {
            User user = getSessionUser(); // Can throw SecurityException

            List<Address> addresses = new AddressService().findByUserId(user.getId());          
            request.getSession().setAttribute("addresses", addresses);  

        } catch (ServiceException | SecurityException ex) {
            throw new ActionException(ex);
        }
    }

}

In this case, the ListAddressAction accesses the data layer via a service which in turn uses a DAO. The service layer is responsible for

  1. Getting a connection from the appropriate data source
  2. Commit/rollback of the action taken in the DAO layer
  3. Closing/handing back the connection
public class AddressService {

    private DataSource dataSource;

    public AddressService() throws ServiceException {
        try {
            dataSource = getDataSource();
        } catch (DaoException dx) {
            throw new ServiceException(dx);
        }
    }

    public List<Address> findByUserId(long userId) throws ServiceException {

        AddressDao addressDao = null;

        try {
            addressDao = getAddressDao();
            return addressDao.findByUserId(userId);
        } catch (DaoException dx) {
            throw new ServiceException(dx);
        } finally {
            close(addressDao);
        }
    }

    public void update(Address address) throws ServiceException {

        AddressDao addressDao = null;

        try {
            addressDao = getAddressDao();

            addressDao.update(address);
            addressDao.commit();
        } catch (DaoException dx) {
            addressDao.rollback();
            throw new ServiceException(dx);
        } finally {
            close(addressDao);
        }
    }

    private AddressDao getAddressDao() throws DaoException {
        try {
            Connection conn = dataSource.getConnection();
            return new AddressDao(conn);
        } catch (SqlException sx) {
            throw new DaoException(sx);
        }
    }

    // TODO move this to an abstract base class
    private DataSource getDataSource() throws DaoException {
        InitialContext ctx = null;

        try {
            ctx = new InitialContext();
        } catch (NamingException nx) {
            throw new DaoException("Failed to create initial context", nx);
        }

        if (ctx == null) {
            throw new DaoException("Failed to create initial context");
        }

        DataSource ds = null;
        try {
            // TODO get this name from global context
            ds = (DataSource) ctx.lookup("java:/comp/env/jdbc/webstore");
        } catch (NamingException nx) {
            throw new DaoException("Failed to retrieve datasource", nx);
        }

        if (ds == null) {
            throw new DaoException("Data source not found!");
        }

        return ds;
    }      

    private void close(AddressDao addressDao) throws DaoException {

        if (addressDao == null) {
            return;
        }

        addressDao.closeConnection();
    }
}

I'm interested views on the following

  1. The service layer has a bit of boiler plate in terms of managing the connections. This could easily be mishandled leading to resource leaks. Is there a better way to approach this?

  2. The handleAction() method instantiates an AddressService each time it's invoked. This should be reasonably cheap in its current form but what if the service layer objects become a bit more heavyweight, for example, maybe it needs to instantiate a pool of resources (sockets, file handles etc.)?

  3. Where would make sense to cache reference data. For example, suppose that Address has a foreign key into Country. While AddressDao could join on the country table, it might be more efficient to query only the address table and assign a country from a cached lookup table of countries?

\$\endgroup\$
  • \$\begingroup\$ This appears to be stub code. \$\endgroup\$ – 23fc9a62-56de-47fb-97b4-737890 Feb 21 '15 at 17:52
  • \$\begingroup\$ Thanks for the code format. I've replaced the elipses with actual code so each class should be compilable. \$\endgroup\$ – PhilDin Feb 21 '15 at 19:06
  • \$\begingroup\$ No problem. Looks good now. \$\endgroup\$ – 23fc9a62-56de-47fb-97b4-737890 Feb 21 '15 at 19:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.