Difference between revisions of "PostgreSQL Adapter Project - Code Review 4 Changes"
(7 intermediate revisions by one other user not shown) | |||
Line 1: | Line 1: | ||
+ | {{Admon/obsolete}} | ||
+ | |||
[[category: NexJ Express PostgreSQL]] | [[category: NexJ Express PostgreSQL]] | ||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
==Classes== | ==Classes== | ||
===Table=== | ===Table=== | ||
Line 21: | Line 11: | ||
|getQuotedName | |getQuotedName | ||
* functionality is not needed just use SQLSchemamanager.quote() the way this is done in other sql manager code | * functionality is not needed just use SQLSchemamanager.quote() the way this is done in other sql manager code | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/72 DONE] |
|} | |} | ||
Line 33: | Line 23: | ||
*remove the condition | *remove the condition | ||
*it should be possible to set 0 timeout | *it should be possible to set 0 timeout | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/73 DONE] |
|- | |- | ||
|CancelTask -> StatementCancelationTask; | |CancelTask -> StatementCancelationTask; | ||
*make it a top level class | *make it a top level class | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/74 DONE] |
|} | |} | ||
Line 49: | Line 39: | ||
*change the comment to "Reads a column from a JDBC driver result set and adds it to a table object". | *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 | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/75 DONE] |
− | |||
− | |||
− | |||
|- | |- | ||
| The message about ignoring an index has changed | | The message about ignoring an index has changed | ||
Line 65: | Line 52: | ||
*remove the separator flag. | *remove the separator flag. | ||
*Adding separators should be done in different code. Each method should do as little as possible. | *Adding separators should be done in different code. Each method should do as little as possible. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/77 DONE] |
|- | |- | ||
|isPortable() | |isPortable() | ||
Line 71: | Line 58: | ||
*(Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability. | *(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.). | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/76 DONE] |
|- | |- | ||
| isWindowsCompatiable -> isWindowsCompatible | | isWindowsCompatiable -> isWindowsCompatible | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/78 DONE] |
|} | |} | ||
Line 85: | Line 72: | ||
|*Proxy.java -> *Wrapper.java | |*Proxy.java -> *Wrapper.java | ||
*(this is not the proxy pattern, but more like the decorator pattern) | *(this is not the proxy pattern, but more like the decorator pattern) | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/79 DONE] |
|} | |} | ||
Line 96: | Line 83: | ||
|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. | |
− | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/80 DONE] | |
− | |||
− | | | ||
|- | |- | ||
|BIND_BLOB | |BIND_BLOB | ||
Line 108: | Line 93: | ||
*If an error occurs, lo will not be closed. Please fix this. | *If an error occurs, lo will not be closed. Please fix this. | ||
* setAutoCommit - why is this modified at all? We cannot simply change the autocommit flag on a connection. | * setAutoCommit - why is this modified at all? We cannot simply change the autocommit flag on a connection. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/87 DONE] |
+ | *Remove BIND_BLOB | ||
|- | |- | ||
|BIND_STRING, BIND_BINARY | |BIND_STRING, BIND_BINARY | ||
Line 115: | Line 101: | ||
|- | |- | ||
|RequestTimeout should take into account non-positive timeouts. | |RequestTimeout should take into account non-positive timeouts. | ||
− | | | + | |values <=0 do not get to requestTimeout |
|- | |- | ||
| 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. | *should be moved to the cancelation task, as static methods. | ||
*The timer singleton instance (lazy initialized) should be there as well. | *The timer singleton instance (lazy initialized) should be there as well. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/81 DONE] |
|- | |- | ||
− | |unwrap | + | |unwrap |
*why is a pooled connection ever used there? | *why is a pooled connection ever used there? | ||
*When could an exception occur? | *When could an exception occur? | ||
*Are we using the correct data source class? | *Are we using the correct data source class? | ||
| | | | ||
+ | - PGXADataSource is the one that gives pooled connections<br/> | ||
+ | - When using data load/schema tool<br/> | ||
+ | - Yes, PGXADataSource is 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? | ||
| | | | ||
+ | -s_PGXAConnection used to ensure that reflected methods and properties loaded successfully<br/> | ||
+ | -s_getQueryExecutor used to ensure that unwrapped connection is a PGConnection | ||
|- | |- | ||
|appendIdentitySuffix | |appendIdentitySuffix | ||
*Add a space after ";" for readability | *Add a space after ";" for readability | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/82 DONE] |
|- | |- | ||
|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. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/83 DONE] |
+ | *used StringUtil.appendUTC | ||
|- | |- | ||
|appendTypeConversion | |appendTypeConversion | ||
*TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps. | *TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/84 DONE] |
+ | *modified postgresql setup scripts to set timezone to UTC | ||
+ | *changed to use date_part('epoch', timestamp) | ||
|- | |- | ||
|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. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/85 DONE] |
|- | |- | ||
− | |isUnicode | + | |isUnicode |
*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 | ||
Line 155: | Line 149: | ||
*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 | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/86 DONE] |
|} | |} | ||
Line 164: | Line 158: | ||
! Status | ! Status | ||
|- | |- | ||
− | |Provide a class comment | + | |*Provide a class comment |
− | + | *Prepend a comment "// inner classes" | |
− | + | *Use visibility declarators on members ("protected") | |
− | + | *Fold lines after about 110 chars | |
− | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/88 DONE] | |
− | |||
− | |||
− | |||
− | |||
− | |||
− | | | ||
|- | |- | ||
|requestSavepoint | |requestSavepoint | ||
Line 182: | Line 170: | ||
*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. | ||
| | | | ||
+ | * I think not, since the savepoints are only created inside a transaction block | ||
+ | * No, the driver does not allow setSavePoint on XA transaction (org.postgresql.xa.PGXAConnection) | ||
+ | *[https://bitbucket.org/gbatumbya/postgresql_external/issue/89 DONE] | ||
|} | |} | ||
Line 194: | Line 185: | ||
*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. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/90 DONE] |
+ | *Yes, it supports indexing booleans. Suggested use is with partial indexes on the condition that queries will use most often. | ||
|- | |- | ||
|appendLOManagerTrigger | |appendLOManagerTrigger | ||
*Method JavaDoc comments are required. | *Method JavaDoc comments are required. | ||
*What does this do? | *What does this do? | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/91 DONE] |
|- | |- | ||
|appendTSIncrement | |appendTSIncrement | ||
*interval' – perhaps a space before "'" for readability? | *interval' – perhaps a space before "'" for readability? | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/92 DONE] |
|- | |- | ||
|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 | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/93 DONE] |
|- | |- | ||
|isImplicitConversion | |isImplicitConversion | ||
*srcType == Primitive.BOOLEAN ) - remove space before ) | *srcType == Primitive.BOOLEAN ) - remove space before ) | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/94 DONE] |
|- | |- | ||
|isValidColumnName | |isValidColumnName | ||
*Spaces around "-" | *Spaces around "-" | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/95 DONE] |
|- | |- | ||
|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. | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/97 DONE] |
+ | |- | ||
+ | |{ | ||
+ | ; | ||
+ | } undo the change to ; | ||
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/96 DONE] | ||
|} | |} |
Latest revision as of 20:50, 26 January 2014
Contents
Classes
Table
Task | Status |
---|---|
getQuotedName
|
DONE |
SQLAdapter
Task | Status |
---|---|
setQueryTimeout()
|
DONE |
CancelTask -> StatementCancelationTask;
|
DONE |
SQLSchemaManager
Task | Status |
---|---|
addColumn
|
DONE |
The message about ignoring an index has changed
|
Diff is malfunctioning |
There are extensive changes to readSchema?
|
Diff is malfunctioning |
getAlterColumnToken
|
DONE |
isPortable()
This hampers method reusability.). |
DONE |
isWindowsCompatiable -> isWindowsCompatible | DONE |
StatementProxy/PreparedStatementProxy
Task | Status |
---|---|
*Proxy.java -> *Wrapper.java
|
DONE |
PostgreSQLAdapter
Task | Status |
---|---|
MAX_FIELD_PRECISION - is this from the PostgreSQL documentation?
|
DONE |
BIND_BLOB
You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.
|
DONE
|
BIND_STRING, BIND_BINARY
|
|
RequestTimeout should take into account non-positive timeouts. | values <=0 do not get to requestTimeout |
The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel)
|
DONE |
unwrap
|
- PGXADataSource is the one that gives pooled connections |
s_PGXAConnection, s_getQueryExecutor
|
-s_PGXAConnection used to ensure that reflected methods and properties loaded successfully |
appendIdentitySuffix
|
DONE |
appendLiteral
|
DONE
|
appendTypeConversion
|
DONE
|
indexNameMatches
|
DONE |
isUnicode
|
DONE |
PostgreSQLPreparedStatementProxy
Task | Status |
---|---|
*Provide a class comment
|
DONE |
requestSavepoint
|
|
PostgreSQLSchemaManager
Task | Status |
---|---|
appendColumnAlteration
|
DONE
|
appendLOManagerTrigger
|
DONE |
appendTSIncrement
|
DONE |
getGUIDExpr
|
DONE |
isImplicitConversion
|
DONE |
isValidColumnName
|
DONE |
addColumn
|
DONE |
{
} undo the change to ; |
DONE |