Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: huntbugs on PgDatabaseMetaData #693

Merged
merged 1 commit into from Dec 30, 2016
Merged

fix: huntbugs on PgDatabaseMetaData #693

merged 1 commit into from Dec 30, 2016

Conversation

@jorsol
Copy link
Member

@jorsol jorsol commented Nov 24, 2016

Class: PgDatabaseMetaData

Fix one "Inefficient number constructor"
Fix three "String concatenation in a loop"

related #586

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 24, 2016

Current coverage is 64.06% (diff: 100%)

Merging #693 into master will decrease coverage by 0.01%

@@             master       #693   diff @@
==========================================
  Files           165        165          
  Lines         15129      15132     +3   
  Methods           0          0          
  Messages          0          0          
  Branches       2978       2978          
==========================================
  Hits           9694       9694          
- Misses         4201       4202     +1   
- Partials       1234       1236     +2   

Powered by Codecov. Last update f48c6bb...a8608d4

Copy link
Member

@vlsi vlsi left a comment

Please revert .append(...).append(...) for cases when string literals are appended

+ " WHERE c.relnamespace = n.oid ";
StringBuilder select = new StringBuilder();
select.append("SELECT NULL AS TABLE_CAT, n.nspname AS TABLE_SCHEM, c.relname AS TABLE_NAME, ")
.append(" CASE n.nspname ~ '^pg_' OR n.nspname = 'information_schema' ")

This comment has been minimized.

@vlsi

vlsi Nov 25, 2016
Member

aaaahh. Why did you do that?
These should be just plain old string concatenations.
Usage of appends here adds no value and makes code hard to read.

What I mean is concatenation of string literals is compiled into a single string by javac. So patterns like "hello " + "world" are fine in any context.

This comment has been minimized.

@jorsol

jorsol Nov 25, 2016
Author Member

At first yes I agree it probably makes a little hard to read but every String converted to StringBuilder here was because they are used on a loop, so a "hello " + "world" pattern is broken in this context, javac can't optimize that kind of concatenation, that why huntbugs emits a warning. This is a performance vs readability choice.

Anyway if you build a dynamic query like this causes the readability of it is harder even with plain old string concatenations, someone ends up printing a debug message.

This comment has been minimized.

@davecramer

davecramer Nov 25, 2016
Member

This is not really in a critical path from a performance perspective.

I'd be ok with creating the StringBuilder just before the loop


if (schemaPattern != null && !schemaPattern.isEmpty()) {
select += " AND n.nspname LIKE " + escapeQuotes(schemaPattern);
select.append(" AND n.nspname LIKE ").append(escapeQuotes(schemaPattern));

This comment has been minimized.

@vlsi

vlsi Nov 25, 2016
Member

appends here are fine

@davecramer
Copy link
Member

@davecramer davecramer commented Nov 25, 2016

Agreed this makes the code harder to read.

@jorsol jorsol force-pushed the jorsol:fix-huntbugs branch from 140e230 to a8608d4 Nov 25, 2016
@jorsol
Copy link
Member Author

@jorsol jorsol commented Nov 25, 2016

What about now?

@davecramer
Copy link
Member

@davecramer davecramer commented Dec 7, 2016

LGTM now

@vlsi vlsi added this to the 42.0.0 milestone Dec 30, 2016
@vlsi vlsi merged commit 3a00ef9 into pgjdbc:master Dec 30, 2016
2 checks passed
2 checks passed
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +35.92% compared to f48c6bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorsol jorsol deleted the jorsol:fix-huntbugs branch Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants