Cleanup j2ee.persistence module #5444

pepness · 2023-02-07T02:09:07Z

I did some cleaning to the j2ee.persistence module with the Inspect & Transform tool. I reviewed each suggestion of the inspection tool with special emphasis on lambdas and try-with-resources and made sure not to break something. After this I plan to improve the JPA support to 2.2 or hopefully 3.1.

Use lambdas when possible (like new Runnable or calling an async task with RequestProcessor)
Use try-with-resources
Use URI.toURL() instead of new URL()
Use Utilities.toURI method
Use NbBundle.getMessage instead of NbBundle.getBundle
Remove redundant if expressions and if statements
if-else and loops statements should use braces
Remove unnecessary continue and return statements
Use Exception constructor when possible
Use multi-catch
Unnecessary boxing and primitive instantiation
Use diamond inference on declaration-initialization
Use generics
Use switch expression
Remove unused logger
Remove imports from the same package
Remove unused imports
Add missing Override annotations
Fix some typos
Fix some formatting
Add a TODO comment

I did full builds and fix the errors that pop out.

NetBeans Testing:

Full build done (with each commit)
Verify successful execution of libraries and licenses Ant test
Verify successful execution of Verify Sigtests
Verify successful execution of unit tests for module j2ee.persistence
Started NetBeans and ensure the log didn't have any ERROR or new WARNINGS
Successfully create different persistence units in different application servers.

mbien · 2023-02-07T02:21:59Z

@pepness I just merged a larger cleanup PR which caused conflicts here. Should be hopefully easy to resolve - bad timing sorry about that.

tbw777 · 2023-02-07T12:11:56Z

@pepness

Use switch expression

You mean replace if -> switch, not SE introduced in jvm14 ))
I think if/else more safe because you cant lost a "break" and no "default" case needed.

Add missing Override annotations

#5203 ALL CASES, OPENED

Use diamond inference on declaration-initialization

#5414 REJECTED, CLOSED
#5214 REJECTED, CLOSED

Use try-with-resources

#5328 MOST CASES/ALL, OPENED

Use lambdas when possible

In most cases equal lambdas are more slowly

Use generics

Changing interfaces are dangerous. But i think its a right way for thinking.

PS Mixing a lot of different changes in single multicommit PR isnt a good idea. Ty.

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/editor/ContextUtilities.java

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/editor/DocumentContext.java

matthiasblaesing · 2023-02-07T20:33:39Z

...e/src/org/netbeans/modules/j2ee/persistence/editor/completion/JPACodeCompletionProvider.java

@@ -146,7 +146,7 @@ protected void query(CompletionResultSet resultSet, Document doc, int caretOffse
                if (anchorOffset > -1) {
                    resultSet.setAnchorOffset(anchorOffset);
                }
-            } catch (Exception e) {
+            } catch (MissingResourceException | ParseException e) {


I would also catch RuntimeException

...2ee.persistence/src/org/netbeans/modules/j2ee/persistence/editor/completion/PUCompletor.java

...2ee.persistence/src/org/netbeans/modules/j2ee/persistence/spi/jpql/support/JPAAttribute.java

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/unit/PUDataObject.java

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/wizard/EntityClosure.java

....persistence/src/org/netbeans/modules/j2ee/persistence/wizard/fromdb/EntityClassesPanel.java

...j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/wizard/fromdb/SelectedTables.java

matthiasblaesing · 2023-02-07T21:29:26Z

@pepness thank you. In general looks good, but I left a few nitpicks inline.

mbien · 2023-02-07T23:46:56Z

@pepness wait, where did the commits go. now its only 3. Was that intended?

- Add a TODO comment - Fix some formatting

possible

Inspector Tool

- Use Utilities.toURI method - Use NbBundle.getMessage instead of NbBundle.getBundle - Redundant if expressions and if statements - Remove unnecessary continue and return statements - Use Exception constructor when possible

- Remove unused imports

- Unnecessary boxing - Use diamond inference on declaration-initialization - Use generics

-Unnecessary unboxing

- Remove unused logger - Inefficient Collection.toArray - Use character literal with indexOf - Use String.isEmpty()

pepness · 2023-02-08T02:12:14Z

@mbien I was working on them. Fixed the suggestions that @matthiasblaesing suggest

mbien · 2023-02-08T02:39:18Z

I was working on them. Fixed the suggestions that @matthiasblaesing suggest

@pepness ah, all good. I was just reviewing it and suddenly most of it was gone :)

