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

Refactoring #1797

Closed
wants to merge 3 commits into from
Closed

Refactoring #1797

wants to merge 3 commits into from

Conversation

AlexanderStocks
Copy link

Description:

As part of my dissertation project, I've undertaken a sequence of automated refactorings and applied formatting with the goal to improve the quality of the codebase in this repository:

Reformat: Utilizing Google's Java Code Style guidelines, I have reformatted 534 files to enhance readability and ensure compliance with widely accepted style norms.

CollapseNestedIfStatements: I have executed the CollapseNestedIfStatements refactoring on 2 files. This refactoring effort is aimed at simplifying the logic of the code and making it more readable.

RemoveDuplication: I have executed the RemoveDuplication refactoring on 10 file, aiming to streamline the codebase by eliminating redundant code blocks.

These changes were executed using the Refactoring Janitor tool.

Request for Feedback:

As this initiative is part of my dissertation project, your comments, feedback, or suggestions about the changes I've made would be immensely valuable. Your insights will greatly contribute to my research and learning.

@manticore-projects
Copy link
Contributor

manticore-projects commented May 28, 2023

Greetings, Alexander.

Thank you very much for your contribution and welcome to JSQLParser. This is all great stuff, but unfortunately you were unlucky with the timing:

  1. there is a massive rewrite on-going, please see pending PR JSQLParser 5.0 #1778. Any of your changes should base on that since there are a lot of changes. Please see my verbose descriptions.

  2. we have already a Formatter and we do use Spotless with the Eclipse Formatter and a slightly modified Google Style. However, that has been applied only to submitted changes in order to keep the DIFF meaning full. We will apply that formatting to all of the code when the PRs with the structural changes got accepted. Never mix re-formatting with substantial changes of the code please.

  3. kudos for the removed duplications, however I will be able to comment only when re-based on the pending PR JSQLParser 5.0 #1778 (since it removed lots of redundant code already)

  4. I am undecided on the collapsed nested if statements and I want to explain why: JSQLParser has been written by many people over a very long period of time and it is crucial to have 'self documenting' code, which is easy to understand and to follow. So sometimes, a structure may not be the most elegant and modern way to write it -- but when it is the easiest way to understand what is happening and why, we would love to keep it that way. This does not mean, that we are not very interested in better code or dogmatic. All I want to say is: we need to decide on a case by case basis and there may be some changes we will kindly reject on our own discretion.

Again, thank you very much for your work. Do have a look at the massive pending changes and then we can pick it up from here. Do let me know how I can assist you to achieve the best out of both worlds. Please provide 1 PR only containing "Removed Duplicates", then one only for "Nested If statements" and only then Reformatting makes sense.

@manticore-projects
Copy link
Contributor

Btw, if you are really keen to format something, then it should be the JavaCC Grammar, which is an utter mess.
Every single production has been formatted in its own way following the taste and knowledge of the particular contributor.

Since there is no beautifier for JavaCC Grammars (I have had no time for that yet), it's an open field for making merit.

@@ -72,11 +74,7 @@ public String toString() {
if (indexExpression != null) {
return objExpression.toString() + "[" + indexExpression.toString() + "]";
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a perfect example for a reject: The Format may be Google style compliant, but it is harder to read how/why the string is composed. The logic is lost.

@AlexanderStocks AlexanderStocks closed this by deleting the head repository Aug 29, 2023
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