Skip to content

Added: SQL builder toString function#730

Merged
vorburger merged 1 commit into
apache:developfrom
thesmallstar:test808
Mar 12, 2020
Merged

Added: SQL builder toString function#730
vorburger merged 1 commit into
apache:developfrom
thesmallstar:test808

Conversation

@thesmallstar
Copy link
Copy Markdown
Member

@thesmallstar thesmallstar commented Mar 10, 2020

Description

To continue to work done in #725 by @vorburger this PR intends to complete one of the remaining tasks.
Please use the following for functional testing: http://tpcg.io/QfG1joot [outdated] (directly run for sample output)
After this PR, I intend to write proper test statements as required.
Note: Another approach could have been directly using sb, I decided to do it this way any suggestions are welcome :)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

Copy link
Copy Markdown
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR, I intend to write proper test statements as required.

If I may be a pain 😈 it is typically considered much better practice to do something like this and have the test in the same PR.. in this particular case, just commenting out the x3 related TODO in SQLBuilderTest.java as part of this PR would be great.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StringBuilder whereClause = new StringBuilder();
StringBuilder whereClause = new StringBuilder("SQLBuilder{ ");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "SQLBuilder{"+ whereClause.toString() +"}";
whereClause.append(" }");
return whereClause.toString();

The point of this is to avoid unnecessary String concatenation object creation.

You should also avoid any other String + in this method - just append() everything.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can probably avoid creating this intermediate list? Just iterate and append to the StringBuilder, no? Or perhaps I'm missing something here (quick review only; didn't read the code in full details).

@thesmallstar
Copy link
Copy Markdown
Member Author

Thank you for the review, working on the required changes.
Thanks- Manthan

@thesmallstar thesmallstar force-pushed the test808 branch 2 times, most recently from 3d87262 to 1be08bb Compare March 10, 2020 15:37
@thesmallstar thesmallstar requested a review from vorburger March 10, 2020 15:40
Copy link
Copy Markdown
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Minor feedback, if you want to further optimize? Just for the fun of it... ;-)

PS: Also formatting is non-standard, but I can't hold that against you so won't be a pain, let's ignore; it will all get bulk re-formatted at some future point (what we really need is more Checkstyle.. want to help with that?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could still save this StringBuilder and just whereClause.append(...) as you go? Just a thought.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, on it.

@thesmallstar
Copy link
Copy Markdown
Member Author

@vorburger No pain, I am willing to learn the correct way( I plan to stay with fineract for long :P)

  1. Can you give a quick link to refer the correct formatting?
  2. Please tell how can we optimize this? ( Maybe something I suggested in the Note above, replacing '?')
  3. Want to help with the Checkstyle :D

Copy link
Copy Markdown
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'll merge when build passed.

@vorburger
Copy link
Copy Markdown
Member

@vorburger vorburger merged commit 93d1716 into apache:develop Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants