Making this site navigation less verbose

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 testimonialList;
List priceList;
List paginationList = new ArrayList();
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":

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 + ".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 PAGES = new HashSet(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 testimonialList;
List priceList;
List paginationList  = new ArrayList();
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 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 testimonialList = testimonialService.getTestimonials(2);
request.setAttribute("testimonialList", testimonialList);
break;
case "about":
List priceList = priceService.getPrices();
request.setAttribute("priceList", priceList);
break

StackExchange Code Review Q#57445, answer score: 7