SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.77k stars 8.2k forks source link

[java] applying variable matching pattern for instanceof #14315

Closed iampopovich closed 3 months ago

iampopovich commented 3 months 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

After switching to JDK17 in the project, the use of a shorter instanceof check syntax became available.

Reference: https://docs.oracle.com/en/java/javase/17/language/pattern-matching-instanceof-operator.html

There are quite a few such checks in the code, so I created a small pull request to avoid changing all the files at once.

Motivation and Context

I made changes to utilize the latest language features and to prevent the static analyzer from raising warnings about the possibility of using the compact form.

Types of changes

Checklist


PR Type

Enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
By.java
Use pattern matching for `instanceof` in `By` class           

java/src/org/openqa/selenium/By.java
  • Updated equals method to use pattern matching for instanceof.
  • Removed redundant type casting.
  • +2/-5     
    Cookie.java
    Use pattern matching for `instanceof` in `Cookie` class   

    java/src/org/openqa/selenium/Cookie.java
  • Updated equals method to use pattern matching for instanceof.
  • Removed redundant type casting.
  • +1/-3     
    Event.java
    Use pattern matching for `instanceof` in `Event` class     

    java/src/org/openqa/selenium/events/Event.java
  • Updated equals method to use pattern matching for instanceof.
  • Removed redundant type casting.
  • +1/-2     
    Response.java
    Use pattern matching for `instanceof` in `Response` class

    java/src/org/openqa/selenium/remote/Response.java
  • Updated equals method to use pattern matching for instanceof.
  • Removed redundant type casting.
  • Simplified fromJson method using pattern matching for instanceof.
  • +4/-8     

    πŸ’‘ 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 3 months ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review: 2 πŸ”΅πŸ”΅βšͺβšͺβšͺ
    πŸ§ͺ No relevant tests
    πŸ”’ No security concerns identified
    ⚑ No key issues to review
    codiumai-pr-agent-pro[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a null check to prevent potential NullPointerException ___ **Consider checking for null before using the instanceof pattern to avoid potential
    NullPointerException. This is a safeguard in case the equals method is called with a
    null argument.** [java/src/org/openqa/selenium/By.java [162]](https://github.com/SeleniumHQ/selenium/pull/14315/files#diff-854cf2c9c399c8699c85f72d95e00cad282a9aacecc851cab5ab59c404acde13R162-R162) ```diff -if (!(o instanceof By that)) { +if (o == null || !(o instanceof By that)) { return false; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Adding a null check before using the `instanceof` pattern is crucial to prevent potential NullPointerException, which is a common safeguard in equals methods.
    10
    Add a type check for json.get("message") to ensure it is a String ___ **Ensure that the json.get("message") is an instance of String before casting, to
    prevent a ClassCastException if it is not a String.** [java/src/org/openqa/selenium/remote/Response.java [99-102]](https://github.com/SeleniumHQ/selenium/pull/14315/files#diff-c7ac2566acafc57a89c6001b59f909b329af762f5718b4509bc5aa14f4cdb199R99-R102) ```diff -if (json.get("error") instanceof String state) { +if (json.get("error") instanceof String state && json.get("message") instanceof String message) { response.setState(state); response.setStatus(errorCodes.toStatus(state, Optional.empty())); - response.setValue(json.get("message")); + response.setValue(message); } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Ensuring that `json.get("message")` is an instance of `String` before casting prevents potential ClassCastException, which is a significant improvement for robustness.
    10
    Best practice
    Use Objects.equals for null-safe comparison of name ___ **Consider using Objects.equals for comparing name with cookie.name to handle
    potential null values more safely.** [java/src/org/openqa/selenium/Cookie.java [281-282]](https://github.com/SeleniumHQ/selenium/pull/14315/files#diff-a67a17fddb3eb96bacf0cd0d4275a94982dd4b0f8485a3e31cd5edbcfb39b04aR281-R282) ```diff -if (!name.equals(cookie.name)) { +if (!Objects.equals(name, cookie.name)) { return false; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Using `Objects.equals` for comparing `name` with `cookie.name` is a best practice for null-safe comparisons, improving code robustness.
    8
    diemol commented 3 months ago

    The build target is still Java 11. I don't remember. Do these code changes work in Java 11?

    iampopovich commented 3 months ago

    If target is still java 11 let's close this pr - it's incompatible with java 11 @diemol Thank you for the clarification.

    diemol commented 3 months ago

    We plan to bump to 17 early next year.

    Thanks,