MVC controller code

Question

Please review my servlet.

public class Controller extends HttpServlet {
    private static final long serialVersionUID = 1L;
    static List < MRTBean > stop1 = new ArrayList < > ();
    static List < MRTBean > stop2 = new ArrayList < > ();
    static List < MRTBean > stop3 = new ArrayList < > ();
    static List < MRTBean > stop4 = new ArrayList < > ();
    static List < MRTBean > stop5 = new ArrayList < > ();
    static List < MRTBean > stop6 = new ArrayList < > ();
    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        doPost(request, response);
    }
    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        try {
            MRTBean mrt = new MRTBean(request.getParameter("lastName"), request.getParameter("firstName"), request.getParameter("destination"));
            request.setAttribute("bean", addToList(mrt)); //this will be accessed by fare.jsp (the first jsp page)
            RequestDispatcher dispatcher = request.getRequestDispatcher("fare.jsp");
            dispatcher.forward(request, response); //go to the first jsp page to inform the user that the bean object have been added to the array list then from their the user can click to the second jsp page where information from all bean objects in the list are displayed.
        } catch (RuntimeException e) {
            RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/missing.jsp");
            dispatcher.forward(request, response);
        }
    }
    //Add each mrtBean object on the ArrayLists depending on their stopNumber
    private MRTBean addToList(MRTBean bean) {
        if (bean.getStopNumber().equals("stop1")) stop1.add(bean);
        else if (bean.getStopNumber().equals("stop2")) stop2.add(bean);
        else if (bean.getStopNumber().equals("stop3")) stop3.add(bean);
        else if (bean.getStopNumber().equals("stop4")) stop4.add(bean);
        else if (bean.getStopNumber().equals("stop5")) stop5.add(bean);
        else stop6.add(bean);
        return bean;
    }
    //view all items in the arrayList. This will not be called by the servlet rather by the second jsp page to display the items
    public static String view(String list) {
        List < MRTBean > bean = new ArrayList < > ();
        if (list.equals("stop1")) bean = stop1;
        else if (list.equals("stop2")) bean = stop2;
        else if (list.equals("stop3")) bean = stop3;
        else if (list.equals("stop4")) bean = stop4;
        else if (list.equals("stop5")) bean = stop5;
        else if (list.equals("stop6")) bean = stop6;
        else return null;
        StringBuilder display = new StringBuilder("");
        if (bean.isEmpty()) display.append("None");
        else for (MRTBean obj: bean)
        display.append(obj.getLastName() + ", " + obj.getFirstName() + " - " + obj.getDestination() + " station");
        return display.toString();
    }
    //gets the size of the arrayLists depending on the stopNumber. Again, this will not be called by this servlet but the second jsp page that will display how many MRTBean objects are in the arrayLists.
    public static int getSize(String list) {
        if (list.equals("stop1")) return stop1.size();
        else if (list.equals("stop2")) return stop2.size();
        else if (list.equals("stop3")) return stop3.size();
        else if (list.equals("stop4")) return stop4.size();
        else if (list.equals("stop5")) return stop5.size();
        else if (list.equals("stop6")) return stop6.size();
        else return 0;
    }
}

This servlet works fine but I feel I'm doing something wrong regarding the MVC architecture in accessing the values in the array lists and displaying them in the second JSP page. I originally want to put all array lists in the servlet context and access them by the JSP expression language, but I have two problems that I really don't know how to solve.

• The only way that I can think of if I want to access each value in the array list is writing a loop then do the EL like this: ${stop1[index of the item].lastName}.
• If I do the loop, I want to get the size of the arrayList so the loop can stop. The only way I can think of is importing the servlet and calling the getSize() method.

If I will do that in six arrayLists from stop1 to stop6, there will be so much Java code in my JSP page, and in MVC the JSP page is only for viewing not business logic. So I thought of putting all java code in my controller. However, the methods view and getSize will only be called by the JSP page by importing the servlet, not the servlet itself. It's like when the controller forwards to the JSP page, the JSP page still calls methods from the servlet and displaying the results, rather than getting the data from the bean. And since I already import my servlet, I don't feel putting the arrayList in the application scope anymore because the view() method can already access them.

I feel I'm violating something in the MVC architecture specially in presenting the output. Is this already in MVC architecture? If not, how can I make this controller into an MVC approach? How can I make this code better?

Answer 1

I'm not very familiar with JSP and EL (which isn't very good considering I have an exam on it in a few days), but in general Java terms you can simplify it a lot by moving the separate lists to a Map<String, List<MRTBean>> which will do the mapping for you so you won't need all these if-statements.

Initialization

static List<MRTBean> stop1 = new ArrayList<>();
static List<MRTBean> stop2 = new ArrayList<>();
static List<MRTBean> stop3 = new ArrayList<>();
static List<MRTBean> stop4 = new ArrayList<>();
static List<MRTBean> stop5 = new ArrayList<>();
static List<MRTBean> stop6 = new ArrayList<>();

to this:

private static Map<String, List<MRTBean>> mapping = new HashMap<>();

static {
    for (int i = 1; i <= 6; i++) {
        mapping.put("stop" + i, new ArrayList<MRTBean>());
    }
}

addToList

private MRTBean addToList(MRTBean bean) {
    if (bean.getStopNumber().equals("stop1")) {
        stop1.add(bean);
    } else if (bean.getStopNumber().equals("stop2")) {
        stop2.add(bean);
    } else if (bean.getStopNumber().equals("stop3")) {
        stop3.add(bean);
    } else if (bean.getStopNumber().equals("stop4")) {
        stop4.add(bean);
    } else if (bean.getStopNumber().equals("stop5")) {
        stop5.add(bean);
    } else {
        stop6.add(bean);
    }
    return bean;
}

to this:

private MRTBean addToList(MRTBean bean) {
    List<MRTBean> list = mapping.get(bean.getStopNumber());
    if(list == null) { list = mapping.get("stop6"); }
    list.add(bean);
    return bean;
}

view

    List< MRTBean> bean = new ArrayList<>();
    if (list.equals("stop1")) {
        bean = stop1;
    } else if (list.equals("stop2")) {
        bean = stop2;
    } else if (list.equals("stop3")) {
        bean = stop3;
    } else if (list.equals("stop4")) {
        bean = stop4;
    } else if (list.equals("stop5")) {
        bean = stop5;
    } else if (list.equals("stop6")) {
        bean = stop6;
    } else {
        return null;
    }

to this:

    List<MRTBean> bean = mapping.get(list);
    if (bean == null) {
        return null;
    }

getSize

public static int getSize(String list) {
    if (list.equals("stop1")) {
        return stop1.size();
    } else if (list.equals("stop2")) {
        return stop2.size();
    } else if (list.equals("stop3")) {
        return stop3.size();
    } else if (list.equals("stop4")) {
        return stop4.size();
    } else if (list.equals("stop5")) {
        return stop5.size();
    } else if (list.equals("stop6")) {
        return stop6.size();
    } else {
        return 0;
    }
}

to this:

public static int getSize(String list) {
    List<MRTBean> value = mapping.get(list);
    if (value == null) {
        return 0;
    }

    return value.size();
}

Other notes

There's excessive spacing here which you'll notice I removed earlier

static List < MRTBean > stop1 = new ArrayList < > ();

---

I prefer to chain these statements together:

RequestDispatcher dispatcher = request.getRequestDispatcher("fare.jsp");
dispatcher.forward(request, response);

becomes

request.getRequestDispatcher("fare.jsp").forward(request, response);

---

It's a good thing to always use braces at for, if, while etc, even when they aren't necessary. They'll cost you 4 extra characters but also prevent you from making a logical error in the future.

---

Your code isn't indented either but I assume this is a result of the copy-pasting here in the editor.

Answer 2

The control flow for addToList() doesn't look optimal: if any of the first five lists cannot be added, the sixth list will be added automatically. That would not be ideal, in case you have an invalid stopNumber. You may have to have an additional path in such a case, or throw an exception.

It also looks less-maintainable since you'll have to modify both the last else if and the else if you ever need to add or remove lists.