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

refac: Refactor Publish module to Improve code quality by removing implementation smell #2382

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

Conversation

nakul3736
Copy link

@nakul3736 nakul3736 commented Mar 22, 2025

Refactor for Improved Readability and Modularity

Changes

  1. Simplified Complex Conditional in publishOutstandingBatch

    • Path: com.google.cloud.pubsub.v1
    • File: Publisher.java
    • Method: publishOutstandingBatch
    • Problem: The method contained a complex conditional (if (!activeAlarm.get() && outstandingBatch.orderingKey != null && !outstandingBatch.orderingKey.isEmpty())) mixing multiple checks, increasing cognitive load.
    • Change: Broke the conditional into a separate method shouldPublishMoreForKey(boolean isAlarmActive, String orderingKey) to isolate the logic and improve readability.
    • Impact: Reduces cyclomatic complexity and makes the intent clearer.
  2. Extracted acquire Method into Modular Helper Methods

    • Path: com.google.cloud.pubsub.v1
    • File: Publisher.java (nested class MessageFlowController)
    • Method: acquire
    • Problem: The original method was long and handled multiple responsibilities (limit checking, message slot acquisition, byte space allocation, waiting), leading to high complexity and duplication.
    • Change: Refactored into five focused methods:
      • checkMessageLimit(): Validates message count against the limit.
      • checkByteLimit(long messageSize): Validates byte count against the limit.
      • acquireMessageSlot(): Acquires a message slot, waiting if necessary.
      • acquireByteSpace(long messageSize): Acquires byte space, handling partial allocation.
      • waitForResource(CountDownLatch waiter): Centralizes waiting logic with lock management.
    • Impact: Each method now has a single responsibility, reducing complexity, eliminating duplication (e.g., waiting logic), and improving testability.
  3. Renamed Variables for Clarity and Consistency

    • Path: com.google.cloud.pubsub.v1
    • File: Publisher.java
    • Changes:
      • topicNameSizeserializedTopicNameSize: Reflects it’s the serialized size, not string length.
      • messagesBatchesbatchesByOrderingKey: Clarifies it’s a map keyed by ordering keys.
      • activeAlarmisBatchPublishScheduled: Indicates it tracks scheduled batch publishing.
      • messagesWaiterpendingPublishesTracker: Specifies it tracks pending publishes.
      • outstandingMessagesinFlightMessages: Uses standard “in-flight” term for processing items.
      • outstandingBytesinFlightBytes: Consistent with inFlightMessages.
      • batchedBytestotalMessageBytesInBatch: Describes it’s the total byte size in a batch.
    • Impact: Improves readability and reduces ambiguity, making the code self-documenting.
  4. Refactor SchemaServiceGrpc to Eliminate Duplicated Code (Extract Class)

  • Change: Extracted duplicated MethodDescriptor creation logic from SchemaServiceGrpc into a new MethodDescriptorFactory class.
  • Impact:
    • Reduces code duplication across 10 methods (e.g., getCreateSchemaMethod).
    • Adheres to DRY principles.
    • Simplifies future maintenance by centralizing descriptor creation.
  1. Replace Conditionals with Polymorphism in OpenTelemetryPubsubTracer
  • Change: Refactored startSubscribeRpcSpan in OpenTelemetryPubsubTracer to use an RpcOperationHandler interface and concrete handlers (AckHandler, ModAckHandler, NackHandler), eliminating conditional logic (if/switch statements).
  • Impact:
    • Removes Conditional Logic Over Type smell.
    • Makes the method extensible (new operations require only a new handler).
    • Aligns with the Open/Closed Principle.
  1. Move Field/Method in Publisher Class (acquire Method)
  • Change: Moved the field and associated logic used in the acquire method of the Publisher class to a more appropriate class (e.g., a resource manager or semaphore wrapper).
  • Impact:
    • Improves cohesion by relocating resource acquisition logic to a dedicated class.
    • Reduces the Publisher class’s responsibilities.
    • Enhances readability and maintainability.

Testing

  • Verified that the refactored code maintains original behavior by running existing unit tests (assumed present in the codebase).
  • Manually tested publishing scenarios with ordered and unordered messages to ensure flow control and batching work as expected.
  • Code coverage is also not reduced in the mockPublisher class.

@nakul3736 nakul3736 requested a review from a team as a code owner March 22, 2025 12:20
Copy link

google-cla bot commented Mar 22, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: pubsub Issues related to the googleapis/java-pubsub API. labels Mar 22, 2025
@nakul3736 nakul3736 requested a review from a team as a code owner March 28, 2025 06:27
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/java-pubsub API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants