[java] Consistent UTF-8 Encoding and Code Enhancements

iampopovich commented 2 days ago

User description

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.

Description

Ensured consistent use of UTF-8 encoding throughout the codebase.
Replaced traditional for loop with enhanced for-each loop in DockerOptions.
Removed unnecessary unboxing in the noErrorNoCry test.

Motivation and Context

These changes improve code readability, maintainability, and performance.

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

[x] 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, Bug fix

Description

Ensured consistent use of UTF-8 encoding throughout the codebase by replacing UnsupportedEncodingException handling with StandardCharsets.UTF_8.
Simplified node retrieval logic in LocalDistributor using Optional.map.
Replaced traditional for loop with enhanced for-each loop in DockerOptions.
Removed unnecessary unboxing in the noErrorNoCry test.

Changes walkthrough 📝

Relevant files

Enhancement

8 files

LocalDistributor.java `Simplified node retrieval logic with Optional.map` java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java - Simplified node retrieval logic using `Optional.map`.	+1/-5
DockerOptions.java `Replaced traditional for loop with enhanced for-each loop` java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java - Replaced traditional for loop with enhanced for-each loop.	+2/-2
Urls.java `Use StandardCharsets.UTF_8 for URL encoding` java/src/org/openqa/selenium/net/Urls.java Replaced `UnsupportedEncodingException` handling with `StandardCharsets.UTF_8`.	+2/-6
FormEncodedData.java `Use StandardCharsets.UTF_8 for URL decoding` java/src/org/openqa/selenium/remote/http/FormEncodedData.java Replaced charset string with `StandardCharsets.UTF_8` in URL decoding.	+1/-1
ReferrerTest.java `Simplified URL encoding in tests` java/test/org/openqa/selenium/ReferrerTest.java Simplified URL encoding by removing `UnsupportedEncodingException` handling.	+1/-6
ListImagesTest.java `Use StandardCharsets.UTF_8 for URL decoding in tests` java/test/org/openqa/selenium/docker/v1_41/ListImagesTest.java Replaced `UnsupportedEncodingException` handling with `StandardCharsets.UTF_8`.	+15/-19
MainTest.java `Simplified PrintStream creation in tests` java/test/org/openqa/selenium/grid/MainTest.java Simplified `PrintStream` creation by removing `UnsupportedEncodingException` handling.	+1/-6
FormEncodedDataTest.java `Simplified URL encoding in tests` java/test/org/openqa/selenium/remote/http/FormEncodedDataTest.java Simplified URL encoding by removing `UnsupportedEncodingException` handling.	+5/-10

Bug fix

1 files

W3CHttpResponseCodecTest.java

Removed unnecessary unboxing in test assertions

java/test/org/openqa/selenium/remote/codec/w3c/W3CHttpResponseCodecTest.java - Removed unnecessary unboxing in assertions.

+1/-1

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 2 days ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The refactoring in `LocalDistributor.java` uses `map` and `orElse` which might not be equivalent to the original logic if `getNodeId()` can return `null` and is expected to be handled.
	Code Improvement: In `DockerOptions.java`, the change from index-based loop to enhanced for-loop is good for readability but ensure that the logic inside the loop does not depend on the index.
	Code Cleanup: The changes in `Urls.java`, `FormEncodedData.java`, and various test files to use `StandardCharsets.UTF_8` instead of `"UTF-8"` string are good for type safety and avoiding unnecessary exceptions.

codiumai-pr-agent-pro[bot] commented 2 days ago

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Simplify the for-loop by removing unnecessary variable assignments ___ Use the enhanced for-loop directly on the `devices` list without intermediate variable assignment for trimming, to simplify the loop and reduce redundancy. [java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java [210-211]](https://github.com/SeleniumHQ/selenium/pull/14218/files#diff-5a4e578c5c5711a9b26d92a059bfe58b426177f8d2fc68aaaa79cd0fbef456bdR210-R211) ```diff for (String device : devices) { - String deviceMappingDefined = device.trim(); + Matcher matcher = linuxDeviceMappingWithDefaultPermissionsPattern.matcher(device.trim()); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion simplifies the loop and reduces redundancy, improving code readability and maintainability.	8
Maintainability	Reduce variable usage by inlining expressions ___ Inline the `decoded` variable directly into the `toType` method call to streamline the code and reduce the number of lines. [java/test/org/openqa/selenium/docker/v1_41/ListImagesTest.java [44-45]](https://github.com/SeleniumHQ/selenium/pull/14218/files#diff-9d6fce0e3e1c13ec6a6ba9cad3106b8c1e03282c0ace46692dc5cc096ae6e0b3R44-R45) ```diff -String decoded = URLDecoder.decode(filters, StandardCharsets.UTF_8); -Map raw = new Json().toType(decoded, MAP_TYPE); +Map raw = new Json().toType(URLDecoder.decode(filters, StandardCharsets.UTF_8), MAP_TYPE); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Inlining the variable reduces the number of lines and simplifies the code, making it more concise without losing clarity.	7
Enhancement	Improve readability and explicit handling of optional values ___ Consider using `Optional.ifPresentOrElse` to handle the optional value more explicitly instead of `map().orElse()`. This can make the code more readable by clearly separating the actions for present and absent cases. [java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java [881]](https://github.com/SeleniumHQ/selenium/pull/14218/files#diff-024d7dccb0a4ff87c444dc3549032e7c6b2595f582f4f267cdf48c7f8c2f87c1R881-R881) ```diff -return nodeStatus.map(status -> nodes.get(status.getNodeId())).orElse(null); +nodeStatus.ifPresentOrElse( + status -> return nodes.get(status.getNodeId()), + () -> return null +); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves readability and explicitly handles the optional value, making the code more maintainable. However, the current implementation is also correct and clear.	7
Best practice	Use static import for `UTF_8` to clean up the code ___ Since `StandardCharsets.UTF_8` is a constant, you can import it statically to make the code cleaner. [java/src/org/openqa/selenium/net/Urls.java [45]](https://github.com/SeleniumHQ/selenium/pull/14218/files#diff-1e1c4b5c9eca0a6339003cf35d6de1224426f6af73007b41ef0fc6d9cc0130adR45-R45) ```diff -return URLEncoder.encode(value, StandardCharsets.UTF_8); +import static java.nio.charset.StandardCharsets.UTF_8; +... +return URLEncoder.encode(value, UTF_8); ``` Suggestion importance[1-10]: 6 Why: Using a static import for `UTF_8` can clean up the code slightly, but the improvement is minor and more a matter of style preference.	6

SeleniumHQ / selenium