patternjavaMinor
Servlet for querying database on some high-loaded system
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 (
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
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:
I'd use an explaining variable:
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
-
JDBC connection details often change. A configuration file be better for them.
-
-
Comments like this are unnecessary:
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:
It's not a big difference but a little bit less error-prone.
-
The
It could be simply the following:
-
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 blockCode 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.