4
\$\begingroup\$

I have this one controller that serves up all the webpages and any dynamic content. Is this the most efficient way of doing this? Can it be improved?

package uk.co.morleys;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/**
 * Servlet implementation class HomeController
 */
@WebServlet("/HomeController")
public class HomeController extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private TestimonialService testimonialService;
    private PriceService priceService;
    private String page, views;
    private int pageNum;
    List<Testimonial> testimonialList;
    List<Price> priceList;
    List<Paginator> paginationList  = new ArrayList<Paginator>();
    RequestDispatcher rd;


    public HomeController(){
        testimonialService  = new TestimonialService();
        priceService        = new PriceService();
        views               = "/WEB-INF/views/";
    }

    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        page    = request.getParameter("page");
        pageNum = (request.getParameter("pageNum") == null )? 1: Integer.parseInt(request.getParameter("pageNum"));
        int perPage = 6;
        if(page != null){
            switch (page){
            case "home":

                // get list of testimonials
                testimonialList = testimonialService.getTestimonials(2);

                // pass list of testimonials to the request object
                request.setAttribute("testimonialList", testimonialList);

                rd = getServletContext().getRequestDispatcher(views + "index.jsp");
                rd.forward(request, response);
                break;
            case "about":

                // get list of prices
                priceList = priceService.getPrices();

                // pass list of prices to the request object
                request.setAttribute("priceList", priceList);

                rd = getServletContext().getRequestDispatcher(views + "about.jsp");
                rd.forward(request, response);
                break;
            case "contact":
                rd = getServletContext().getRequestDispatcher(views + "contact.jsp");
                rd.forward(request, response);
                break;
            case "theory":
                rd = getServletContext().getRequestDispatcher(views + "theory.jsp");
                rd.forward(request, response);
                break;
            case "testimonials":

                // get list of testimonials
                testimonialList = testimonialService.getTestimonials(pageNum, perPage);

                // get pagination details
                paginationList = testimonialService.getPaginationDetails(pageNum, perPage);

                // pass list of testimonials to the request object
                request.setAttribute("paginationList", paginationList);
                request.setAttribute("testimonialList", testimonialList);
                request.setAttribute("perPage", perPage);

                rd = getServletContext().getRequestDispatcher(views + "testimonials.jsp");
                rd.forward(request, response);
                break;
            case "":
                rd = getServletContext().getRequestDispatcher(views + "index.jsp");
                rd.forward(request, response);
                break;
            case " ":
                rd = getServletContext().getRequestDispatcher(views + "index.jsp");
                rd.forward(request, response);
                break;
            default:
                rd = getServletContext().getRequestDispatcher(views + "index.jsp");
                rd.forward(request, response);
                break;
            }
        }else{
            rd = getServletContext().getRequestDispatcher(views + "index.jsp");
            rd.forward(request, response);
        }
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I'm not familiar with web server development in Java, but on the first look, I'd suggest using a framework that handles routing for you. \$\endgroup\$
    – RoToRa
    Jul 19, 2014 at 17:56

1 Answer 1

7
\$\begingroup\$

First, a few points on case statements. The last three cases all do the same thing. To reduce duplication, you can allow cases to fall through:

case "":
case " ":
default:
    rd = getServletContext().getRequestDispatcher(views + "index.jsp");
    rd.forward(request, response);
    break;

Since, you are falling through to the default, you can just omit the other cases completely, and it will still work. After the rest of the review, this will become less relevant, but added here for future reference.


The biggest problem in this method is duplication. Every case statement, and the else clause execute some form of the following:

rd = getServletContext().getRequestDispatcher(views + "<PageFilename>.jsp");
rd.forward(request, response);

Over and over and over we see this. Let's do some basic refactoring to clean this up a bit. First, we extract it out of the case statement:

private Set<String> PAGES = new HashSet<String>(Arrays.asList(
    "about", "contact", "theory", "testimonials"
));
...
if(page != null) {
    switch (page){
    case "home":
       testimonialList = testimonialService.getTestimonials(2);
       request.setAttribute("testimonialList", testimonialList);
       break;
    case "about":
        priceList = priceService.getPrices();
        request.setAttribute("priceList", priceList);
        break;
    case "testimonials":
        testimonialList = testimonialService.getTestimonials(pageNum, perPage);
        paginationList = testimonialService.getPaginationDetails(pageNum, perPage);

        request.setAttribute("paginationList", paginationList);
        request.setAttribute("testimonialList", testimonialList);
        request.setAttribute("perPage", perPage);
        break;
    }

    String pageFilename = PAGES.contains(page) ? page : "index";
    rd = getServletContext().getRequestDispatcher(views + pageFilename + ".jsp");
    rd.forward(request, response);
} else {
    ...
}

