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…') |
|||
(10 intermediate revisions by one other user not shown) | |||
Line 1: | Line 1: | ||
− | + | {{Admon/obsolete}} | |
− | { | ||
− | |||
− | |||
− | |||
− | |||
− | |||
− | |||
− | } | ||
− | |||
− | |||
+ | [[category: NexJ Express PostgreSQL]] | ||
==Classes== | ==Classes== | ||
===Table=== | ===Table=== | ||
Line 18: | Line 9: | ||
! 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 |
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/72 DONE] | ||
|} | |} | ||
Line 28: | Line 20: | ||
! Status | ! Status | ||
|- | |- | ||
− | | setQueryTimeout() | + | | setQueryTimeout() |
− | + | *remove the condition | |
+ | *it should be possible to set 0 timeout | ||
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/73 DONE] | ||
|- | |- | ||
− | |CancelTask -> StatementCancelationTask; make it a top level class | + | |CancelTask -> StatementCancelationTask; |
− | | | + | *make it a top level class |
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/74 DONE] | ||
|} | |} | ||
Line 41: | Line 36: | ||
! 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 | ||
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/75 DONE] | ||
|- | |- | ||
− | | | + | | The message about ignoring an index has changed |
− | | | + | *I think the extra info in the original message that the index was ignored was important. |
+ | |Diff is malfunctioning | ||
|- | |- | ||
− | + | |There are extensive changes to readSchema? | |
− | + | *What is the reason for these (or is it the diff malfunctioning)? | |
− | + | |Diff is 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. |
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/77 DONE] |
|- | |- | ||
− | |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.). | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/76 DONE] |
|- | |- | ||
| isWindowsCompatiable -> isWindowsCompatible | | isWindowsCompatiable -> isWindowsCompatible | ||
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/78 DONE] |
|} | |} | ||
Line 76: | Line 70: | ||
! 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) |
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/79 DONE] | ||
|} | |} | ||
Line 87: | Line 82: | ||
|- | |- | ||
|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 100: | 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 |
+ | *you can optimize these with much simpler conditions than the ones in isLOB. | ||
| | | | ||
|- | |- | ||
|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. 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. |
+ | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/81 DONE] | ||
|- | |- | ||
− | |unwrap | + | |unwrap |
− | 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? | ||
| | | | ||
+ | - 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 |
− | 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 |
− | | | + | |[https://bitbucket.org/gbatumbya/postgresql_external/issue/86 DONE] |
|} | |} | ||
Line 153: | 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 | ||
− | 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. |
| | | | ||
+ | * 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 180: | Line 182: | ||
|- | |- | ||
|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. |
− | | | + | |[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 |