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] Reduce redundant toString() calls #13932

Merged
merged 6 commits into from
May 13, 2024

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented May 11, 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

In the changes, I removed the explicit casting to the string type in concatenation operations and substring replacement. These operations are performed implicitly when operating on strings.

Motivation and Context

The changes are minor and mostly pertain to the quality and readability of the code.

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

  • Removed unnecessary semicolons and redundant toString() calls across various classes to enhance code readability and performance.
  • Simplified exception message constructions and proxy element descriptions.
  • Corrected syntax errors in test classes related to proxy settings.

Changes walkthrough 📝

Relevant files
Formatting
2 files
FetchError.java
Cleanup of try-with-resources syntax                                         

java/src/org/openqa/selenium/bidi/network/FetchError.java

  • Removed unnecessary semicolon in try-with-resources statement.
+1/-1     
SerializationOptions.java
Cleanup enum declaration syntax                                                   

java/src/org/openqa/selenium/bidi/script/SerializationOptions.java

  • Removed unnecessary semicolon from enum declaration.
+1/-1     
Enhancement
8 files
EvaluateResult.java
Simplify access modifier in enum declaration                         

java/src/org/openqa/selenium/bidi/script/EvaluateResult.java

  • Removed redundant 'public' access modifier from enum declaration.
+1/-1     
RedisBackedSessionMap.java
Remove redundant toString() calls in RedisBackedSessionMap

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java

  • Removed redundant toString() calls on SessionId objects in various key
    generation methods.
  • +4/-4     
    JsonInput.java
    Simplify exception message construction                                   

    java/src/org/openqa/selenium/json/JsonInput.java

    • Simplified string concatenation in exception message.
    +1/-1     
    ErrorCodec.java
    Remove redundant toString() calls in ErrorCodec                   

    java/src/org/openqa/selenium/remote/ErrorCodec.java

    • Removed redundant toString() calls in exception messages.
    +2/-2     
    LocatingElementHandler.java
    Simplify proxy element description generation                       

    java/src/org/openqa/selenium/support/pagefactory/internal/LocatingElementHandler.java

    • Simplified toString() usage in proxy element description.
    +1/-1     
    DragAndDropTest.java
    Simplify exception message in DragAndDropTest                       

    java/test/org/openqa/selenium/interactions/DragAndDropTest.java

    • Removed redundant toString() call in exception handling.
    +1/-1     
    TestFileLocator.java
    Optimize path replacement in TestFileLocator                         

    java/test/org/openqa/selenium/javascript/TestFileLocator.java

  • Simplified path replacement logic by removing redundant toString()
    call.
  • +1/-1     
    JsonTest.java
    Streamline JSON serialization test                                             

    java/test/org/openqa/selenium/json/JsonTest.java

    • Removed redundant toString() call in JSON serialization test.
    +1/-1     
    Bug fix
    1 files
    ProxySettingTest.java
    Syntax correction in ProxySettingTest                                       

    java/test/org/openqa/selenium/ProxySettingTest.java

  • Corrected syntax for try-with-resources in proxy configuration tests.
  • +2/-2     

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

    Copy link
    Contributor

    PR Description updated to latest commit (06d0453)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and mostly involve removing redundant toString() calls and minor syntax adjustments. The PR is focused on code cleanup and readability improvements, which are generally easier to review than changes that involve complex logic or new features.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The removal of toString() in error and logging messages could potentially change the output if the default toString() implementation is not overridden in the object being logged or thrown. This should be verified to ensure that the output remains informative and useful.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Add null check before converting to JSON to prevent potential null pointer exceptions.

    Consider checking if jsonMap.get("errorText") returns null before converting it to JSON to
    avoid potential NullPointerException.

    java/src/org/openqa/selenium/bidi/network/FetchError.java [45]

    -String errorText = JSON.toJson(jsonMap.get("errorText"));
    +Object errorTextObj = jsonMap.get("errorText");
    +String errorText = errorTextObj != null ? JSON.toJson(errorTextObj) : "Error text not available";
     
    Enhancement
    Improve error messages for better debugging and clarity.

    Add more specific error messages in the IllegalArgumentException to help identify which
    part of the response map is incorrect.

    java/src/org/openqa/selenium/remote/ErrorCodec.java [147]

    -throw new IllegalArgumentException("Unable to find mapping for " + response);
    +throw new IllegalArgumentException("Expected 'value' key to be a Map in response: " + response);
     
    Enhance exception handling by including the root cause in the thrown exception.

    Include the exception message in the JsonException to provide more context about the
    NumberFormatException.

    java/src/org/openqa/selenium/json/JsonInput.java [249]

    -throw new JsonException("Unable to parse to a number: " + builder + ". " + input);
    +throw new JsonException("Unable to parse to a number: " + builder + ". " + input, e);
     
    Maintainability
    Refactor string concatenation to use String.format for better readability.

    Use String.format for constructing Redis keys to enhance readability and maintainability.

    java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java [319]

    -return "session:" + id + ":uri";
    +return String.format("session:%s:uri", id);
     
    Best practice
    Handle thread interruption status properly after an InterruptedException.

    Restore the interrupted status after catching InterruptedException to allow higher-level
    handlers to react.

    java/test/org/openqa/selenium/interactions/DragAndDropTest.java [46]

    +Thread.currentThread().interrupt();
     throw new RuntimeException("Interrupted: " + e);
     

    @diemol diemol merged commit e7324ef into SeleniumHQ:trunk May 13, 2024
    38 of 40 checks passed
    @iampopovich iampopovich deleted the reduce-redundant-calls branch May 13, 2024 11:52
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    * removed unnecessary semicolons
    
    * removed redundant access modifier for interface member
    
    * removed redundant toString() calls for id in RedisBackedSessionMap
    
    * removed redundant toString() call in DragAndDropTest
    
    * removed redundant toString() calls
    
    * applying formatting
    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.

    2 participants