"V.Narayanan (JIRA)" <[email protected]> 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
