Need Code Review

[Elton Hughes]

Hello All and thank you for your time.
Background: I work for a small non-profit agency. My primary responsibility is developing ad hoc reoprts for management from our Informix database. My usual tools are SQL and Perl. In June, I found Head First Java in a local bookstore and loved it. I went through the whole book and felt comfortable with all that I learned.
Fast forward to today. Our Informix database is getting replaced by a MS SQL Server. When I asked the vendor about ad hoc reporting tools, I was told that none were in the original proposal, but you can use ODBC and/or JDBC to do what you need. <expletive deleted by author>
So, today I put together the following.

  
import java.sql.*;
public class CISRSAgencyList {
public static void main (String args[]) {

try {

DriverManager.registerDriver(new com.microsoft.jdbc.sqlserver.SQLServerDriver());
Connection connection = DriverManager.getConnection(
"jdbc:microsoft:sqlserver://10.0.15.139:1433","<userid>","<passwd>");
Statement stmt = connection.createStatement();
ResultSet result = stmt.executeQuery(
"select * from CMS.dbo.Agency");

while ( result.next() ) {

System.out.println( result.getString("AgencyCode") + "\t" +
result.getString("AgencyName") + "\t" +
result.getString("Address") + "\t" +
result.getString("City") + "\t" +
result.getString("State") + "\t" +
result.getString("Zip5") + "\t" +
result.getString("ZipLast4") + "\t" +
result.getString("AreaCode") + "\t" +
result.getString("Phone") );

} // End of while
try { 
stmt.close();
} catch ( Exception e ) {}

try { 
connection.close();
} catch ( Exception e ) {}

} catch (Exception e) {

System.err.println("problems connecting to the the database");
System.err.println(e.getMessage());

}
} // end of main
} // End of class

Crude, but it works.
But I am also certain that it can be written better, after all, I am a beginner at this. So JavaRanchers, what would you do to make it better.
Thanks!
Elton

[Jeanne Boyarsky]

Elton,
The code looks good and functional. It's not really necessary to put closing the statement/connection inside a try/catch. If the first one fails, the second will as well. It's easier to just let the outer try/catch deal with the exception.

[Jamie Robertson]

Elton, as you have experienced, databases come and go. IP addresses change, ports change and the such. So it is a good idea to separate the connection creation from the programs using it so that you only have to maintain it in one place( not hard-coded in several programs ).
it can be as simple as:

import java.sql.*  
public class DBConnect
{
   public static Connection getConnection( String username, String password ) throws SQLException
   {
      DriverManager.registerDriver( new com.microsoft.jdbc.sqlserver.SQLServerDriver() );
      Connection connection = DriverManager.getConnection( "jdbc:microsoft:sqlserver://10.0.15.139:1433",username,passord ); 
      return connection;
   }
}
import java.sql.*;

public class CISRSAgencyList {
public static void main (String args[]) {
Connection connection = null;
try {
connection = DBConnect.getConnection( "<username>", "<password>" );
Statement stmt = connection.createStatement();
ResultSet result = stmt.executeQuery(
"select * from CMS.dbo.Agency");

while ( result.next() ) {

System.out.println( result.getString("AgencyCode") + "\t" +
result.getString("AgencyName") + "\t" +
result.getString("Address") + "\t" +
result.getString("City") + "\t" +
result.getString("State") + "\t" +
result.getString("Zip5") + "\t" +
result.getString("ZipLast4") + "\t" +
result.getString("AreaCode") + "\t" +
result.getString("Phone") );

} // End of while
stmt.close();
connection.close();

} catch (Exception e) {

System.err.println("problems connecting to the the database");
System.err.println(e.getMessage());
try
{
if( connection != null )
{
connection.close();
}
}
catch( Exception ex ){}
}
} // end of main
} // End of class

you can modify it to be more flexible as you need it to be ( i.e. loading multiple database instances and drivers ) but all you really need to do now is to separate the connection logic from the programs that use it.
[ just edited to get rid of the extra try/catch blocks and to add a connection.close to your exception block to ensure db resources are released. ]
Jamie
[ December 17, 2003: Message edited by: Jamie Robertson ]

[Wayne L Johnson]

Why not use the "finally" block for cleaning up database resources? That's why it's there.

public class CISRSAgencyList {
   public static void main (String args[]) {
      Connection conn = null;  // [B]declared outside of block[/B]
      Statement  stmt = null;  // [B]declared outside of block[/B]
      ResultSet  rset = null;  // [B]declared outside of block[/B]
      try {
         conn = DBConnect.getConnection( "username", "password" );
         stmt = conn.createStatement();
         rset = stmt.executeQuery( ...):
         while ( rset.next() ) {
         }
      } catch (Exception e) {
         ...
      } finally {
         try { if (rset != null) rset.close(); } catch (Exception ex) { }
         try { if (stmt != null) stmt.close(); } catch (Exception ex) { }
         try { if (conn != null) conn.close(); } catch (Exception ex) { }
      }
   }
}

By putting the code in the "finally" block you are guaranteed that it will be executed regardless of whether you threw an exception or not. Also, with some [most? all?] databases it's a good idea to explicitly close the result set as well as the statement and database.
This is a good model to use when doing database access, and helps prevent dangling resources on the database.
[ December 18, 2003: Message edited by: Wayne L Johnson ]

[Dana Hanna]

As a Java enterprise developer of 5 years - the above is right on. Better safe than sorry when cleaning up. Too many connection leaks around my place in the past.

[Elton Hughes]

Hello All,
I am sorry for being so tardy with this. But I did modify my original program and it looks like this:

 import java.sql.*;
public class CISRSAgencyList {
    public static void main (String args[]) {

    Connection conn = null;
    Statement  stmt = null;
    ResultSet  rset = null;
    
    try {
    
        DriverManager.registerDriver(new com.microsoft.jdbc.sqlserver.SQLServerDriver());
        
        conn = DriverManager.getConnection(
"jdbc:microsoft:sqlserver://10.0.15.139:1433",<userdid>,<password> :wink: ;
stmt = conn.createStatement();
rset = stmt.executeQuery(
    "select * from CMS.dbo.Agency");
    
while ( rset.next() ) {

System.out.println( rset.getString("AgencyCode") + "\t" +
rset.getString("AgencyName") + "\t" +
rset.getString("Address") + "\t" +
rset.getString("City") + "\t" +
rset.getString("State") + "\t" +
rset.getString("Zip5") + "\t" +
rset.getString("ZipLast4") + "\t" +
rset.getString("AreaCode") + "\t" +
rset.getString("Phone") );

} // End of while

} catch (Exception e) {

    System.err.println("problems connecting to the database");
    System.err.println(e.getMessage());
    
} finally {

    try { 
        if (rset != null) rset.close();
    } catch (Exception ex) {}
    
    try {
        if (stmt != null) stmt.close();
    } catch (Exception ex) {}
    
    try {
    if (conn != null) conn.close();
    } catch (Exception ex) {}
    
} // end of finally. The finally section cleans up data resources.

    } // end of main method
    
} // end of class 

It looks much better, at least to my novice eye.
My thanks to you all.
Elton

[Lu Battist]

I'd suggest that simply doing a tab delimited output while functional isn't pretty. If your datatypes are varchar then the tabs won't always line up and it can get hard to read. I fairly basic html page output could greatly impove the look. Put the results in a table complete with column headers. The table format will keep everything lined up. The column header can be done manually if you always do custom reports. If you want a dynamic sql query then you'll have to get and use resultSetMetaData.