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

[java] Consistent UTF-8 Encoding and Code Enhancements #14218

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 1, 2024

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

  • 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

    Copy link

    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.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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]

     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
    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]

    -String decoded = URLDecoder.decode(filters, StandardCharsets.UTF_8);
    -Map<String, Object> raw = new Json().toType(decoded, MAP_TYPE);
    +Map<String, Object> 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]

    -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]

    -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

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants