Refactor: Extraction of HTTP Connection Manager in FullDuplexHttpStream#10010
Closed
matheusbus wants to merge 3 commits intojenkinsci:masterfrom
Closed
Refactor: Extraction of HTTP Connection Manager in FullDuplexHttpStream#10010matheusbus wants to merge 3 commits intojenkinsci:masterfrom
matheusbus wants to merge 3 commits intojenkinsci:masterfrom
Conversation
Contributor
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Member
|
I'll go ahead and close this PR, given the change proposed doesn't compile at all and progress has been stale for over 6 months. However, you are very much welcome to submit a new PR, which has been tested locally before submission. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request - Extraction of HTTP Connection Manager in
FullDuplexHttpStream.javaSummary of the Change
This pull request refactors the
FullDuplexHttpStreamclass by extracting the HTTP connection management logic into a separate class. This change improves the maintainability, readability, and testability of the code. Additionally, the class name have been updated to reflect the actual functionality more clearly.Code Smells Identified:
Long Method: The
FullDuplexHttpStreamclass method was overly long, handling both the HTTP connection creation and data transport logic, making it complex and difficult to understand.Single Responsibility Principle Violation: The class was managing both the HTTP connection setup and the data transport, which violated the single responsibility principle.
Duplicated Code: Connection configuration logic (such as establishing the connection, setting request methods, headers, etc.) was repeated for both upload and download streams.
Refactoring Details:
Refactoring 1: Extract HTTP Connection Logic:
HttpConnectionManager. This refactor allowsFullDuplexHttpStreamto focus on managing the data streams, while the connection logic is handled by the new class.FullDuplexHttpStream, promotes better maintainability, and enables easier unit testing of the connection logic.Refactoring 2: Misleading Class Name:
FullDuplexHttpStreamwas misleading, as the class actually handles two separate streams (upload and download), not a simultaneous full-duplex communication.HttpUploadDownloadStreamto better reflect its actual functionality.Refactoring Techniques Applied:
Extract Class: The HTTP connection management logic was moved to a new class (
HttpConnectionManager), which improves the cohesion of theFullDuplexHttpStreamclass.Rename Class: The class was renamed to reflect the true nature of the class (upload/download streams), resolving the misleading name issue.
Testing Done
Manual Testing:
Automated Testing:
Proposed Changelog Entries
HttpConnectionManager.FullDuplexHttpStreamclass toHttpUploadDownloadStreamfor clarity.Proposed Upgrade Guidelines
N/A
Submitter Checklist
eval.Desired Reviewers
Maintainer Checklist
upgrade-guide-neededlabel is set.lts-candidatelabel.