Difference between revisions of "PostgreSQL Adapter Project - Code Review 4 Changes"

From CDOT Wiki
Jump to: navigation, search
Line 9: Line 9:
 
|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
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/72 DONE]
 
|}
 
|}
  
Line 21: Line 21:
 
*remove the condition
 
*remove the condition
 
*it should be possible to set 0 timeout
 
*it should be possible to set 0 timeout
−
|'''DONE'''
+
|[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
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/74 DONE]
 
|}
 
|}
  
Line 37: Line 37:
 
*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
−
|'''DONE'''
+
|[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
 
| The message about ignoring an index has changed
Line 53: Line 50:
 
*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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/77 DONE]
 
|-
 
|-
 
|isPortable()
 
|isPortable()
Line 59: Line 56:
 
*(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.).
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/76 DONE]
 
|-
 
|-
 
| isWindowsCompatiable -> isWindowsCompatible
 
| isWindowsCompatiable -> isWindowsCompatible
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/78 DONE]
 
|}
 
|}
  
Line 73: Line 70:
 
|*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)
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/79 DONE]
 
|}
 
|}
  
Line 85: Line 82:
 
*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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/80 DONE]
 
|-
 
|-
 
|BIND_BLOB
 
|BIND_BLOB
Line 94: Line 91:
 
*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.
−
|''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/87 DONE]
 
*Remove BIND_BLOB
 
*Remove BIND_BLOB
 
|-
 
|-
Line 107: Line 104:
 
*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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/81 DONE]
 
|-
 
|-
 
|unwrap
 
|unwrap
Line 126: Line 123:
 
|appendIdentitySuffix
 
|appendIdentitySuffix
 
*Add a space after ";" for readability
 
*Add a space after ";" for readability
−
|'''DONE'''
+
|[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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/83 DONE]
 
*used StringUtil.appendUTC
 
*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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/84 DONE]
 
*modified postgresql setup scripts to set timezone to UTC
 
*modified postgresql setup scripts to set timezone to UTC
 
*changed to use date_part('epoch', timestamp)
 
*changed to use date_part('epoch', timestamp)
Line 142: Line 139:
 
|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.
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/85 DONE]
 
|-
 
|-
 
|isUnicode
 
|isUnicode
Line 150: Line 147:
 
*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
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/86 DONE]
 
|}
 
|}
  
Line 163: Line 160:
 
*Use visibility declarators on members ("protected")
 
*Use visibility declarators on members ("protected")
 
*Fold lines after about 110 chars
 
*Fold lines after about 110 chars
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/88 DONE]
 
|-
 
|-
 
|requestSavepoint
 
|requestSavepoint
Line 173: Line 170:
 
* I think not, since the savepoints are only created inside a transaction block
 
* 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)
 
* No, the driver does not allow setSavePoint on XA transaction (org.postgresql.xa.PGXAConnection)
−
*'''DONE'''
+
*[https://bitbucket.org/gbatumbya/postgresql_external/issue/89 DONE]
−
*'''DONE'''
 
 
|}
 
|}
  
Line 187: Line 183:
 
*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.
−
|'''DONE'''
+
|[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.
 
*Yes, it supports indexing booleans. Suggested use is with partial indexes on the condition that queries will use most often.
 
|-
 
|-
Line 193: Line 189:
 
*Method JavaDoc comments are required.
 
*Method JavaDoc comments are required.
 
*What does this do?
 
*What does this do?
−
|'''DONE'''
+
|[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?
−
|'''DONE'''
+
|[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
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/93 DONE]
 
|-
 
|-
 
|isImplicitConversion
 
|isImplicitConversion
 
*srcType == Primitive.BOOLEAN ) - remove space before )
 
*srcType == Primitive.BOOLEAN ) - remove space before )
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/94 DONE]
 
|-
 
|-
 
|isValidColumnName
 
|isValidColumnName
 
*Spaces around "-"
 
*Spaces around "-"
−
|'''DONE'''
+
|[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 ;
 
}  undo the change to ;
−
|'''DONE'''
+
|[https://bitbucket.org/gbatumbya/postgresql_external/issue/96 DONE]
 
|}
 
|}

Revision as of 16:06, 20 September 2011

Classes

Table

Task Status
getQuotedName
  • functionality is not needed just use SQLSchemamanager.quote() the way this is done in other sql manager code
 DONE

SQLAdapter

Task Status
setQueryTimeout()
  • remove the condition
  • it should be possible to set 0 timeout
 DONE
CancelTask -> StatementCancelationTask;
  • make it a top level class
 DONE

SQLSchemaManager

Task Status
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
 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
getAlterColumnToken
  • remove the separator flag.
  • Adding separators should be done in different code. Each method should do as little as possible.
 DONE
isPortable()
  • 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.).

DONE
isWindowsCompatiable -> isWindowsCompatible DONE

StatementProxy/PreparedStatementProxy

Task Status
*Proxy.java -> *Wrapper.java
  • (this is not the proxy pattern, but more like the decorator pattern)
 DONE

PostgreSQLAdapter

Task Status
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.
 DONE
BIND_BLOB
  • Why is this code needed? Under what circumstances
  • m_connection -> use getConnection instead.
  • nLO -> l

You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.

  • 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.
 DONE
  • Remove BIND_BLOB
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. 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.
 DONE
unwrap
  • 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
- When using data load/schema tool
- 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
-s_getQueryExecutor used to ensure that unwrapped connection is a PGConnection

appendIdentitySuffix
  • Add a space after ";" for readability
 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.
 DONE
  • used StringUtil.appendUTC
appendTypeConversion
  • TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps.
 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.
 DONE
isUnicode
  • Use lower case SQL keywords
  • Use bind variables - this is what makes prepared statements cacheable by the DB
  • Object [] { -> no spaces here
  • catch(SQLException e) space before (
  • Do not repackage the SQLException as a runtime exception - the method already allows throwing an SQLException
 DONE

PostgreSQLPreparedStatementProxy

Task Status
*Provide a class comment
  • Prepend a comment "// inner classes"
  • Use visibility declarators on members ("protected")
  • Fold lines after about 110 chars
 DONE
requestSavepoint
  • 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?
  • Should we use prepared statements here?
  • 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)
  • DONE

PostgreSQLSchemaManager

Task Status
appendColumnAlteration
  • "extract( epoch from" - remove space after (
  • Use getColumnAlterationToken
  • Are booleans indexable in Postgres? If not, we should use integers instead.
 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?
 DONE
appendTSIncrement
  • interval' – perhaps a space before "'" for readability?
 DONE
getGUIDExpr
  • Remove "XXX: from the comment
  • Move the comment to the class header comment
 DONE
isImplicitConversion
  • srcType == Primitive.BOOLEAN ) - remove space before )
 DONE
isValidColumnName
  • Spaces around "-"
 DONE
addColumn
  • This might be worth refactoring further by adding a template method in order to avoid code duplication with the base class.
 DONE
{

} undo the change to ;

DONE
Retrieved from "https://wiki.cdot.senecapolytechnic.ca/w/index.php?title=PostgreSQL_Adapter_Project_-_Code_Review_4_Changes&oldid=69550"