Skip to content

Conversation

@LuisOsv
Copy link
Contributor

@LuisOsv LuisOsv commented Jun 3, 2025

User description

🔗 Related Issues

N/A — performance and readability enhancement

💥 What does this PR do?

This PR improves performance and readability by caching the size or length of collections and arrays before entering loops.
Instead of repeatedly calling .size() or .length in the loop condition, the value is stored in a local variable and reused.

🔧 Implementation Notes

This change is based on a common Java performance recommendation highlighted in High Performance with Java by Javier Fernandez Gonzalez (O’Reilly, 2024).
The book explains that repeatedly calling methods like .size() inside loop conditions is less efficient than caching their values in local variables. This approach improves both runtime performance and code readability.
Example refactor:

int size = list.size();
for (int i = 0; i < size; i++) {
    // logic
}

Instead of:

for (int i = 0; i < list.size(); i++) {
    // logic
}

💡 Additional Considerations

  • 🔒 No behavioral or logic changes were made.
  • ✅ 100% backwards compatible
  • 🔍 The refactor strictly targets loop conditions for micro-optimization and readability.
  • 🧼 Improves code maintainability by reducing repetition.

🔄 Types of changes

  • ✅ Cleanup (formatting, renaming)
  • ✅ Performance improvement (non-breaking change)

🧪 Tests Executed
To ensure nothing was broken by this cleanup, the following affected test targets were executed:

bazel test //java/test/org/openqa/selenium/support:SmallTests
bazel test //java/test/org/openqa/selenium/support/pagefactory:SmallTests
bazel test //java/test/org/openqa/selenium/remote:small-tests
bazel test //java/test/org/openqa/selenium/remote/http:small-tests
bazel test //java/test/org/openqa/selenium/support/ui:SmallTests
bazel test //java/test/org/openqa/selenium/grid/node/docker:small-tests
bazel test //java/test/org/openqa/selenium/grid/node/relay:medium-tests
bazel test //java/test/org/openqa/selenium/json:SmallTests

✅ All tests passed successfully.


PR Type

Enhancement


Description

  • Cache collection/array size in loops for performance

  • Refactor for improved code readability and maintainability

  • No behavioral or logic changes introduced

  • Applied changes across grid, remote, tools, json, and support packages


Changes walkthrough 📝

Relevant files
Enhancement
14 files
JavadocJarMaker.java
Cache argument array length before loop for efficiency     
+2/-1     
ModuleGenerator.java
Store argument array length before loop iteration               
+2/-1     
DockerOptions.java
Cache config list size before loop for performance             
+3/-2     
RelayOptions.java
Store config list size before loop for clarity                     
+3/-2     
JsonInput.java
Cache string length before loop for efficiency                     
+2/-1     
WebElementToJsonConverter.java
Cache array length before loop and pre-size list                 
+3/-2     
AbstractHttpCommandCodec.java
Cache list size before loop for efficiency                             
+5/-3     
Route.java
Cache prefix path array length before loop                             
+3/-2     
UrlTemplate.java
Cache group list size before switch and loop                         
+3/-2     
FindAll.java
Cache annotation array length before loop                               
+3/-2     
FindBys.java
Cache annotation array length before loop                               
+3/-2     
EventFiringDecorator.java
Cache parameter array length before loop and comparison   
+3/-2     
ByChained.java
Cache locator array length before loop and checks               
+3/-2     
Quotes.java
Cache substring array length before loop for efficiency   
+3/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @CLAassistant
    Copy link

    CLAassistant commented Jun 3, 2025

    CLA assistant check
    All committers have signed the CLA.

    @selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings B-support Issue or PR related to support classes labels Jun 3, 2025
    @selenium-ci
    Copy link
    Member

    Thank you, @LuisOsv for this code suggestion.

    The support packages contain example code that many users find helpful, but they do not necessarily represent
    the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

    We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
    If you have any questions, please contact us

    @selenium-ci selenium-ci closed this Jun 3, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ConnectFailure error when instantiating ChromeDriver multiple times

    Requires further human verification:

    • The PR doesn't address the ChromeDriver connection issue at all, as it's focused on loop optimization rather than connection handling

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where JavaScript in link's href is not triggered on click() in Firefox

    Requires further human verification:

    • The PR doesn't address the Firefox JavaScript click issue at all, as it's focused on loop optimization rather than Firefox-specific behavior

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Bug

    The variable i is incremented twice in the loop - once in the for loop increment and once manually inside the loop body. This could lead to skipping elements or incorrect indexing.

    for (int i = 0; i < configsCount; i++) {
      String imageName = allConfigs.get(i);
      i++;
      if (i == configsCount) {
    Potential Bug

    The variable i is incremented twice in the loop - once in the for loop increment and once manually inside the loop body. This could lead to skipping elements or incorrect indexing.

    for (int i = 0; i < configsCount; i++) {
      int maxSessions;
      try {
        maxSessions = Integer.parseInt(extractConfiguredValue(allConfigs.get(i)));
      } catch (NumberFormatException e) {
        throw new ConfigException("Unable parse value as number. " + allConfigs.get(i));
      }
      i++;

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent array index bounds exception

    The pre-increment of i before accessing args[i] can lead to an
    ArrayIndexOutOfBoundsException if the last argument is a flag without a
    corresponding value. Use a bounds check before accessing the next argument.

    java/src/dev/selenium/tools/javadoc/JavadocJarMaker.java [61]

    -String next = args[++i];
    +if (i + 1 < argCount) {
    +  String next = args[++i];
    +} else {
    +  throw new IllegalArgumentException("Missing value for flag: " + flag);
    +}
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: Valid concern about potential ArrayIndexOutOfBoundsException when args[++i] is called if the last argument is a flag without a corresponding value. This addresses a real runtime error risk.

    Medium
    Fix loop iteration pattern

    The loop manually increments i inside the loop body, which can lead to skipping
    elements or going out of bounds. Consider using a step of 2 in the loop
    condition to process pairs of elements safely.

    java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java [143-149]

     int configsCount = allConfigs.size();
    -for (int i = 0; i < configsCount; i++) {
    +for (int i = 0; i < configsCount - 1; i += 2) {
       String imageName = allConfigs.get(i);
    -  i++;
    -  if (i == configsCount) {
    -    throw new DockerException("Unable to find JSON config");
    -  }
    +  Capabilities stereotype =
    +      options.enhanceStereotype(JSON.toType(allConfigs.get(i + 1), Capabilities.class));
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: Identifies a valid issue with the manual increment pattern in the loop, but the improved_code doesn't preserve the original error handling logic and significantly restructures the code without maintaining equivalent functionality.

    Low
    General
    Handle null array elements

    Initializing the ArrayList with a capacity is good, but if the array contains
    null elements, they will be added to the list. Consider adding a null check in
    the loop to handle potential null elements properly.

    java/src/org/openqa/selenium/remote/WebElementToJsonConverter.java [87-88]

     int arrayLength = Array.getLength(array);
     List<Object> list = new ArrayList<>(arrayLength);
    +for (int i = 0; i < arrayLength; i++) {
    +  Object item = Array.get(array, i);
    +  if (item != null) {
    +    list.add(item);
    +  }
    +}
    +return list;
    • Apply / Chat
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion changes behavior by filtering out null elements, which may not be intended. The original code was designed to include null values from arrays, and this behavioral change isn't necessarily an improvement.

    Low
    • More

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

    Labels

    B-grid Everything grid and server related B-support Issue or PR related to support classes C-java Java Bindings Review effort 2/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants