Open main menu

CDOT Wiki β

Changes

PostgreSQL Adapter Project - Code Review 4 Changes

2,201 bytes added, 20:50, 26 January 2014
no edit summary
{{Admon/obsolete}}
 
[[category: NexJ Express PostgreSQL]]
==General==
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|{
;
} undo the change to ;
|
|}
 
==Classes==
===Table===
|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''']
|}
*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
|'''[https://bitbucket.org/gbatumbya/postgresql_external/issue/74 DONE''']
|}
*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'''|-|I think the portable flag should not take into account columns that are not added to the table (return null in this case)?|'''DONE''']
|-
| The message about ignoring an index has changed
*remove the separator flag.
*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()
*(Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability.
This hampers method reusability.).
|'''[https://bitbucket.org/gbatumbya/postgresql_external/issue/76 DONE''']
|-
| isWindowsCompatiable -> isWindowsCompatible
|'''[https://bitbucket.org/gbatumbya/postgresql_external/issue/78 DONE''']
|}
|*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''']
|}
|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 "~~"
||-|*MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1.|[https://bitbucket.org/gbatumbya/postgresql_external/issue/80 DONE]
|-
|BIND_BLOB
*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.
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/87 DONE]*Remove BIND_BLOB
|-
|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)
*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<br/>
*why is a pooled connection ever used there?
*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
*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
*Add a space after ";" for readability
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/82 DONE]
|-
|appendLiteral
*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.
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/83 DONE]*used StringUtil.appendUTC
|-
|appendTypeConversion
*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
*Spaces around commas are needed. Wrap the line after about 110 chars.
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/85 DONE]
|-
|isUnicode<br/>
*Use lower case SQL keywords
*Use bind variables - this is what makes prepared statements cacheable by the DB
*catch(SQLException e) space before (
*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]
|}
! Status
|-
|*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
*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]
|}
*Use getColumnAlterationToken
*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
*Method JavaDoc comments are required.
*What does this do?
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/91 DONE]
|-
|appendTSIncrement
*interval' – perhaps a space before "'" for readability?
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/92 DONE]
|-
|getGUIDExpr
*Remove "XXX: from the comment
*Move the comment to the class header comment
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/93 DONE]
|-
|isImplicitConversion
*srcType == Primitive.BOOLEAN ) - remove space before )
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/94 DONE]
|-
|isValidColumnName
*Spaces around "-"
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/95 DONE]
|-
|addColumn
*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]
|}