HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Servlet for querying database on some high-loaded system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemhighqueryingdatabaseloadedsomeforservlet

Problem

What should the code do: Process client HTML requests, query database and return the answer in XML. Working with a high load.
I need to know how can it be optimized.
Is something terribly wrong with this code?

Input data: HTML-session, MAC-address (in form of GET argument).

Output data: XML (session_id, session_sign).

Servlet:

```
package com.packageexample.servlet;

import java.io.IOException;

import javax.servlet.*;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

import org.apache.tomcat.jdbc.pool.DataSource;
import org.apache.tomcat.jdbc.pool.PoolProperties;

/**
* Servlet implementation class TestServlet
*/
@WebServlet("/Servlet")
public class Servlet extends HttpServlet {
private static final long serialVersionUID = -3954448641206344959L;
private static final long SESSION_TIMEOUT = 360000;
private static final String JDBC_DRIVER = "com.mysql.jdbc.Driver";
private static final String JDBC_MYSQL_SERVER = "jdbc:mysql://192.168.0.100:3306/test?useUnicode=true&characterEncoding=UTF-8";
private static final String JDBC_MYSQL_USER = "test";
private static final String JDBC_MYSQL_PASSWORD = "test";

private static DataSource datasource;
private static enum RESPONSE_STATUS {SESSION_NEW, SESSION_RESTORED, SESSION_OK, MAC_UNDEFINED, UNAUTHORIZED_ACCESS, HACK_ATTEMPT};
/**
* @see HttpServlet#HttpServlet()
*/
public Servlet() {
super();
PoolProperties p = new PoolProperties();
p.setUrl(JDBC_MYSQL_SERVER);
p.setDriverClassName(JDBC_DRIVER);
p.setUsername(JDBC_MYSQL_USER);
p.setPassword(JDBC_MYSQL_PASSWORD);
p.setJmxEnabled(true);
p.setTestWhileIdle(false);
p.setTestOnBorrow(true);
p.setValidationQuery("SELECT 1");
p.setTestOnReturn(false);
p.setValidatio

Solution

Some minor random notes:

-
Instead of commenting, like this:

//If session_id is not null, we need to check if it is valid for mac provided
    if (!(device.getSessionId().equals(session.getId()))) {
        makeXml (request,response,session_id, RESPONSE_STATUS.HACK_ATTEMPT);
        return;
    }


I'd use an explaining variable:

final boolean isValidMac = device.getSessionId().equals(session.getId());
if (!isValidMac) {
    makeXml (request, response, session_id, RESPONSE_STATUS.HACK_ATTEMPT);
    return;
}


This would make the comment unnecessary. References:

Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:


Put the result of the expression, or parts of the expression,
in a temporary variable with a name that explains the purpose.

And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.

-
I'd rename SESSION_TIMEOUT to SESSION_TIMEOUT_MILLIS. It would help readers, maintainers.

-
JDBC connection details often change. A configuration file be better for them.

-
session_id and mac_address do not follow the camelCase convention which the other parts of the code use.

-
Comments like this are unnecessary:

/**
 * Servlet implementation class TestServlet
 */


It says nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

-
Instead of raw strings I'd consider using this:

ConnectionState.class.getCanonicalName() + ";" + 
    StatementFinalizer.class.getCanonicalName()


It's not a big difference but a little bit less error-prone.

-
The else keyword is unnecessary here:

/*Now that we know for sure that we've got some valid mac-address provided, 
/we need to check, if client has got some SESSION_ID*/
if (session_id == null) { 
    ...
    return;
}
else {
    ... // content of the else block
}


It could be simply the following:

if (session_id == null) { 
    ...
    return;
}
... // content of the (former) else block

Code Snippets

//If session_id is not null, we need to check if it is valid for mac provided
    if (!(device.getSessionId().equals(session.getId()))) {
        makeXml (request,response,session_id, RESPONSE_STATUS.HACK_ATTEMPT);
        return;
    }
final boolean isValidMac = device.getSessionId().equals(session.getId());
if (!isValidMac) {
    makeXml (request, response, session_id, RESPONSE_STATUS.HACK_ATTEMPT);
    return;
}
/**
 * Servlet implementation class TestServlet
 */
ConnectionState.class.getCanonicalName() + ";" + 
    StatementFinalizer.class.getCanonicalName()
/*Now that we know for sure that we've got some valid mac-address provided, 
/we need to check, if client has got some SESSION_ID*/
if (session_id == null) { 
    ...
    return;
}
else {
    ... // content of the else block
}

Context

StackExchange Code Review Q#37740, answer score: 5

Revisions (0)

No revisions yet.