We've extracted out the duplication, created a structure to abstract out picking the file name to use, and remove empty cases. You'll note that I've also removed all the comments. This is not just for brevity; the comments restated what the code did, and were thus superfluous. We can also just get rid of the if(page != null) { ... } else { ... } as there is the same duplication there:

switch (page){
    ...
}

String pageFilename = (page != null && PAGES.contains(page)) ? page : "index";
rd = getServletContext().getRequestDispatcher(views + pageFilename + ".jsp");
rd.forward(request, response);

It's better, but that ternary statement to get pageFilename is getting a little gnarly, let's extract a helper method:

private String getPageFilename(String page) {
    if(page == null) {
        return "index.jsp";
    }

    if(!PAGES.contains(page)) {
        return "index.jsp";
    }

    return page + ".jsp";
}

And our code becomes:

rd = getServletContext().getRequestDispatcher(views + getPageFilename(page));
rd.forward(request, response);

Now let's take a moment to talk about instance variables. You have a bunch of them, and yet they only act as local variables in your method. If this is their whole purpose, we need to just make them local variables. I'm talking about:

private String page;
private int pageNum;
List<Testimonial> testimonialList;
List<Price> priceList;
List<Paginator> paginationList  = new ArrayList<Paginator>();
RequestDispatcher rd;

Unless these serve some other purpose in their lives, we don't need them as instance variables. Ironically, there is one local variable that should be higher in scope:

int perPage = 6;

This should probably be a static final int PER_PAGE = 6 in the class, since it behaves as a class constant.

Here is the code as it stands now:

@WebServlet("/HomeController")
public class HomeController extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private static final int PER_PAGE = 6;
    private Set<String> PAGES = new HashSet<>(Arrays.asList(
        "about", "contact", "theory", "testimonials"
    ));

    private TestimonialService testimonialService;
    private PriceService priceService;
    private String views;

    public HomeController(){
        testimonialService  = new TestimonialService();
        priceService        = new PriceService();
        views               = "/WEB-INF/views/";
    }

    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        String page = request.getParameter("page");
        int pageNum = (request.getParameter("pageNum") == null)? 1 : Integer.parseInt(request.getParameter("pageNum"));

        switch (page){
        case "home":
            List<Testimonial> testimonialList = testimonialService.getTestimonials(2);
            request.setAttribute("testimonialList", testimonialList);
            break;
        case "about":
            List<Price> priceList = priceService.getPrices();
            request.setAttribute("priceList", priceList);
            break;
        case "testimonials":
            List<Testimonial> testimonialList = testimonialService.getTestimonials(pageNum, PER_PAGE);
            List<Paginator> paginationList = testimonialService.getPaginationDetails(pageNum, PER_PAGE);

            request.setAttribute("paginationList", paginationList);
            request.setAttribute("testimonialList", testimonialList);
            request.setAttribute("perPage", perPage);
            break;
        }

        RequestDispatcher rd = getServletContext().getRequestDispatcher(views + getPageFilename(page));
        rd.forward(request, response);
    }

    private String getPageFilename(String page) {
        if(page == null) {
            return "index.jsp";
        }

        if(!PAGES.contains(page)) {
            return "index.jsp";
        }

        return page + ".jsp";
    }
}

I'm going to stop here. The code is shorter, and duplication has been removed. If I were writing this code myself, though, I would keep refactoring. The switch statement would be replaced with strategy objects. The getPageFilename method would be extracted into a separate class, and would use a proper configuration pattern rather than just a HashSet. There might also be one or two more helper methods to pull out. I would also look into injecting the dependencies instantiated in the constructor, though this might not be possible based on the framework being used.

\$\endgroup\$
6
  • \$\begingroup\$ I can't thank you enough. Your code review contains so much useful information, much of which I'd never used before. Thanks so much! \$\endgroup\$
    – crmepham
    Jul 20, 2014 at 10:38
  • \$\begingroup\$ One quick question on the testimonialList because it is duplicated twice should I move it up above the switch statement? \$\endgroup\$
    – crmepham
    Jul 20, 2014 at 10:39
  • \$\begingroup\$ Since I would continue making other refactorings that would split that logic out, I would leave the two declarations as they are. Even if I wasn't making any more changes, I would probably still leave them in because it really shows that those variables are specific to those case blocks and aren't used elsewhere in the method. \$\endgroup\$
    – cbojar
    Jul 21, 2014 at 1:10
  • \$\begingroup\$ I'm now getting a java.lang.NullPointerException for when page is null on the switch statement. If I remove the switch entirely it loads the index.jsp page fine. Shouldnt the switch statement just ignore it if it is null? \$\endgroup\$
    – crmepham
    Jul 22, 2014 at 10:50
  • \$\begingroup\$ Also if the page is null I need it to go to the home case to deliver some dynamic content. \$\endgroup\$
    – crmepham
    Jul 22, 2014 at 10:53

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.