patternjavaModerate
MVC controller code
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);
```
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
Initialization
to this:
addToList
to this:
view
to this:
getSize
to this:
Other notes
There's excessive spacing here which you'll notice I removed earlier
I prefer to chain these statements together:
becomes
It's a good thing to always use braces at
Your code isn't indented either but I assume this is a result of the copy-pasting here in the editor.
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.