mbien

looks good to me. Some comments inline for you to consider.

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/unit/PUDataObject.java

...stence/src/org/netbeans/modules/j2ee/persistence/wizard/fromdb/JavaPersistenceGenerator.java

...e/src/org/netbeans/modules/j2ee/persistence/wizard/jpacontroller/JpaControllerGenerator.java

mbien · 2023-02-08T05:26:14Z

...src/org/netbeans/modules/j2ee/persistence/editor/completion/ETCompletionContextResolver.java

-                        return metadata.getRoot().getEntity();
-                    }
-                });
+                entities = ecs.getEntityMappingsModel(false).runReadAction( (EntityMappingsMetadata metadata) -> metadata.getRoot().getEntity() );


just a nitpick. Consider the one-action-per-line formatting when method chaining is used. Makes the lines shorter.

entities = ecs.getEntityMappingsModel(false) .runReadAction( (EntityMappingsMetadata metadata) -> metadata.getRoot().getEntity() );

there are many comparable occurrences like this one

I like it that way too. Done

@pepness there are some more invocations of this method in the same style which you converted to lambdas btw. You changed only the one occurrence so far if I see this correctly.

search for getEntityMappingsModel(false). in
https://patch-diff.githubusercontent.com/raw/apache/netbeans/pull/5444.diff

mbien · 2023-02-08T05:43:18Z

just a general comment:
converting big blocks into lambdas basically loses the git blame history of the whole block. So I am not sure how useful that is all things considered. Maybe we shouldn't keep doing this in future. (Maybe consider reverting the largest candidates?)

Converting single char Strings into chars to use a different String method variant is also a stylistic change which has no runtime benefits anymore. Probably something we can skip over in future too.

matthiasblaesing

In general: Lets get this in. Some nitpicks inline, but I don't think this need another round of reviews. Github somehow thought every file was touched between my reviews, which makes it ugly to follow up.

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/unit/PUDataObject.java

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/wizard/EntityClosure.java

matthiasblaesing · 2023-02-09T18:02:42Z

Looks sane to me. Thank you.

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/editor/JPAEditorUtil.java

.../org/netbeans/modules/j2ee/persistence/editor/completion/db/DBCompletionContextResolver.java

...tence/src/org/netbeans/modules/j2ee/persistence/editor/completion/db/DBMetaDataProvider.java

...sistence/src/org/netbeans/modules/j2ee/persistence/entitygenerator/DbSchemaEntityMember.java

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/provider/ProviderUtil.java

...2ee.persistence/src/org/netbeans/modules/j2ee/persistence/spi/jpql/support/JPAAttribute.java

...src/org/netbeans/modules/j2ee/persistence/wizard/PersistenceClientEntitySelectionVisual.java

...2ee.persistence/src/org/netbeans/modules/j2ee/persistence/wizard/fromdb/DBSchemaManager.java

....persistence/src/org/netbeans/modules/j2ee/persistence/wizard/fromdb/EntityClassesPanel.java

...e/src/org/netbeans/modules/j2ee/persistence/wizard/jpacontroller/JpaControllerGenerator.java

- Fix some formatting - Use try-with-resources - Use lambdas when possible. Manually check every lambda with the Inspector Tool - if-else and loops statements should use braces - Use URI.toURL() instead of new URL() - Use Utilities.toURI method - Use NbBundle.getMessage instead of NbBundle.getBundle - Remove redundant if expressions and if statements - Remove unnecessary continue and return statements - Use Exception constructor when possible - Remove imports from the same package - Remove unused imports - Add missing Override annotations - Unnecessary boxing or unboxing - Use diamond inference on declaration-initialization - Use generics - Use multi-catch - Use switch statement - Remove unused logger - Inefficient Collection.toArray - Use character literal with indexOf - Use String.isEmpty()

