JettyGwtSpringSample web application

Question

I just created a sample project using Spring 3.x, Jetty 9.x, GWT 2.5.1 and Gradle here. It's a slightly modified version of the 'Greeting' GWT application from Google, the only change being that RPC calls are handled by a spring controller.

It would be really great if someone can review it, I am particularly apprehensive of injecting the GwtRpcController into the GreetingServiceImpl. Should I be worried about any thread safety issues?

Main Controller

@Controller
public class GwtRpcController extends RemoteServiceServlet {
@Autowired 
private ServletContext servletContext;
@Autowired
private RemoteService remoteService;
private Class<?> remoteServiceClass;
private HttpServletRequest request;
@RequestMapping(value = "/jettygwtspringsampleapp/*.rpc")
public ModelAndView handleRequest( HttpServletRequest request, HttpServletResponse response ) throws Exception {
 super.doPost( request, response );
 return null;
}
@Override
public String processCall( String payload ) throws SerializationException {
 try {
 RPCRequest rpcRequest = RPC.decodeRequest( payload, this.remoteServiceClass );
 // delegate work to the spring injected service
 return RPC.invokeAndEncodeResponse( this.remoteService, rpcRequest.getMethod(), rpcRequest.getParameters() );
 } catch ( IncompatibleRemoteServiceException ex ) {
 getServletContext().log( "An IncompatibleRemoteServiceException was thrown while processing this call.", ex );
 return RPC.encodeResponseForFailure( null, ex );
 }
}
@Override
public ServletContext getServletContext() {
 return servletContext;
}
public void setServletContext( ServletContext servletContext ) {
 this.servletContext = servletContext;
}
public void setRemoteService( RemoteService remoteService ) {
 this.remoteService = remoteService;
 this.remoteServiceClass = this.remoteService.getClass();
}
public HttpServletRequest getThreadLocalHttpRequest() {
 return getThreadLocalRequest();
}
}

Service implementation

@Configuration
public class GreetingServiceImpl implements GreetingService {
@Autowired
GwtRpcController rpcController;
public String greetServer( String input ) throws IllegalArgumentException {
 // Verify that the input is valid.
 if ( !FieldVerifier.isValidName( input ) ) {
 // If the input is not valid, throw an IllegalArgumentException back to
 // the client.
 throw new IllegalArgumentException( "Name must be at least 4 characters long" );
 }
 String serverInfo = rpcController.getServletContext().getServerInfo();
 String userAgent = rpcController.getThreadLocalHttpRequest().getHeader( "User-Agent" );
 // Escape data from the client to avoid cross-site script vulnerabilities.
 input = escapeHtml( input );
 userAgent = escapeHtml( userAgent );
 return "Hello, " + input + "!<br><br>I am running " + serverInfo + ".<br><br>It looks like you are using:<br>" + userAgent;
}
.....
.....

Answer 1

Just some generic note about the code:

1. If the parameter called input contains a name you should rename it to name to express its purpose.

2. Method names should be verbs. I would rename greetServer to serveGreeting or doGreeting. (Check Code Conventions for the Java Programming Language, Naming Conventions and Clean Code, page 25.)

3. Comments like this are rather just noise:

   // If the input is not valid, throw an IllegalArgumentException back to
   // the client.
   

   It's pretty obvious from the code itself, so I'd remove the comment. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

4. I'd move the complete validation logic to the FieldVerifier class (or another helper class or helper method) for better reusability and higher abstraction. It also would make the greetServer method more compact.

5. The isValidName could be a little bit shorter with the null-safe Apache Commons Lang StringUtils.length:

   public static boolean isValidName(String name) {
    return StringUtils.length(name) > 3;
   }
   

6. Usage of Google Guava would make checkName more compact:

   public static void checkName(final String name) {
    checkArgument(isValidName(name), 
    "Name must be at least 4 characters long");
   }
   

   (See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

7. public String greetServer(String name) throws IllegalArgumentException {
    ...
   }
   

   IllegalArgumentException is a RuntimeException, so declaring them in the method signature is not mandatory, you could omit it:

   public String greetServer(String name) {
    ...
   }