Difference between revisions of "PostgreSQL Adapter Project - Code Review 4 Changes"
(Created page with '==General== {| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;" |- ! Task ! Status |- |{ ; } undo the change to ; | |} ==Cl…') |
|||
Line 18: | Line 18: | ||
! Status | ! Status | ||
|- | |- | ||
− | |getQuotedName functionality is not needed | + | |getQuotedName |
+ | * functionality is not needed just use SQLSchemamanager.quote() the way this is done in other sql manager code | ||
| | | | ||
|} | |} | ||
Line 28: | Line 29: | ||
! Status | ! Status | ||
|- | |- | ||
− | | setQueryTimeout() | + | | setQueryTimeout() |
− | + | *remove the condition | |
+ | *it should be possible to set 0 timeout | ||
+ | | | ||
|- | |- | ||
− | |CancelTask -> StatementCancelationTask; make it a top level class | + | |CancelTask -> StatementCancelationTask; |
+ | *make it a top level class | ||
| | | | ||
|} | |} | ||
Line 41: | Line 45: | ||
! Status | ! Status | ||
|- | |- | ||
− | |addColumn | + | |addColumn |
− | + | *change the comment to "Reads a column from a JDBC driver result set and adds it to a table object". | |
+ | | | ||
|- | |- | ||
|Move rs argument to the last position | |Move rs argument to the last position | ||
Line 50: | Line 55: | ||
| | | | ||
|- | |- | ||
− | | The message about ignoring an index has changed | + | | The message about ignoring an index has changed |
+ | *I think the extra info in the original message that the index was ignored was important. | ||
| | | | ||
|- | |- | ||
− | |There are extensive changes to readSchema? What is the reason for these (or is it the diff malfunctioning)? | + | |There are extensive changes to readSchema? |
+ | *What is the reason for these (or is it the diff malfunctioning)? | ||
| | | | ||
|- | |- | ||
|getAlterColumnToken | |getAlterColumnToken | ||
− | remove the separator flag. Adding separators should be done in different code. | + | *remove the separator flag. |
− | Each method should do as little as possible. | + | *Adding separators should be done in different code. Each method should do as little as possible. |
| | | | ||
|- | |- | ||
− | |isPortable() | + | |isPortable() |
− | (Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability. | + | *null/skipped columns should not affect portability. It is better to handle null columns in the caller. |
+ | *(Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability. | ||
This hampers method reusability.). | This hampers method reusability.). | ||
| | | | ||
Line 76: | Line 84: | ||
! Status | ! Status | ||
|- | |- | ||
− | |*Proxy.java -> *Wrapper.java (this is not the proxy pattern, but more like the decorator pattern) | + | |*Proxy.java -> *Wrapper.java |
+ | *(this is not the proxy pattern, but more like the decorator pattern) | ||
| | | | ||
|} | |} | ||
Line 87: | Line 96: | ||
|- | |- | ||
|MAX_FIELD_PRECISION - is this from the PostgreSQL documentation? | |MAX_FIELD_PRECISION - is this from the PostgreSQL documentation? | ||
− | The comment is incorrect. This is not 1048576*8*10 and is not 1GB, but 10MB. Remove "~~" | + | *The comment is incorrect. This is not 1048576*8*10 and is not 1GB, but 10MB. Remove "~~" |
− | + | | | |
|- | |- | ||
|MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1. | |MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1. | ||
Line 102: | Line 111: | ||
| | | | ||
|- | |- | ||
− | |BIND_STRING, BIND_BINARY | + | |BIND_STRING, BIND_BINARY |
+ | *you can optimize these with much simpler conditions than the ones in isLOB. | ||
| | | | ||
|- | |- | ||
Line 109: | Line 119: | ||
|- | |- | ||
| The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel) | | The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel) | ||
− | should be moved to the cancelation task, as static methods. The timer singleton instance (lazy initialized) should be there as well. | + | *should be moved to the cancelation task, as static methods. |
+ | *The timer singleton instance (lazy initialized) should be there as well. | ||
| | | | ||
|- | |- | ||
− | |unwrap | + | |unwrap<br/> |
− | When could an exception occur? | + | *why is a pooled connection ever used there? |
− | Are we using the correct data source class? | + | *When could an exception occur? |
+ | *Are we using the correct data source class? | ||
| | | | ||
|- | |- | ||
− | |s_PGXAConnection, s_getQueryExecutor | + | |s_PGXAConnection, s_getQueryExecutor |
− | what is the purpose of the conditional logic involving these? | + | *what is the purpose of the conditional logic involving these? |
| | | | ||
|- | |- | ||
|appendIdentitySuffix | |appendIdentitySuffix | ||
− | Add a space after ";" for readability | + | *Add a space after ";" for readability |
| | | | ||
|- | |- | ||
|appendLiteral | |appendLiteral | ||
− | TIMESTAMP_ORDINAL - this should be in UTC, I think that simply converting the timestamp to a string might not do it. | + | *TIMESTAMP_ORDINAL - this should be in UTC, I think that simply converting the timestamp to a string might not do it. |
− | Try to see if using Primitive.toString() will provide the right format. | + | *Try to see if using Primitive.toString() will provide the right format. |
| | | | ||
|- | |- | ||
|appendTypeConversion | |appendTypeConversion | ||
− | TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps. | + | *TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps. |
| | | | ||
|- | |- | ||
|indexNameMatches | |indexNameMatches | ||
− | Spaces around commas are needed. Wrap the line after about 110 chars. | + | *Spaces around commas are needed. Wrap the line after about 110 chars. |
| | | | ||
|- | |- | ||
− | |isUnicode | + | |isUnicode<br/> |
− | Use lower case SQL keywords | + | *Use lower case SQL keywords |
− | Use bind variables - this is what makes prepared statements cacheable by the DB | + | *Use bind variables - this is what makes prepared statements cacheable by the DB |
− | Object [] { -> no spaces here | + | *Object [] { -> no spaces here |
− | catch(SQLException e) space before ( | + | *catch(SQLException e) space before ( |
− | Do not repackage the SQLException as a runtime exception - the method already allows throwing an SQLException | + | *Do not repackage the SQLException as a runtime exception - the method already allows throwing an SQLException |
| | | | ||
|} | |} | ||
Line 166: | Line 178: | ||
|- | |- | ||
|requestSavepoint | |requestSavepoint | ||
− | Do we need to generate unique names for save points? (E.g. can savepoint names on different connections conflict)? | + | *Do we need to generate unique names for save points? (E.g. can savepoint names on different connections conflict)? |
− | Can the JDBC driver be used to manage savepoints? | + | *Can the JDBC driver be used to manage savepoints? |
− | Should we use prepared statements here? | + | *Should we use prepared statements here? |
− | The statement cleanup should be fixed. Right now it will not close the statement if an error occurs. | + | *The statement cleanup should be fixed. Right now it will not close the statement if an error occurs. |
| | | | ||
|} | |} | ||
Line 180: | Line 192: | ||
|- | |- | ||
|appendColumnAlteration | |appendColumnAlteration | ||
− | "extract( epoch from" - remove space after ( | + | *"extract( epoch from" - remove space after ( |
− | Use getColumnAlterationToken | + | *Use getColumnAlterationToken |
− | Are booleans indexable in Postgres? If not, we should use integers instead. | + | *Are booleans indexable in Postgres? If not, we should use integers instead. |
| | | | ||
|- | |- | ||
|appendLOManagerTrigger | |appendLOManagerTrigger | ||
− | Method JavaDoc comments are required. | + | *Method JavaDoc comments are required. |
− | What does this do? | + | *What does this do? |
| | | | ||
|- | |- | ||
|appendTSIncrement | |appendTSIncrement | ||
− | interval' – perhaps a space before "'" for readability? | + | *interval' – perhaps a space before "'" for readability? |
| | | | ||
|- | |- | ||
|getGUIDExpr | |getGUIDExpr | ||
− | Remove "XXX: from the comment | + | *Remove "XXX: from the comment |
− | Move the comment to the class header comment | + | *Move the comment to the class header comment |
| | | | ||
|- | |- | ||
|isImplicitConversion | |isImplicitConversion | ||
− | srcType == Primitive.BOOLEAN ) - remove space before ) | + | *srcType == Primitive.BOOLEAN ) - remove space before ) |
| | | | ||
|- | |- | ||
|isValidColumnName | |isValidColumnName | ||
− | Spaces around "-" | + | *Spaces around "-" |
| | | | ||
|- | |- | ||
|addColumn | |addColumn | ||
− | This might be worth refactoring further by adding a template method in order to avoid code duplication with the base class. | + | *This might be worth refactoring further by adding a template method in order to avoid code duplication with the base class. |
| | | | ||
|} | |} |
Revision as of 13:27, 12 September 2011
Contents
General
Task | Status |
---|---|
{
} undo the change to ; |
Classes
Table
Task | Status |
---|---|
getQuotedName
|
SQLAdapter
Task | Status |
---|---|
setQueryTimeout()
|
|
CancelTask -> StatementCancelationTask;
|
SQLSchemaManager
Task | Status |
---|---|
addColumn
|
|
Move rs argument to the last position | |
I think the portable flag should not take into account columns that are not added to the table (return null in this case)? | |
The message about ignoring an index has changed
|
|
There are extensive changes to readSchema?
|
|
getAlterColumnToken
|
|
isPortable()
This hampers method reusability.). |
|
isWindowsCompatiable -> isWindowsCompatible |
StatementProxy/PreparedStatementProxy
Task | Status |
---|---|
*Proxy.java -> *Wrapper.java
|
PostgreSQLAdapter
Task | Status |
---|---|
MAX_FIELD_PRECISION - is this from the PostgreSQL documentation?
|
|
MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1. | |
BIND_BLOB
You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.
|
|
BIND_STRING, BIND_BINARY
|
|
RequestTimeout should take into account non-positive timeouts. | |
The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel)
|
|
unwrap
|
|
s_PGXAConnection, s_getQueryExecutor
|
|
appendIdentitySuffix
|
|
appendLiteral
|
|
appendTypeConversion
|
|
indexNameMatches
|
|
isUnicode
|
PostgreSQLPreparedStatementProxy
Task | Status |
---|---|
Provide a class comment | |
Prepend a comment "// inner classes" | |
Use visibility declarators on members ("protected") | |
Fold lines after about 110 chars | |
requestSavepoint
|
PostgreSQLSchemaManager
Task | Status |
---|---|
appendColumnAlteration
|
|
appendLOManagerTrigger
|
|
appendTSIncrement
|
|
getGUIDExpr
|
|
isImplicitConversion
|
|
isValidColumnName
|
|
addColumn
|