4
\$\begingroup\$

I wanted to work on HttpSessions and JSP.

This is the view I have:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
</head>
<body>
<div>
    <c:out value="All available products:"/>
    <br/>
    <br/>
    <c:forEach items="${applicationScope.allProducts}" var="product">
        <c:out value="${product}"/>
        &nbsp;
        <a href="${pageContext.servletContext.contextPath}/shop?addProductbyName=<c:out  value="${product}"/>">Add to basket</a>
        <br/>
    </c:forEach>
    <br/>
    Currently your cart contains these items dude:
    <br/>

    <c:forEach items="${sessionScope.cart}" var="product">
        <c:out value="${product.key}"/>
        &nbsp;
        <c:out value="${product.value}"/>
        <br/>
    </c:forEach>
</div>
</body>
</html>

And the Servlet:

package com.tugay;

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: 20/07/14
 * Time: 18:07
 */

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

    @Override
    public void init() throws ServletException {
        if (getServletContext().getAttribute("allProducts") == null) {
            getServletContext().setAttribute("allProducts", Database.getAllProducts());
        }
    }

    @SuppressWarnings("unchecked")
    @Override
    protected void doGet(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse)
            throws ServletException, IOException {
        Map<String, Integer> cart = doSessionStuff(httpServletRequest, httpServletResponse);
        String addProductbyName = httpServletRequest.getParameter("addProductbyName");
        if (addProductbyName != null) {
            if (cart.get(addProductbyName) == null) {
                cart.put(addProductbyName, 0);
            }
            cart.put(addProductbyName, cart.get(addProductbyName) + 1);
        }

        httpServletResponse.sendRedirect(getServletContext().getContextPath()+"/shopping.jsp");
    }

    @SuppressWarnings("unchecked")
    private Map<String, Integer> doSessionStuff(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse) {
        HttpSession session = httpServletRequest.getSession();
        Map<String, Integer> cartMap = null;
        if (session.getAttribute("cart") == null) {
            cartMap = new HashMap<String, Integer>();
            session.setAttribute("cart", cartMap);
        }
        cartMap = (Map<String, Integer>) session.getAttribute("cart");
        return cartMap;
    }
}

And the "Mock" Db:

package com.tugay;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

/**
 * User: koraytugay
 * Date: 20/07/14
 * Time: 17:58
 */
public class Database {

    private static HashMap<Integer,String> products = new HashMap<Integer, String>();

    static {

        products.put(1,"Table");
        products.put(2,"Sofa");
        products.put(3,"Chair");
        products.put(4,"Window");

    }

    public static List<String> getAllProducts() {
        return new ArrayList<String>(products.values());
    }

}

This is what my page looks like:

enter image description here

If anyone wants to review this, they are welcome.

\$\endgroup\$

2 Answers 2

Reset to default
5
\$\begingroup\$

Interfaces vs implementations

Always use interfaces in declarations, not implementations. Instead of:

 private static HashMap<Integer,String> products = new HashMap<Integer, String>();

Do like this:

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

doSessionStuff

The httpServletResponse parameter is unused, so drop it.

No need to initialize cartMap to null, as you will assign to it anyway. Actually you don't need that local variable at all, you could simplify the method without it:

@SuppressWarnings("unchecked")
private Map<String, Integer> doSessionStuff(HttpServletRequest httpServletRequest) {
    HttpSession session = httpServletRequest.getSession();
    if (session.getAttribute("cart") == null) {
        session.setAttribute("cart", new HashMap<String, Integer>());
    }
    return (Map<String, Integer>) session.getAttribute("cart");
}

doGet

You only use the cart variable if addProductbyName != null, so it would be better to move it inside the if block.

It's a bit awkward to do cart.put(addProductbyName, 0); and then again cart.put(addProductbyName, cart.get(addProductbyName) + 1);. And you are doing cart.get(addProductbyName) multiple times.

The @SuppressWarnings("unchecked") is also unnecessary for this method, as there are no unchecked casts.

The method would be cleaner like this:

@Override
protected void doGet(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse)
        throws ServletException, IOException {
    String addProductbyName = httpServletRequest.getParameter("addProductbyName");
    if (addProductbyName != null) {
        Map<String, Integer> cart = doSessionStuff(httpServletRequest);
        Integer count = cart.get(addProductbyName);
        if (count == null) {
            cart.put(addProductbyName, 1);
        } else {
            cart.put(addProductbyName, count + 1);
        }
    }

    httpServletResponse.sendRedirect(getServletContext().getContextPath()+"/shopping.jsp");
}
\$\endgroup\$
0
1
\$\begingroup\$

In the JSP, there are a couple areas where you are inconsistant in your style. For example, you are outputting strings via

<c:out value="All available products:"/>

vs

  Currently your cart contains these items dude:

Also, you are mixing/matching c:out and EL string interpolation in your url construction here. You can simplify it:

<a href="${pageContext.servletContext.contextPath}/shop?addProductbyName=${product}"/>">Add to basket</a>
\$\endgroup\$
4
  • \$\begingroup\$ So this is not required: <c:out value="${product}" ? \$\endgroup\$
    – Koray Tugay
    Commented Jul 20, 2014 at 18:45
  • \$\begingroup\$ Nope, just like you are able to add the contextPath in the same way. \$\endgroup\$
    – Jon Anderson
    Commented Jul 20, 2014 at 18:48
  • \$\begingroup\$ Is there any better way to make this link only with <c:url and maybe c:param ? \$\endgroup\$
    – Koray Tugay
    Commented Jul 20, 2014 at 18:51
  • \$\begingroup\$ It is quite important to stress the difference between using c:out or not. Always use c:out for dynamic data, @KorayTugay. See stackoverflow.com/questions/291031/jsp-cout-tag \$\endgroup\$
    – Simon Forsberg
    Commented Jul 20, 2014 at 19:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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