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.

Sign up for GitHub

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

Jump to bottom

Refactored Design and Implementation Smells #147

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Refactored Design and Implementation Smells #147

wants to merge 8 commits into from

Conversation

Kushaljp
Copy link

@Kushaljp Kushaljp commented Apr 1, 2024 •
edited by qodo-merge-pro bot
Loading

User description

I have refactored Design Smells and Implementation Smells to improve code readability and code quality

Type

enhancement, bug_fix

Description

  • Introduced ProxyConfigurationManager to manage proxy configurations, enhancing code modularity and readability.
  • Refactored HtmlUnitDriver to use a WebClient instance directly, improving the design.
  • Simplified keyboard interaction logic in HtmlUnitKeyboard and HtmlUnitWebElement, removing unused code and enhancing readability.
  • Added constants for alert lock handling in HtmlUnitTargetLocator for better maintainability.
  • Enhanced modifier keys state management in KeyboardModifiersState.
  • Updated proxy tests to reflect the new proxy management approach.

Changes walkthrough

Relevant files
Enhancement
HtmlUnitDriver.java
Refactor WebClient and Proxy Configuration Management       

src/main/java/org/openqa/selenium/htmlunit/HtmlUnitDriver.java

  • Introduced WebClient and ProxyConfigurationManager instances to manage
    web client and proxy configurations.
  • Removed the setProxySettings method and related proxy configuration
    methods.
  • Proxy settings now managed by ProxyConfigurationManager.
    		•  +8/-136 
    HtmlUnitKeyboard.java
    Simplify Keyboard Interaction and Remove Unused Code         

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitKeyboard.java

  • Simplified sendKeys method logic for HtmlFileInput and general
    elements.
  • Removed commented-out addToKeyboard method, replaced its functionality
    with modifiersState_.addToKeyboard.
    		•  +24/-33 
    HtmlUnitTargetLocator.java
    Improve Alert Lock Handling with Constants                             

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitTargetLocator.java

  • Introduced constants for alert lock retry count and delay.
  • Simplified alert lock retry logic using constants.
    		•  +5/-4     
    HtmlUnitWebElement.java
    Clean Up WebElement Focus and Attribute Handling                 

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitWebElement.java

  • Added shouldSwitchFocus method to clean up focus switching logic.
  • Simplified getAttribute method for HtmlInput elements.
    		•  +15/-12 
    KeyboardModifiersState.java
    Enhance Modifier Keys State Management                                     

    src/main/java/org/openqa/selenium/htmlunit/KeyboardModifiersState.java

  • Added addToKeyboard method to manage keyboard interactions with
    modifier keys.
    		•  +16/-1   
    ProxyConfigurationManager.java
    Introduce ProxyConfigurationManager for Proxy Settings     

    src/main/java/org/openqa/selenium/htmlunit/ProxyConfigurationManager.java

  • Introduced ProxyConfigurationManager class to encapsulate proxy
    configuration logic.
  • Methods for setting HTTP, SOCKS, and PAC proxy configurations.
    		•  +117/-0 
    Tests
    HtmlUnitProxyTest.java
    Update Proxy Tests to Use ProxyConfigurationManager           

    src/test/java/org/openqa/selenium/htmlunit/HtmlUnitProxyTest.java

    • Updated proxy tests to use ProxyConfigurationManager.
    		 +31/-10 

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

    @CLAassistant
    Copy link

    CLA assistant check
    Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
    You have signed the CLA already but the status is still pending? Let us recheck it.

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Apr 1, 2024

    PR Description updated to latest commit (970fc60)

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Apr 1, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and significant refactoring, including the introduction of new classes and methods, modifications to existing logic, and removal of deprecated code. The changes are well-structured and seem to follow good coding practices, but due to the extent of modifications across different components, a thorough review is required to ensure functionality and integration are preserved.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Regression: The removal of proxy setting methods from HtmlUnitDriver and delegation to ProxyConfigurationManager could potentially introduce regressions if not all use cases are covered by the new implementation.

    Code Duplication: The ProxyConfigurationManager class introduces methods (setHTTPProxy, setSocksProxy, etc.) that are very similar to each other and could benefit from further refactoring to reduce code duplication.

    Error Handling: The new proxy configuration management does not seem to include explicit error handling for invalid proxy configurations. It would be beneficial to add validation and error handling to prevent runtime issues.

    🔒 Security concerns

    No
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Apr 1, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Initialize ProxyConfigurationManager in the constructor for better design.

    It's recommended to initialize the ProxyConfigurationManager in the constructor of
    HtmlUnitDriver or through dependency injection, rather than instantiating it directly in
    the method where it's used. This approach enhances the design for better testability and
    adherence to the single responsibility principle.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitDriver.java [171]

    -private ProxyConfigurationManager proxyConfigurationManager=new ProxyConfigurationManager();
