HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

MVC controller code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codemvccontroller

Problem

Please review my servlet.

```
public class Controller extends HttpServlet {
private static final long serialVersionUID = 1L;
static List stop1 = new ArrayList ();
static List stop2 = new ArrayList ();
static List stop3 = new ArrayList ();
static List stop4 = new ArrayList ();
static List stop5 = new ArrayList ();
static List 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);

Solution

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> which will do the mapping for you so you won't need all these if-statements.

Initialization

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


to this:

private static Map> mapping = new HashMap<>();

static {
    for (int i = 1; i ());
    }
}


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 list = mapping.get(bean.getStopNumber());
    if(list == null) { list = mapping.get("stop6"); }
    list.add(bean);
    return bean;
}


view

List 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 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 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  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.

Code Snippets

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<>();
private static Map<String, List<MRTBean>> mapping = new HashMap<>();

static {
    for (int i = 1; i <= 6; i++) {
        mapping.put("stop" + i, new ArrayList<MRTBean>());
    }
}
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;
}
private MRTBean addToList(MRTBean bean) {
    List<MRTBean> list = mapping.get(bean.getStopNumber());
    if(list == null) { list = mapping.get("stop6"); }
    list.add(bean);
    return bean;
}
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;
    }

Context

StackExchange Code Review Q#52194, answer score: 11

Revisions (0)

No revisions yet.