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);        }    }}
Making this site navigation less verbose
java;mvc;http
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.