+// Assuming there's a constructor or an initialization block
+this.proxyConfigurationManager = new ProxyConfigurationManager();
    Add exception handling for StaleElementReferenceException in shouldSwitchFocus method.

    Consider handling StaleElementReferenceException in shouldSwitchFocus method to maintain
    the previous behavior where exceptions were caught and handled.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitWebElement.java [258-263]

     private boolean shouldSwitchFocus(HtmlUnitWebElement oldActiveElement) {
-    final boolean jsEnabled = driver_.isJavascriptEnabled();
-    final boolean oldActiveEqualsCurrent = oldActiveElement.equals(this);
-    final boolean isOldElementBody = "body".equalsIgnoreCase(oldActiveElement.getTagName());
+    try {
+        final boolean jsEnabled = driver_.isJavascriptEnabled();
+        final boolean oldActiveEqualsCurrent = oldActiveElement.equals(this);
+        final boolean isOldElementBody = "body".equalsIgnoreCase(oldActiveElement.getTagName());
 
-    return jsEnabled && !oldActiveEqualsCurrent && !isOldElementBody;
+        return jsEnabled && !oldActiveEqualsCurrent && !isOldElementBody;
+    } catch (final StaleElementReferenceException ex) {
+        // old element has gone, do nothing
+        return false;
+    }
 }
    Add a break statement to exit the loop once the alert is locked in alert method.

    Consider adding a break statement inside the if condition after Thread.sleep to exit the
    loop once the alert is locked, to avoid unnecessary iterations.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitTargetLocator.java [194-201]

     for (int i = 0; i < ALERT_LOCK_RETRY_COUNT; i++) {
     if (!alert.isLocked()) {
         try {
             Thread.sleep(ALERT_LOCK_RETRY_DELAY_MS);
         } catch (final InterruptedException e) {
             throw new RuntimeException(e);
         }
+    } else {
+        break; // Exit the loop once the alert is locked
     }
 }
    Best practice
    Encapsulate the logic of applying proxy settings in a method.

    Instead of directly setting the proxy settings on proxyConfigurationManager in the
    HtmlUnitDriver class, consider using a method that encapsulates the logic of applying
    proxy settings to the WebClient. This approach adheres to encapsulation and separation of
    concerns principles.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitDriver.java [264]

    -proxyConfigurationManager.setProxySettings(proxy);
+// In HtmlUnitDriver class
+applyProxySettings(proxy);
+// This method then uses proxyConfigurationManager internally
    Add input validation to proxy configuration methods to enhance robustness.

    The ProxyConfigurationManager class should validate the input parameters for methods like
    setHTTPProxy, setSocksProxy, and others to ensure they are not null or invalid, which
    could lead to runtime errors. Adding input validation enhances the robustness of the code.

    src/main/java/org/openqa/selenium/htmlunit/ProxyConfigurationManager.java [90-93]

     public void setHTTPProxy(final String host, final int port, final List<String> noProxyHosts) {
+    if (host == null || host.trim().isEmpty()) throw new IllegalArgumentException("Host cannot be null or empty");
+    if (port < 0) throw new IllegalArgumentException("Port cannot be negative");
     this.proxyConfig.setProxyHost(host);
     this.proxyConfig.setProxyPort(port);
-    noProxyHosts.forEach(this.proxyConfig::addHostsToProxyBypass);
+    if (noProxyHosts != null) {
+        noProxyHosts.forEach(this.proxyConfig::addHostsToProxyBypass);
+    }
 }
    Use a more specific exception for handling InterruptedException in alert method.

    Replace the direct instantiation of RuntimeException with a more specific exception or
    create a custom exception class to provide more context about the error.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitTargetLocator.java [197-199]

     try {
     Thread.sleep(ALERT_LOCK_RETRY_DELAY_MS);
 } catch (final InterruptedException e) {
-    throw new RuntimeException(e);
+    throw new AlertLockInterruptedException("Interrupted while waiting for alert lock", e);
 }
 
