A Java Web App with Servlets, JSP and Session: shopping cart example

Question

This is the only Servlet I have:

package com.tugay.shopping;

import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

/**
 * User: koraytugay
 * Date: 28/09/14
 * Time: 14:29
 */

@WebServlet(name = "shoppingServlet", urlPatterns = "/shop")
public class ShoppingServlet extends HttpServlet {

    final Map<Integer, String> products = new HashMap<>();

    @Override
    public void init() throws ServletException {
        products.put(1, "Sandpaper");
        products.put(2, "Nails");
        products.put(3, "Glue");
        products.put(4, "Paint");
        products.put(5, "Tape");
    }

    @Override
    protected void doGet(HttpServletRequest req, HttpServletResponse resp)
            throws ServletException, IOException {

        String action = req.getParameter("action");
        if (action == null)
            action = "browse";

        switch (action) {
            case "addToCart":
                this.addToCart(req, resp);
                break;
            case "viewCart":
                this.viewCart(req, resp);
                break;
            case "empty":
                this.empty(req, resp);
                break;
            case "browse":
                this.browse(req, resp);
                break;
        }
    }

    private void empty(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        req.getSession().removeAttribute("cart");
        browse(req, resp);
    }

    @SuppressWarnings("unchecked")
    private void viewCart(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        Map<String, Integer> cart = (Map<String, Integer>) req.getSession().getAttribute("cart");
        req.setAttribute("cart", cart);
        req.getRequestDispatcher("/WEB-INF/jsp/view/viewCart.jsp")
                .forward(req, resp);
    }

    @SuppressWarnings("unchecked")
    private void addToCart(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        Integer productId = Integer.parseInt(req.getParameter("productId"));

        HttpSession session = req.getSession();
        if (session.getAttribute("cart") == null) {
            session.setAttribute("cart", new HashMap<String, Integer>());
        }

        Map<String, Integer> cart = (Map<String, Integer>) session.getAttribute("cart");

        if (cart.containsKey(products.get(productId))) {
            cart.put(products.get(productId), cart.get(products.get(productId)) + 1);
        } else
            cart.put(products.get(productId), 1);

        resp.sendRedirect("shop?action=viewCart");

    }

    private void browse(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        req.setAttribute("products", products);
        req.getRequestDispatcher("/WEB-INF/jsp/view/browse.jsp").forward(req, resp);
    }
}

viewCart.jsp:

<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
</head>
<body>
<h1>You have these items in your cart:</h1>
<c:forEach items="${requestScope.cart}" var="cartItem">
    ${cartItem.key}  - ${cartItem.value}
    <br />
</c:forEach>
<a href="<c:url value="/shop"><c:param name="action" value="browse" /></c:url>">Back to Browse</a>
<a href="<c:url value="/shop"><c:param name="action" value="empty" /></c:url>">Empty Cart</a>
</body>
</html>

browse.jsp:

<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
</head>
<body>
<h1>Click on the product to add product to your cart:</h1>
<c:forEach items="${requestScope.products}" var="product">
    <a href="<c:url value="/shop">
        <c:param name="action" value="addToCart"/>
        <c:param name="productId" value="${product.key}"/>
        </c:url>">${product.value}</a>
    <br/>
</c:forEach>

</body>
</html>

web.xml:

<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee
                             http://xmlns.jcp.org/xml/ns/javaee/web-app_3_1.xsd"
         version="3.1">

    <jsp-config>
        <jsp-property-group>
            <url-pattern>*.jsp</url-pattern>
            <el-ignored>false</el-ignored>
            <page-encoding>UTF-8</page-encoding>
            <scripting-invalid>false</scripting-invalid>
            <include-prelude>/WEB-INF/jsp/base.jsf</include-prelude>
            <trim-directive-whitespaces>true</trim-directive-whitespaces>
            <default-content-type>text/html</default-content-type>
        </jsp-property-group>
    </jsp-config>

    <session-config>
        <session-timeout>30</session-timeout>
        <cookie-config>
            <http-only>true</http-only>
        </cookie-config>
        <tracking-mode>COOKIE</tracking-mode>
    </session-config>

</web-app>

You'll get better reviews if you take a little time to explain what your code is supposed to do. — RubberDuck, Commented Sep 28, 2014 at 12:51

janos · Accepted Answer · 2014-09-28 14:15:42Z

It would seem more natural to initialize the products map in the constructor instead of an init method:

private final Map<Integer, String> products;

public ShoppingServlet() {
    products = new HashMap<>();
    products.put(1, "Sandpaper");
    products.put(2, "Nails");
    products.put(3, "Glue");
    products.put(4, "Paint");
    products.put(5, "Tape");
}

Instead of checking if action is null and setting to a default, you could add a default in the switch to handle that:

String action = req.getParameter("action");

switch (action) {
    case "addToCart":
        this.addToCart(req, resp);
        break;
    case "viewCart":
        this.viewCart(req, resp);
        break;
    case "empty":
        this.empty(req, resp);
        break;
    case "browse":
    default:
        this.browse(req, resp);
}

Instead of duplicating string literals like "cart" in multiple places, it would be better to use constant variables, for example:

private static final String KEY_CART = "cart";

This would make future changes easier and avoid some errors, and also help with typing, because you can autocomplete variable names.

Sleiman Jneidi · Accepted Answer · 2014-09-28 14:30:22Z

3

You need to be concerned about thread safety in web applications, HashMap is not thread safe so you can use the Concurrent version.

private final Map<Integer, String> products = new ConcurrentHashMap<>();

I added private to the declaration, because sharing mutable state is not a good idea.

Integer productId = Integer.parseInt(req.getParameter("productId"));// DODGY!

This code here is a bit dodgy, you shouldnt never trust values you getting from the request, so you got to handle the exception and refuse the request if the value is not parsable to Integer

answered Sep 28, 2014 at 14:30

Sleiman Jneidi

3,23311 silver badges20 bronze badges

\$\begingroup\$ @KorayTugay You welcome! \$\endgroup\$
– Sleiman Jneidi
Commented Sep 28, 2014 at 14:54

Add a comment |

Grim · Accepted Answer · 2014-09-30 09:32:56Z

Its often a good idea in OOP to use Objects, so why dont have a Cart-Session-Bean?
I think <el-ignored> is off by default.
ConcurrentHashMap ok, but afaik we only read from products... So i guess no loop will throw a ConcurrentModificationException because all put is done. You better use Hashtable because it does not allow a null as value. But on the cart you realy shall use the ConcurrentHashMap!
~~30 seconds are very tiny, we actually use 5 hours. even my online-bank uses 5 minutes.~~

Keep care of async calles like SleimanJneidi said because if ....

if (session.getAttribute("cart") == null) {
    session.setAttribute("cart", new HashMap<String, Integer>());
}
// <!-- ... someone call "empty" right here, you have a NPE then!!!!
Map<String, Integer> cart = (Map<String, Integer>) session.getAttribute("cart");

resp.sendRedirect("shop?action=viewCart"); do not need the shop prefix.
Firms like TÜV and TrustedShops discourage from have unused arguments, they like you to have blamed them, so check request.getParameterMap().size()==1.
No need to set the indexes for the map.
I realy like xml, therefore i love to use expression-language. If you like xml too, use <c:url var="shopBaseURL" value="/shop" />

I think this is 30 minutes? <session-timeout>30</session-timeout> — Koray Tugay, Commented Sep 29, 2014 at 16:44
What do you mean by No need to set the indexes for the map. ? — Koray Tugay, Commented Sep 29, 2014 at 16:45

Stack Exchange Network

A Java Web App with Servlets, JSP and Session: shopping cart example

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged
java
servlets
e-commerce
jsp
or ask your own question.

Hot Network Questions

A Java Web App with Servlets, JSP and Session: shopping cart example

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javaservletse-commercejsp or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
java
servlets
e-commerce
jsp
or ask your own question.