pepness added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) enterprise [ci] enable enterprise job labels Feb 7, 2023

pepness added this to the NB18 milestone Feb 7, 2023

pepness self-assigned this Feb 7, 2023

mbien added the Code cleanup label Feb 7, 2023

matthiasblaesing reviewed Feb 7, 2023

View reviewed changes

pepness force-pushed the clean-persistence branch from 199970d to c00c34d Compare February 7, 2023 23:35

pepness added 12 commits February 7, 2023 18:37

- Fix some typos

0ba6091

- Add a TODO comment - Fix some formatting

Use try-with-resources

a94d5e0

Search for 'new Runnable()' and change it to a lambda expression when

0184734

possible

Use lambdas when possible. Manually check every lambda with the

5965d57

Inspector Tool

if-else and loops statements should use braces

1dbcb5b

- Use URI.toURL() instead of new URL()

33d2673

- Use Utilities.toURI method - Use NbBundle.getMessage instead of NbBundle.getBundle - Redundant if expressions and if statements - Remove unnecessary continue and return statements - Use Exception constructor when possible

- Remove imports from the same package

a7135ae

- Remove unused imports

Add missing Override annotations

c783ae8

- Box primitive instantiation

388e2a6

- Unnecessary boxing - Use diamond inference on declaration-initialization - Use generics

-Use multi-catch

d8d3fdf

-Unnecessary unboxing

- Use switch expression

5f0f2b4

- Remove unused logger - Inefficient Collection.toArray - Use character literal with indexOf - Use String.isEmpty()

Resolve GitHub review

250fcf9

pepness force-pushed the clean-persistence branch from c00c34d to 250fcf9 Compare February 8, 2023 01:47

mbien approved these changes Feb 8, 2023

View reviewed changes

matthiasblaesing approved these changes Feb 8, 2023

View reviewed changes

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/unit/PUDataObject.java Outdated Show resolved Hide resolved

java/j2ee.persistence/src/org/netbeans/modules/j2ee/persistence/wizard/EntityClosure.java Outdated Show resolved Hide resolved

Resolve GitHub review #2

efc7d79

noelma reviewed Feb 9, 2023

View reviewed changes

pepness merged commit c122fc6 into apache:master Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup j2ee.persistence module #5444

Cleanup j2ee.persistence module #5444

pepness commented Feb 7, 2023

mbien commented Feb 7, 2023

tbw777 commented Feb 7, 2023 •

edited

Loading

matthiasblaesing Feb 7, 2023

matthiasblaesing commented Feb 7, 2023

mbien commented Feb 7, 2023

pepness commented Feb 8, 2023

mbien commented Feb 8, 2023

mbien left a comment

mbien Feb 8, 2023

pepness Feb 8, 2023

mbien Feb 10, 2023

mbien commented Feb 8, 2023

matthiasblaesing left a comment

matthiasblaesing commented Feb 9, 2023

Cleanup j2ee.persistence module #5444

Cleanup j2ee.persistence module #5444

Conversation

pepness commented Feb 7, 2023

mbien commented Feb 7, 2023

tbw777 commented Feb 7, 2023 • edited Loading

matthiasblaesing Feb 7, 2023

Choose a reason for hiding this comment

matthiasblaesing commented Feb 7, 2023

mbien commented Feb 7, 2023

pepness commented Feb 8, 2023

mbien commented Feb 8, 2023

mbien left a comment

Choose a reason for hiding this comment

mbien Feb 8, 2023

Choose a reason for hiding this comment

pepness Feb 8, 2023

Choose a reason for hiding this comment

mbien Feb 10, 2023

Choose a reason for hiding this comment

mbien commented Feb 8, 2023

matthiasblaesing left a comment

Choose a reason for hiding this comment

matthiasblaesing commented Feb 9, 2023

tbw777 commented Feb 7, 2023 •

edited

Loading