Student Registration System

This application is a "Student Registration System" using JDBC with Oracle as part of our school project. It works as it stands right now.

Questions:

• What is a better way to structure the project? Why?

• Any 'crimes' committed in the code?

Details:

• I've decided to make the interface as a web service/page instead of a swing application.

• I'm using Tomcat 7.0, jTable.org jquery based table, JSP, Java.

The current files in the project:

• DBInterface.java

• StudentInterface.java

• Students.java - Acts as the model interfacing with the database using JDBC

• StudentsAjax.jsp - Acts as the view providing data and services to the HTML client

index.html

Important parts of the files:

DBInterface

package models;

public interface DBInterface {      
    static final String dburl = "jdbc:oracle:thin:@";
    static final String dbuser = "xxx";
    static final String dbpass = "xxx";
}

StudentInterface

package models;

import java.util.List;

import org.json.JSONObject;

public interface StudentsInterface extends DBInterface {

    public int createStudent(String sid, String firstName, String lastName, String status, double gpa,  String email);

    public List retrieveStudent(String sid);

...........

}

Students

```
package models;

import java.sql.CallableStatement;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;

import oracle.jdbc.OracleTypes;
import oracle.jdbc.pool.OracleDataSource;

import org.json.JSONObject;

public class Students implements StudentsInterface {

/*
* Method creates a Student entry into the table.
* @Returns 1 if successfull. 0 otherwise.
* (non-Javadoc)
* @see models.StudentsInterface#createStudent(java.lang.String, java.lang.String, java.lang.String, java.lang.String, int, java.lang.String)
*/

The first 'crime' is a SQL injection vulnerability. You are using a PreparedStatement when inserting, which is great. You avoided the problem there. But this is a problem:

String query = "SELECT * FROM students";
if(!sid.equals("ALL")) 
    query += " WHERE sid = '" + sid + "' ";

If the sid parameter comes from outside the system (as it does above), it can be anything, including a maliciously crafted string that could cause damage to the system. The simple solution is to build the query using parameter placeholders and use a PreparedStatement here as well.

String query = "SELECT * FROM students";
if(!sid.equals("ALL")) 
    query += " WHERE sid = ?";

The second 'crime' is the unsafe closing of database resources. Each resource should be closed in a finally block to ensure that it is properly released, even in the event that an exception occurs while working with the resource.

For example, given this code:

Connection conn = ds.getConnection(dbuser, dbpass);
// do some work using the connection (what if an exception occurs here?)
conn.close();

The connection will not be closed if an exception occurs between opening and closing the connection. To ensure the connection is always closed, use something like this:

Connection conn = ds.getConnection(dbuser, dbpass);
try {
    // do some work using the connection
}
finally {
    conn.close(); // this will be called even if an exception occurs in the try block
}

If you are using Java 7, check out the try-with-resources statement. The above code can be simplified to this:

try (Connection conn = ds.getConnection(dbuser, dbpass)) {
    // do some work using the connection
} // connection will automatically be closed here

This also goes for prepared statements, result sets, and any other type of resource that might need to be closed/released.

createStudent is declared to return an int of 1 if successful, 0 otherwise. It seems like a boolean would better represent the possible outcomes. Having said that, it might be even better to throw an exception in the event of failure.

The retrieveStudent function is confusing. The name and argument imply that it will retrieve a single student by an sid, but the return type implies it will return multiple. It is not obvious that you can call it in a 'special' way to get all students. I would recommend splitting this into 2 separate functions:

public JSONObject retrieveStudent(String sid) { ... }
public List retrieveStudents() { ... }

JSP files are generally used for presentation and very limited logic. It is more common to put request handling logic in a servlet (or controller in a MVC framework), then render the presentation using a JSP. In this case, there really is no presentation, so all of that code could be in a servlet.

You should not need to cast the request.getParameter() calls to a String. They are already declared to return a String.

StackExchange Code Review Q#25572, answer score: 2