4
\$\begingroup\$

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:

  1. What is a better way to structure the project? Why?
  2. 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<JSONObject> 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)
     */
    @Override
    public int createStudent(String sid, String firstName, String lastName, String status, double gpa,  String email) {

        int retVal = 0;

        try {
            // Connection to Oracle server
            OracleDataSource ds = new oracle.jdbc.pool.OracleDataSource();
            ds.setURL(dburl);
            Connection conn = ds.getConnection(dbuser, dbpass);

              //Insert
              PreparedStatement insert = conn.prepareStatement("INSERT into students VALUES(?,?,?,?,?,?)");

              // Input other values 
              insert.setString(1, sid);
              insert.setString(2, firstName);
              insert.setString(3, lastName);
              insert.setString(4, status);
              insert.setDouble(5, gpa);
              insert.setString(6, email);             

              // execute the update
              insert.executeUpdate();

            // close the result set, statement, and the connection
            conn.close();
            retVal = 1;
        } catch (SQLException ex) {
            System.out.println("\n*** SQLException caught ***\n");
        } catch (Exception e) {
            System.out.println("\n*** other Exception caught ***\n");
        }

        return  retVal;
    }


    //--------------------------------------
    @Override
    public List<JSONObject> retrieveStudent(String sid) {

        List<JSONObject> t = new ArrayList<JSONObject>();

        try {
            // Connection to Oracle server
            OracleDataSource ds = new oracle.jdbc.pool.OracleDataSource();
            ds.setURL(dburl);
            Connection conn = ds.getConnection(dbuser, dbpass);

            // Query
            Statement stmt = conn.createStatement();
            System.out.println("\n*** Executing");

            String query = "SELECT * FROM students";

            if(!sid.equals("ALL")) 
                query += " WHERE sid = '" + sid + "' ";

            // Save result
            ResultSet rset;
            rset = stmt.executeQuery(query);
            System.out.println("\n*** Done");
            // determine the number of columns in each row of the result set
            ResultSetMetaData rsetMeta = rset.getMetaData();
            int columnCount = rsetMeta.getColumnCount();
            System.out.println("\n*** Retrieved Student Count is 


            //insert result into a List
            while (rset.next()) {
                JSONObject e = new JSONObject();

                //loop through columns
                for (int i = 1; i <= columnCount; i++) {

                     String key = rsetMeta.getColumnName(i);
                     String value = rset.getString(i);

                     //Convert First char to upper case
                     key = key.toLowerCase();
            key =toString(key.charAt(0)).toUpperCase()+key.substring(1);

                     e.put(key,value);

                }

                 t.add(e);
            }

            // close the result set, statement, and the connection
            rset.close();
            stmt.close();
            conn.close();
        } catch (SQLException ex) {
            System.out.println("\n*** SQLException caught ***\n");
        } catch (Exception e) {
            System.out.println("\n*** other Exception caught ***\n");
        }

        return t;
    }
....
....
....
}

StudentsAjax

<%@page import="org.json.JSONObject"%>
<%@page import="java.util.ArrayList"%>
<%@page import="java.util.List"%>
<%@ page language="java" contentType="application/json; charset=ISO-8859-1"
    pageEncoding="ISO-8859-1"%>
<%@ page import="models.*" %>
<% String incomingAction = (String)request.getParameter("action");  %>
<%
    StudentsInterface si = new Students();
    JSONObject cover = new JSONObject();

    //Decide what to perform based on incoming action.
    //-------------------- LIST
    if(incomingAction.equals("list")) {
        //Prepare JSON output
        cover.put("Result","OK");
        cover.put("Records", listStudents("ALL"));
    }
    else //-------------------- CREATE
    if(incomingAction.equals("create")) {


        //Retrieve result
        int reply = si.createStudent(
                    (String)request.getParameter("Sid")
                    ,(String)request.getParameter("Firstname")
                    ,(String)request.getParameter("Lastname")
                    ,(String)request.getParameter("Status")
                    ,(String)request.getParameter("Email")
                );

        //Prepare JSON output
        cover.put("Result","OK");
    cover.put("Record", listStudents((String)request.getParameter("Sid")).get(0)  );
    }
    else //-------------------- UPDATE
.....
.....
.....


    //Output in the JSON format.
    out.println(cover.toString());

%>
<%!


// -------------------------------- Functions
List<JSONObject> listStudents(String id) {
    StudentsInterface si = new Students();
    List<JSONObject> t;

    //Retrieve result
    if(id.equals("ALL")) {
        t = si.showAllStudents();
    }
    else {

        t = si.retrieveStudent(id);
    }

    return t;
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  1. Follow layered architecture. Read about Model-View-Controller pattern. (This should eliminate most of your design issues so I won't specify each one of them here)
  2. Externalize environment specific entries like dburl, dbuser and dbpassword by creating a JDBC DataSource in tomcat. You should also pool database connections. This should eliminate the need for DBInterface.
  3. Use Checked (like StudentNotFoundException etc.) and Unchecked exceptions. Here are the guidelines.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks Pangea for commenting on and pointing out good resources for improving the design of the system. \$\endgroup\$
    – Jack Brown
    May 10, 2013 at 23:26
2
\$\begingroup\$

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<JSONObject> 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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks Joe for Pointing out the security issue and commenting on the general coding standards ! mucha gracis ! \$\endgroup\$
    – Jack Brown
    May 10, 2013 at 23:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.