+// Elsewhere in the class or package
+public class AlertLockInterruptedException extends RuntimeException {
+    public AlertLockInterruptedException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}
+
    Maintainability
    Refactor setHTTPProxy and setSocksProxy to avoid code duplication.

    The setHTTPProxy and setSocksProxy methods in ProxyConfigurationManager have overlapping
    functionality. Consider refactoring these methods to avoid code duplication, possibly by
    extracting the common logic into a private method.

    src/main/java/org/openqa/selenium/htmlunit/ProxyConfigurationManager.java [90-98]

    -public void setHTTPProxy(final String host, final int port, final List<String> noProxyHosts) {
+private void configureProxy(final String host, final int port, final List<String> noProxyHosts, boolean isSocks) {
     this.proxyConfig.setProxyHost(host);
     this.proxyConfig.setProxyPort(port);
+    if (isSocks) {
+        this.proxyConfig.setSocksProxy(true);
+    }
     noProxyHosts.forEach(this.proxyConfig::addHostsToProxyBypass);
 }
-public void setSocksProxy(final String host, final int port, final List<String> noProxyHosts) {
-    this.proxyConfig.setSocksProxy(true);
-    setHTTPProxy(host, port, noProxyHosts);
-}
    Remove commented-out code for better maintainability.

    The method sendKeys in HtmlUnitKeyboard class has commented-out code that seems to be an
    old implementation. It's a good practice to remove such commented-out code to keep the
    codebase clean and maintainable.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitKeyboard.java [106-120]

    -//    private void addToKeyboard(final Keyboard keyboard, final char ch, final boolean isPress) {
-//        if (HtmlUnitKeyboardMapping.isSpecialKey(ch)) {
-//            final int keyCode = HtmlUnitKeyboardMapping.getKeysMapping(ch);
-//            if (isPress) {
-//                keyboard.press(keyCode);
-//                modifiersState_.storeKeyDown(ch);
-//            }
-//            else {
-//                keyboard.release(keyCode);
-//                modifiersState_.storeKeyUp(ch);
-//            }
-//        }
-//        else {
-//            keyboard.type(ch);
-//        }
-//    }
+// Code block removed for clarity and maintainability.
    Refactor redundant condition checks into a separate method in getAttribute.

    Refactor the getAttribute method to reduce redundancy by extracting the common condition
    check into a separate method.

    src/main/java/org/openqa/selenium/htmlunit/HtmlUnitWebElement.java [285-290]

    -final boolean isHtmlInput = element_ instanceof HtmlInput;
-final boolean isSelectableAttribute = "selected".equals(lowerName) || "checked".equals(lowerName);
-
-if (isHtmlInput && isSelectableAttribute) {
+if (isSelectableInputAttribute(lowerName)) {
     HtmlInput htmlInput = (HtmlInput) element_;
     return trueOrNull(htmlInput.isChecked());
 }
 
+// Elsewhere in the class
+private boolean isSelectableInputAttribute(String attributeName) {
+    final boolean isHtmlInput = element_ instanceof HtmlInput;
+    final boolean isSelectableAttribute = "selected".equals(attributeName) || "checked".equals(attributeName);
+    return isHtmlInput && isSelectableAttribute;
+}
+
    Possible issue
    Add null check for keyboard parameter in addToKeyboard method.

    Ensure that addToKeyboard method handles the case where keyboard is null to prevent
    NullPointerException.

    src/main/java/org/openqa/selenium/htmlunit/KeyboardModifiersState.java [64-76]

     public void addToKeyboard(final Keyboard keyboard, final char ch, final boolean isPress) {
+    if (keyboard == null) {
+        throw new IllegalArgumentException("Keyboard cannot be null");
+    }
     if (HtmlUnitKeyboardMapping.isSpecialKey(ch)) {
         final int keyCode = HtmlUnitKeyboardMapping.getKeysMapping(ch);
         if (isPress) {
             keyboard.press(keyCode);
             storeKeyDown(ch);
         } else {
             keyboard.release(keyCode);
             storeKeyUp(ch);
         }
     } else {
         keyboard.type(ch);
     }
 }
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    bug_fix enhancement Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @Kushaljp @CLAassistant