You have three general options for connection management.
1) create connection/statement at the beginning of a transaction, perform transaction, then close all resources (result set, statement, connection) after the transaction. This means doing it in every single method of your DAO.
2) use a library/framework to handle connection pooling
3) manually reuse connection/statement in your code without closing them
i wouldn't worry about pooling for your purposes, and manually reusing them leads to bad practices and potential problems, especially for a beginner. so i recommend you take option 1.
Notice your code below? you're reusing the connection and you're not closing your result set, statement, or connection.
Code
public boolean validID() throws SQLException
{
this.setQuery("Select s.ID FROM STUDENT s WHERE s.ID = ?");
this.pStmt = conn.prepareStatement(this.query);
this.pStmt.setInt(1, this.studentID);
ResultSet rs = this.pStmt.executeQuery();
return rs.isBeforeFirst();
}
Example of what to change it to:
Code
public boolean isValidId(long id) throws SQLException{
Connection con = null;
PreparedStatement ps = null;
ResultSet rs = null;
try{
con = getConnection(); // write this method which has the connection string, username, password inside
ps = con.prepareStatement("Select 1 FROM STUDENT WHERE ID=?");
ps.setLong(1, id);
rs = ps.executeQuery();
boolean isValid = rs.isBeforeFirst();
log.debug("isValidId(" + id + ")=" + isValid);
return isValid;
} catch (SQLException e){
throw e;
} catch (Exception e){
log.error(e);
} finally{
close(rs, ps, conn);
}
}
A couple of points to notice.
1) the connection/statement/resultset variables are declared OUTSIDE of the try/catch. this is done so the variables are in scope for the finally
2) always close the result set, statement, and connection when you no longer need it
3) always close them inside a finally block. if an error occurs when the connection is open but before you close it manually inside of the try, then the cleanup might not execute.
4) use PreparedStatement instead of Statement. i see you already used it, which is good.
5) always include some kind of logging. it saves you from debugging your stuff.
close is a shortcut. it's really annoying that closing a ResultSet, Statement, and Connection can all throw exceptions. here's the code for it:
Code
public static void close(ResultSet rs, Statement s, Connection con) {
if (rs != null) {
try {
rs.close();
} catch (SQLException e) {
log.error(e);
}
}
if (s != null) {
try {
s.close();
} catch (SQLException e) {
log.error(e);
}
}
if (con != null) {
try {
con.close();
} catch (SQLException e) {
log.error(e);
}
}
}
this method returned a simple boolean. if you needed to return more data (eg: name, id, class, etc etc), then you assign data from the result set into a Data Transfer Object, eg: Student. then return the Student or List of Student objects as needed.
i am not a fan of using instance variables for things like this.query and this.studentId. the query should only exist inside the method it's used, and studentId should be a parameter IMO. each method should be useable on its own without side effects or dependencies. the main reason it's not static is because, as Minko pointed out, it's best to put them behind an interface + factory. (static methods don't get along with interfaces)
i personally use long instead of int because i've been screwed by it in the past. but for your assignment, int should be fine. i use log4j for my logging, but you can wrap System.out.println if you wish.
ex:
Code
public static void log(String message){
if (true) // alternatively, check for some flag/config/etc
{
System.out.println(message);
// alternatively write to a file
}
}
then put your log statements everywhere you want. when you're ready to turn it in, you dont want the logging turned on, so just change true to false or whatever you want. if you directly put System.out.println(..) everywhere, you have to manually remove every single one of them when you're done.
This post was edited by carteblanche on Nov 10 2014 10:19pm