Simple servlet code to sum digits

Question

This servlet takes an input from the user and

• Stores the number inside the bean but with spaces in between each digit then stores the bean in the request scope.
• If the last digit is even, sums the numbers and stores it in the request attribute. It will be displayed by even.jsp. Otherwise, the servlet forwards to odd.jsp and just displays the numbers.

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
 numberBean bean = new numberBean();
 int sum = 0;
 char[] inputNumbers = request.getParameter("input").toCharArray();
 StringBuilder inputs = new StringBuilder();
 for (char digit: inputNumbers) {
 inputs.append(digit).append(" ");
 }
 bean.setInput(inputs.toString());
 request.setAttribute("input", bean);
 if (inputNumbers[inputNumbers.length - 1] % 2 == 0) {
 for (char digit: inputNumbers) {
 sum += Character.getNumericValue(digit);
 }
 request.setAttribute("sum", sum);
 request.getRequestDispatcher("/WEB-INF/even.jsp").forward(request, response);
 } else {
 request.getRequestDispatcher("/WEB-INF/odd.jsp").forward(request, response);
 }
}

I can't help thinking that this code is too long and can be shorter and better. How can I improve this code?

Answer 1

I am not aware of how to improve the code so that it will be shorter. What I would suggest is that this code can be improved in terms of clarity of intent (in this case, the LOCs will increase). This can be achieved by using small functions and good names for the functions (for more information look and Robert C. Martins wonderful book: Clean Code or this for a little introduction).

I refactored your code by extracting some methods and giving more or less meaningful names:

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
 char[] inputNumbers = extractInputNumbersFromRequest(request);
 NumberBean bean = createNumberBeanForInputNumbers(inputNumbers);
 request.setAttribute("input", bean);
 forwardRequestToCorrectView(request, response, inputNumbers);
}
private char[] extractInputNumbersFromRequest(HttpServletRequest request) {
 return request.getParameter("input").toCharArray();
}
private NumberBean createNumberBeanForInputNumbers(char[] inputNumbers) {
 NumberBean bean = new NumberBean();
 String inputNumbersWithSpaces = combineInputNumbersWithSpaces(inputNumbers);
 bean.setInput(inputNumbersWithSpaces);
 return bean;
}
private String combineInputNumbersWithSpaces(char[] inputNumbers) {
 StringBuilder inputs = new StringBuilder();
 for (char digit: inputNumbers) {
 inputs.append(digit).append(" ");
 }
 return inputs.toString();
}
private void forwardRequestToCorrectView(HttpServletRequest request, HttpServletResponse response, char[] inputNumbers) throws ServletException, IOException {
 if (lastNumberIsOdd(inputNumbers)) {
 handleLastNumberOddCase(request, response, inputNumbers);
 } else {
 handleLastNumberEvenCase(request, response);
 }
}
private void handleLastNumberEvenCase(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
 request.getRequestDispatcher("/WEB-INF/odd.jsp").forward(request, response);
}
private void handleLastNumberOddCase(HttpServletRequest request, HttpServletResponse response, char[] inputNumbers) throws ServletException, IOException {
 request.setAttribute("sum", sumOfInputNumbers(inputNumbers));
 request.getRequestDispatcher("/WEB-INF/even.jsp").forward(request, response);
}
private int sumOfInputNumbers(char[] inputNumbers) {
 int sum = 0;
 for (char digit: inputNumbers) {
 sum += Character.getNumericValue(digit);
 }
 return sum;
}
private boolean lastNumberIsOdd(char[] inputNumbers) {
 return inputNumbers[inputNumbers.length - 1] % 2 == 0;
}

One could argue about the existence about some of the methods like handleLastNumberEvenCase, but I like the idea from Uncle Bob, that a method should not have different levels of abstractions. As such, I've introduced these alias-like methods, which just give a one liner a name, that is more clear.

In addition, the class numberBean was renamed to NumberBean to use the standard conventions in the Java world. Furthermore, it should be renamed to a more meaningful name, because the word "Number" does not give you a hint on what this class does. The word "Bean" does not give you further information as well, so I would suggest something like WhitespaceNumberRequestor - which is obviously false, because I don't have any hints on what this class does.

• lightning_missile: The NumberBean class is the java bean. It has the getInput and setInput methods.
• Mario David: with further information about the name bean i meant, that the majority of all java classes fulfil the java bean spec, but the minority of it are named like this. the question is, what additional information do you get, when you name a class with these extra suffixes like *Manager, *Controller, *Bean? If it does give you a very important hint, that this class has to have a getter, then it could be reasonable enough to name it like this. Of course its a matter of taste, but i would try to choose a name that is more self describing than *Bean

Answer 2

How about determining whether the number is an even number or not first so that you can do the String building and optionally the summation in one loop? See the use of evenNumber below...

protected void doPost( HttpServletRequest request, HttpServletResponse response ) {
 NumberBean bean = new NumberBean();
 int sum = 0;
 String input = request.getParameter( "input" );
 char[] inputNumbers = input.toCharArray();
 final boolean evenNumber = Integer.parseInt( input ) % 2 == 0;
 StringBuilder inputs = new StringBuilder();
 for ( char digit : inputNumbers ) {
 inputs.append( digit ).append( ' ' );
 if ( evenNumber ) {
 sum += Character.getNumericValue( digit );
 }
 }
 bean.setInput( inputs.toString() );
 request.setAttribute( "input", bean );
 if ( evenNumber ) {
 request.setAttribute( "sum", sum );
 }
 request.getRequestDispatcher( "/WEB-INF/" + (evenNumber ? "even" : "odd") + ".jsp" ).forward( request, response );
}

Another small change: I am appending a ' ' character to the StringBuilder purely because the method to append a character exist.

• lightning_missile: I considered placing the if statement inside the loop to determine the even numbers, however I thought the loop may become slower because in each iteration the if statement will execute but I think it only needs to execute once to find out if the input number is even. I could be wrong.
• h.j.k.: See my small edit to use final boolean evenNumber = .... I think (aka not empirically tested) when it is false, the JVM will be smart enough to side-step that if statement for us... Even if it doesn't, I think one if statement in one for loop beats two for loops still.