Update ModuleGenerator.java

[Gitusernamee]

User description

-Reduction of repetitions in argument and file management. -Use of streams and Collectors to simplify transformations. -Encapsulation of complex tasks into well-defined methods. -Use of try-with-resources for safe resource management.

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement

---

Description

• Refactored ModuleGenerator.java to reduce code repetition.

• Simplified argument parsing using Map.ofEntries.

• Encapsulated complex logic into helper methods for clarity.

• Improved resource management with try-with-resources.

---

Changes walkthrough 📝

Relevant files

Enhancement

ModuleGenerator.java Refactored `ModuleGenerator.java` for clarity and efficiency java/src/dev/selenium/tools/modules/ModuleGenerator.java Replaced repetitive argument parsing with Map.ofEntries. Encapsulated logic into helper methods like validateInputs and prepareJdepsArgs. Simplified resource handling using try-with-resources. Reduced code size by removing redundant logic and improving modularity. +129/-500

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The readServicesFromClasses() method returns an empty TreeSet instead of implementing the actual service reading logic, which could cause missing service dependencies private static Set<String> readServicesFromClasses(Path inJar) { return new TreeSet<>(); // Simulated service reading logic. } Resource Leak The temporary directory created by createTempDir() is not explicitly cleaned up after use, which could lead to resource leaks private static Path createTempDir() { return TemporaryFilesystem.getDefaultTmpFS().createTempDir("module-dir", "").toPath(); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Fix missing service provider detection The readServicesFromClasses method returns an empty set without actually reading any services. This could cause missing service providers in the module. java/src/dev/selenium/tools/modules/ModuleGenerator.java [193-195] -private static Set<String> readServicesFromClasses(Path inJar) { - return new TreeSet<>(); // Simulated service reading logic. +private static Set<String> readServicesFromClasses(Path inJar) throws IOException { + Set<String> services = new TreeSet<>(); + try (JarInputStream jis = new JarInputStream(Files.newInputStream(inJar))) { + JarEntry entry; + while ((entry = jis.getNextJarEntry()) != null) { + if (!entry.isDirectory() && entry.getName().endsWith(".class")) { + // Implement actual service reading logic here + // Read class files and look for ServiceLoader.load() calls + } + } + } + return services; } Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion addresses a critical issue where service providers are not being detected due to a placeholder implementation, which could lead to missing functionality in the module system.	9
General	Add descriptive error message Add error handling for the case when jdeps tool is not found. Currently the code will throw a generic exception if jdeps is not available. java/src/dev/selenium/tools/modules/ModuleGenerator.java [125] -ToolProvider jdeps = ToolProvider.findFirst("jdeps").orElseThrow(); +ToolProvider jdeps = ToolProvider.findFirst("jdeps") + .orElseThrow(() -> new IllegalStateException("jdeps tool not found. Please ensure JDK is properly installed.")); Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion improves error handling by providing a more descriptive error message when jdeps tool is not found, making it easier for users to diagnose and fix the issue.	6
Learned best practice	Add null parameter validation checks at the start of methods to prevent NullPointerExceptions The configureModule() method should validate its parameters at the start to prevent potential NullPointerExceptions. Add null checks for the required parameters unit, moduleName, and uses. java/src/dev/selenium/tools/modules/ModuleGenerator.java [158-161] private static ModuleDeclaration configureModule(CompilationUnit unit, String moduleName, boolean isOpen, Set<String> uses, Path inJar) throws IOException { + Objects.requireNonNull(unit, "CompilationUnit cannot be null"); + Objects.requireNonNull(moduleName, "Module name cannot be null"); + Objects.requireNonNull(uses, "Uses set cannot be null"); + ModuleDeclaration moduleDeclaration = unit.getModule() .orElseThrow(() -> new RuntimeException("No module declaration in module-info.java")); Apply this suggestion	6

[pujagani]

Please ensure formatting changes are done as part of a separate PR. Also, I suggest breaking down the PR into smaller changes that are grouped with a single motivation so that it is easy to review. Since this class impacts how CDP classes are generated and I don't think there is an easy way to test if there are any regressions with the changes made in this so we want to ensure no breaking changes go in.