[ZEPPELIN-6299] Refactor StaticRepl for readability, correctness, and modern Java usage#5048
Conversation
…unnecessary compare statement
- Replace Arrays.asList to List.of - Move variable definitions closer to where it is used - Introduce diamond operator <>
ParkGyeongTae
left a comment
There was a problem hiding this comment.
Everything looks really good! I was wondering if this code could be placed a bit higher.
What’s your thought?
| System.setErr(newErr); | ||
|
|
||
| JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); | ||
| DiagnosticCollector<JavaFileObject> diagnostics = new DiagnosticCollector<>(); |
There was a problem hiding this comment.
Maybe consider moving this code a bit higher?
There was a problem hiding this comment.
I previously suggested moving DiagnosticCollector<JavaFileObject> diagnostics = new DiagnosticCollector<>(); to the top, but that was my mistake.
What I really meant is that we should move JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); to the top.
The reason is that ToolProvider.getSystemJavaCompiler() can return null if running with a JRE instead of a JDK.
By checking this early, we can fail fast before doing unnecessary work like parsing the code or redirecting System.out/err.
There was a problem hiding this comment.
Oh I see, thanks for the point!
Would it make sense to throw an exception here if JavaCompiler is null, since running Java code requires a JDK?
Since this seems more related to functional stability, I feel it could be handled in a separate PR. How do you feel about that?
There was a problem hiding this comment.
My personal opinion is that it would be good to throw an exception if JavaCompiler is null.
I also think it would be better to handle this in a separate PR.
There was a problem hiding this comment.
Thanks for your opinion 👍 I have created another Jira Issue for this. https://issues.apache.org/jira/browse/ZEPPELIN-6306
ParkGyeongTae
left a comment
There was a problem hiding this comment.
I have checked the existing unit tests.
… modern Java usage
### What is this PR for?
This PR refactors the `StaticRepl` class to improve readability, maintainability, and align with modern Java best practices.
There are no functional changes.
- Generic type refinement with diamond operator in loop: Adds type safety by explicitly specifying the generic type. Prevents unchecked warnings and makes the code clearer to readers and tools.
- Replace `Arrays.asList` to `List.of` : List.of (Java 9+) produces an immutable list, which is safer and better communicates the intent that the list will not be modified. It avoids side effects of Arrays.asList, which is fixed-size and backed by the original array.
- Minor typos fixed: Corrected spelling mistakes and adjusted comments.
- Removes redundant boolean comparison to follow standard Java coding conventions.
- Replace index-based loop with enhanced for-each to eliminate boilerplate index management, improves readability, and reduces chances of off-by-one errors. It makes the intent ("iterate all elements") clearer.
- Variable declaration moved closer to usage to improve code locality and readability by reducing variable scope. This makes the code easier to maintain and follow.
### What type of PR is it?
Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* [ZEPPELIN-6299](https://issues.apache.org/jira/browse/ZEPPELIN-6299)
### How should this be tested?
* Run existing unit tests to confirm behavior remains unchanged.
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5048 from eunhwa99/ZEPPELIN-6299.
Signed-off-by: ParkGyeongTae <[email protected]>
(cherry picked from commit f45587d)
Signed-off-by: ParkGyeongTae <[email protected]>
|
Merged into |
What is this PR for?
This PR refactors the
StaticReplclass to improve readability, maintainability, and align with modern Java best practices.There are no functional changes.
Arrays.asListtoList.of: List.of (Java 9+) produces an immutable list, which is safer and better communicates the intent that the list will not be modified. It avoids side effects of Arrays.asList, which is fixed-size and backed by the original array.What type of PR is it?
Refactoring
Todos
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: