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] type casting and numeric improvements #13909

Merged
merged 8 commits into from
May 6, 2024

Conversation

iampopovich
Copy link
Contributor

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

While reviewing the source code, I noticed that some sections contain unnecessary type conversions in arithmetic operations and string formatting.

Motivation and Context

I made part of these changes with the guidance of IntelliJ IDEA's code inspection. I believe that making such changes will help maintain it in a cleaner and more readable format for future maintenance.

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 conditional statements to switch-case in CdpClientGenerator.java for cleaner code.
  • Removed redundant toString calls and simplified exception messages in several test classes.
  • Optimized hash code calculation in Color.java using Long.hashCode.
  • Eliminated unnecessary division by 1 in mouse and pointer movement calculations across multiple test classes.

Changes walkthrough 📝

Relevant files
Enhancement
CdpClientGenerator.java
Refactor Conditional Statements to Switch-Case in toJson Method

java/src/org/openqa/selenium/devtools/CdpClientGenerator.java

  • Replaced conditional statements with a switch-case structure for type
    handling in toJson method.
  • +17/-12 
    AbstractFindByBuilder.java
    Simplify Exception Message Formatting in assertValidFindBy

    java/src/org/openqa/selenium/support/AbstractFindByBuilder.java

  • Removed redundant toString method call in exception message
    formatting.
  • +1/-1     
    Color.java
    Optimize Hash Code Calculation in Color Class                       

    java/src/org/openqa/selenium/support/Color.java

  • Replaced manual bitwise operation with Long.hashCode for hash code
    calculation.
  • +1/-1     
    DefaultMouseTest.java
    Remove Redundant Division in Mouse Movement Test                 

    java/test/org/openqa/selenium/bidi/input/DefaultMouseTest.java

    • Removed unnecessary division by 1 in mouse movement coordinates.
    +1/-1     
    DragAndDropTest.java
    Simplify Exception Message in DragAndDropTest                       

    java/test/org/openqa/selenium/bidi/input/DragAndDropTest.java

  • Simplified exception message by removing redundant toString method
    call.
  • +1/-1     
    DefaultMouseTest.java
    Clean Up Redundant Division in Mouse Interaction Test       

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

    • Removed unnecessary division by 1 in mouse movement coordinates.
    +1/-1     
    PenPointerTest.java
    Refactor Pointer Movement Calculations                                     

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

  • Removed unnecessary division by 1 in pointer movement coordinates.
  • Simplified calculation of center coordinates without casting.
  • +3/-3     

    💡 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 (0357d1f)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and mostly involve refactoring and simplification of existing code. The PR modifies type casting, simplifies switch-case statements, and updates some arithmetic operations. These changes are not complex but require careful review to ensure they maintain the same functionality.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The changes in arithmetic operations (e.g., removing division by 1) might alter the behavior if not tested thoroughly. For example, changing redSize.getWidth() / 1 + 1 to redSize.getWidth() + 1 in various tests could potentially lead to different outcomes if the original intent was misunderstood.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Performance
    Replace String.format with string concatenation for performance improvement.

    Replace the use of String.format with direct string concatenation in the toJson method to
    enhance performance, as String.format can be relatively slower and is not necessary for
    simple concatenations.

    java/src/org/openqa/selenium/devtools/CdpClientGenerator.java [931]

    -toJson.getBody().get().addStatement(String.format("return %s.toString();", propertyName));
    +toJson.getBody().get().addStatement("return " + propertyName + ".toString();");
     
    Best practice
    Ensure meaningful output of finders in exception messages.

    Ensure that the finders object's toString method provides a meaningful representation of
    its contents, as it is used in an exception message. If finders is a collection, consider
    overriding its toString method or using a utility to format its contents more readably.

    java/src/org/openqa/selenium/support/AbstractFindByBuilder.java [112]

    -throw new IllegalArgumentException(String.format("You must specify at most one location strategy. Number found: %d (%s)", finders.size(), finders));
    +throw new IllegalArgumentException(String.format("You must specify at most one location strategy. Number found: %d (%s)", finders.size(), finders.toString()));
     
    Add validation for element movement coordinates.

    Consider checking the validity of the coordinates before performing the moveToElement
    operation to ensure they are within the expected range, which could prevent runtime errors
    or unexpected behavior.

    java/test/org/openqa/selenium/bidi/input/DefaultMouseTest.java [484]

    +if (redSize.getWidth() + 1 > someMaxWidth || redSize.getHeight() + 1 > someMaxHeight) throw new IllegalArgumentException("Coordinates are out of bounds.");
     .moveToElement(redbox, redSize.getWidth() + 1, redSize.getHeight() + 1)
     
    Properly handle InterruptedException by resetting the interrupt flag.

    When catching InterruptedException, it's a good practice to reset the interrupt flag with
    Thread.currentThread().interrupt(), which allows other methods higher up the call stack to
    know about the interruption.

    java/test/org/openqa/selenium/bidi/input/DragAndDropTest.java [60]

    +Thread.currentThread().interrupt();
     throw new RuntimeException("Interrupted: " + e);
     
    Enhancement
    Simplify hashing logic in hashCode method.

    Use Long.hashCode(temp) directly for hashing temp in the hashCode method to simplify the
    code and potentially improve its readability and performance.

    java/src/org/openqa/selenium/support/Color.java [119]

    -result = 31 * result + Long.hashCode(temp);
    +result = 31 * result + (int) (temp ^ (temp >>> 32));
     

    @diemol
    Copy link
    Member

    diemol commented May 6, 2024

    Thank you, @iampopovich!

    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