db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Knut Anders Hatlen <Knut.Hat...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-796) jdbc 4.0 specific Blob and Clob method support
Date Thu, 16 Feb 2006 15:22:40 GMT
"V.Narayanan (JIRA)" <derby-dev@db.apache.org> writes:

> V.Narayanan updated DERBY-796:
> ------------------------------
>
>     Attachment: lob_5.diff
>                 lob_5.stat
>                 ClientFrameworkExplanation_1.txt
>
> I have explained the changes made in the attachment ClientFrameworkExplanation_1.txt

Hi Narayanan,

JIRA seems to be down, so I am sending my comments by mail.

Thanks for addressing my previous comments. I have finally had time to
read through the rest of the client driver changes in your patch. A
couple of comments below.

First of all some general comments:

 * Some of the javadoc comments don't have @param/@return tags.

 * Some classes/methods don't have javadoc (for instance none of the
   methods in ClientJDBCObjectFactoryImpl and
   ClientJDBCObjectFactoryImpl40 are commented)

 * The indentation style is not consistent and makes the code harder
   to read. Some places tabs are used, other places spaces. This would
   have been less of a problem if you had used four-character tabs
   (which is the tab width that is normally used in Derby), but your
   editor seems to use eight-character tabs. If you had used spaces
   instead of tabs (which the rest of the client code seems to do),
   the problem would be solved entirely.

Then some coding issues:

I found this code in ClientConnectionPoolDataSource:

  private PooledConnection getPooledConnectionX(LogWriter dncLogWriter, ClientDataSource ds,
String user, String password) throws SQLException {
      try {
          return ClientDriver.getFactory().newClientPooledConnection(ds, 
                                           dncLogWriter, user, password);
      } catch(SQLException sqle) {
          throw sqle;
      }
  }

I don't see any reason to catch the SQLException since it's just going
to be thrown again anyway.

In ClientDriver.createJDBC40FactoryImpl() there are a couple of
problems:

   private static ClientJDBCObjectFactory createJDBC40FactoryImpl() {
       ClientJDBCObjectFactory factoryObject_=null;
       try {
               factoryObject_ = createObject("org.apache.derby.jdbc.ClientJDBCObjectFactoryImpl40");
       }
       catch(Exception e) {
               if(e instanceof ClassNotFoundException) {
                       createDefaultFactoryImpl();
               }
               else {
                       throw new RuntimeException(e.getMessage());
               }
       }
       return factoryObject_;
   }

 1. createDefaultFactoryImpl() is called in the exception handler, but
    the return value is discarded. I guess it is supposed to be
    returned.

 2. I don't like the way exceptions are handled (in particular the use
    of instanceof and throwing away the stack trace)

 3. I don't see much value in the createObject() method, since it's
    just calling Class.forName(...).newInstance() and is just being
    used once

I would have deleted the createObject() method and rewritten
createJDBC40FactoryImpl() like this:

    private static ClientJDBCObjectFactory createJDBC40FactoryImpl() {
        final String factoryName =
            "org.apache.derby.jdbc.ClientJDBCObjectFactoryImpl40";
        try {
            return (ClientJDBCObjectFactory)
                Class.forName(factoryName).newInstance();
        } catch (ClassNotFoundException cnfe) {
            return createJDBC40FactoryImpl();
        } catch (Exception e) {
            RuntimeException rte = new RuntimeException(e.getMessage());
            if (JVMInfo.JDK_ID >= JVMInfo.J2SE_14) {
                rte.initCause(e);
            }
            throw rte;
        }
    }

The method Configuration.atLeast() duplicates functionality found in
JVMInfo. You can use something like

   if (JVMInfo.JDK_ID >= JVMInfo.J2SE_16 )

instead. Preferably, you could create a method supportsJDBC40() that
returns a boolean.

-- 
Knut Anders


Mime
View raw message