1
\$\begingroup\$

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>
\$\endgroup\$
  • 1
    \$\begingroup\$ You'll get better reviews if you take a little time to explain what your code is supposed to do. \$\endgroup\$ – RubberDuck Sep 28 '14 at 12:51
2
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ @KorayTugay You welcome! \$\endgroup\$ – Sleiman Jneidi Sep 28 '14 at 14:54
1
\$\begingroup\$
  1. Its often a good idea in OOP to use Objects, so why dont have a Cart-Session-Bean?
  2. I think <el-ignored> is off by default.
  3. 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!
  4. 30 seconds are very tiny, we actually use 5 hours. even my online-bank uses 5 minutes.
  5. 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");
    
  6. resp.sendRedirect("shop?action=viewCart"); do not need the shop prefix.
  7. Firms like TÜV and TrustedShops discourage from have unused arguments, they like you to have blamed them, so check request.getParameterMap().size()==1.
  8. No need to set the indexes for the map.
  9. I realy like xml, therefore i love to use expression-language. If you like xml too, use <c:url var="shopBaseURL" value="/shop" />
\$\endgroup\$
  • 1
    \$\begingroup\$ I think this is 30 minutes? <session-timeout>30</session-timeout> \$\endgroup\$ – Koray Tugay Sep 29 '14 at 16:44
  • \$\begingroup\$ What do you mean by No need to set the indexes for the map. ? \$\endgroup\$ – Koray Tugay Sep 29 '14 at 16:45

Your Answer

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

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