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

Refactor/cli protocol refactor changes #10009

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

Conversation

matheusbus
Copy link

Pull Request - Improvements to CLI Protocol in PlainCLIProtocol.java

Summary of the Change

This pull request refactors the PlainCLIProtocol class to improve the structure and readability of the code.

Code Smells Identified:

  • Complexity: The run() method was too complex with multiple responsibilities.
  • Duplicated Code: Several validation operations were repeated in different parts of the code.
  • Magic Numbers: Numeric literals used without explanation, which decreased code clarity.
  • Switch Statements: The use of switch statements for operation validation made the code difficult to extend and modify.

Refactoring Details:

  • Refactoring 1: Simplify FramedReader:

    • Changes: The run() method was split into smaller methods: readFrameLength, processFrame, skipUnreadBytes, and handleException.
    • Benefit: Improves readability and maintainability by reducing method complexity.
  • Refactoring 2: Centralize Frame Validation:

    • Changes: A new method, validateFrameLength, was created to centralize frame validation logic.
    • Benefit: Reduces code duplication, improves clarity, and enables the reuse of validation logic.
  • Refactoring 3: Add Direct Validation in Op:

    • Changes: Validation for whether an operation is allowed was previously implicit in the handle method. It has now been moved to the Op enum as a direct method.
    • Benefit: Clarifies the validation process and makes the logic more explicit.
  • Refactoring 4: Eliminate switch for Operation Validation:

    • Changes: The switch statements used for validating operations were replaced with polymorphism, moving the logic into the Op enum.
    • Benefit: Makes the code easier to understand, extend, and modify without the risk of introducing errors when adding new operations.

Refactoring Methods Used:

  • Simplify FramedReader: Extract Method, Replace Nested Conditional with Guard Clauses, Long Method - Duplicated Code
  • Centralize Frame Validation: Extract Method, Replace Magic Number with Symbolic Constant - Duplicated Code, Magic Numbers
  • Validation in Op: Move Method - Duplicated Code
  • Eliminate switch for Operation Validation: Replace Conditional with Polymorphism - Large Conditional (Switch Statements)

Testing Done

  • Manual Testing:

    • The refactor has been manually tested to ensure that the logic remains intact. The functionality of sending and receiving data between the client and server, as well as frame validation, was verified.
    • The changes were tested for both typical and edge cases to confirm that the refactoring did not introduce regressions.
  • Automated Testing:

    • No new automated tests were introduced. However, the existing tests were run to verify that no existing functionality was broken.

Proposed Changelog Entries

  • Refactored the PlainCLIProtocol class to improve readability and maintainability.
  • Simplified the run() method in FramedReader by breaking it into smaller, focused methods.
  • Centralized frame validation logic to reduce duplication and improve clarity.
  • Replaced switch statements with polymorphism for operation validation, making the code easier to extend.

Proposed Upgrade Guidelines

N/A


Submitter Checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate and written in the imperative mood.
  • The change has been manually tested, and the behavior has been verified.
  • No new public classes or methods require additional annotations.
  • No new deprecations have been introduced.
  • New or changed JavaScript does not use inline definitions or eval.
  • For dependency updates, relevant changelogs have been linked.
  • For new APIs, at least one consumer is referenced.

Desired Reviewers

  • @jenkinsci/core-pr-reviewers

Maintainer Checklist

  • There are at least two approvals for the pull request.
  • All conversations in the pull request are resolved.
  • The changelog entries are accurate and in the imperative mood.
  • Proper changelog labels have been set.
  • If the change needs additional upgrade steps, the upgrade-guide-needed label is set.
  • If this change should be backported to LTS, a Jira issue has been created with the lts-candidate label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant