Skip to content

refactor: apply idiomatic-Kotlin simplifications across the SDK#163

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
refactor/idiomatic-kotlin-cleanup
Open

refactor: apply idiomatic-Kotlin simplifications across the SDK#163
OmarAlJarrah wants to merge 1 commit into
mainfrom
refactor/idiomatic-kotlin-cleanup

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

A pass of idiomatic-Kotlin readability cleanups across sdk-core and the adapter
modules, replacing a number of "Java-in-Kotlin" patterns with their idiomatic
equivalents. These are maintainability refactors only — no behavior changes and
no new features.

Every change is body-only or adds a private member, so there are no public
signature changes
: the committed .api snapshots are untouched and apiCheck
passes without an apiDump. The full gated build is green — tests, detekt,
ktlint, binary-compatibility validation, the Kover line-coverage floor, and the
R8 shrink-survival guard all pass.

What changed

Collection operators replace hand-rolled loops and accumulators

  • HttpPipelineBuilder / AsyncHttpPipelineBuilder: arrayOfNulls<…> + index-fill
    • an unchecked cast collapse to Array(ordered.size) { ordered[it] }, dropping
      two @Suppress("UNCHECKED_CAST") and an obsolete KDoc note that explained the old
      workaround.
  • DigestChallengeHandler.pickChallenge: a five-continue accumulation loop becomes
    challenges.mapNotNull(::toCandidate). Per-challenge validation moves into an
    extracted toCandidate() that keeps one early return per gate (scheme, realm,
    nonce, qop, algorithm) — the existing design preference for distinct gates over a
    composite predicate is preserved and now documented on the helper.
  • DefaultRedirectStep (filter/forEach), UrlRedactor (mapTo),
    ResponsePipeline (fold), Annotations (filterIsInstance),
    LinkHeaderPaginationStrategy (asSequence/mapNotNull/firstOrNull),
    TristateModule (forEachIndexed).

Control flow and casts

  • RetryStep: when (val readyState = prepareNextAttempt(...)) subject form;
    data object Proceed.
  • JdkHttpTransport: exhaustive when over the two-constant HttpVersion enum with
    no else, so adding a third constant later fails to compile rather than silently
    falling through.
  • WriteAllInto: subject when (read).
  • RequestAdapter: capture request.body into a local so it smart-casts, removing
    the cross-module !!.
  • AuthChallengeParser: the long || punctuation chains in isTokenChar /
    isToken68Char become membership checks against named private val Set<Char>.
  • PageNumberPaginationStrategy: try/catch (NumberFormatException) plus a
    null/empty pre-check becomes raw?.toIntOrNull() ?: fallback.
  • LoggableResponseBody.contentLength: a nested if becomes a single Elvis fold.

Boilerplate de-duplicated via private helpers

  • MdcAwareExecutor: the capture-then-wrap pair that was copy-pasted across seven
    ExecutorService overrides is now two private MdcSnapshot.wrap helpers. The
    snapshot is still captured at the call site (submit time), so MDC propagation
    semantics are unchanged.
  • SpanLoggingExtensions: the per-key MDC restore idiom becomes restoreMdc(key, previous).
  • TeeSink: the tap-budget clamp arithmetic, previously written three times, becomes
    a single tapAllowance(requested); the actual writes and budget advancement stay
    inline at each site.
  • Io.installProvider: the hand-thrown IllegalStateException for a conflicting
    provider becomes check(...) with a lazily built message (same thrown type, same
    text; re-installing the same instance is still a no-op).

Idiom polish

  • MediaType.formatParameterValue: manual StringBuilderbuildString (capacity
    hint retained).
  • DispatchContext: string concatenation → templates (the call-key counter is still
    incremented exactly once).
  • Configuration.parseDuration: Character.toUpperCase(...)Char.uppercaseChar().
  • RequestRebuilder: drop a @Suppress("UNUSED_VARIABLE") by naming the caught
    exception catch (ignored: …).
  • ThrowOnHttpErrorStep: explicit return type on an anonymous mediaType() override.
  • ForeignSinkAdapter / ForeignSourceAdapter: return Timeout.NONE inline instead
    of caching it in a per-instance field.
  • RestrictedHeaders (okhttp + jdkhttp): drop the no-op .lowercase(Locale.US) on
    string literals that are already lower-case (the Locale import is kept where it is
    still used for runtime normalization).

Interop & compatibility

sdk-core is consumed from Java, so nothing here touches a publicly visible signature,
default arguments, or annotations. No value/inline classes were introduced on public
types, no data class copy()/componentN() was exposed, and no overload was collapsed
in a way that would box at the Java call site. apiCheck confirms the .api snapshots are
unchanged.

Considered but intentionally not included

  • RetryStep explicit Failure casts (in invoke and runRetryLoop): Kotlin does
    not smart-cast outcome to ResponseOutcome.Failure after an early return on the
    Success branch — excluding one sealed subtype does not narrow to the sibling — so the
    explicit as ResponseOutcome.Failure casts are required and were left in place.
  • ThrowOnHttpErrorStep.readUpTo "Buffer-native bounded read": BufferedSource has no
    primitive for "read up to N bytes, returning however many are available" without an
    EOFException on a short read, so the existing bounded InputStream loop is kept to
    avoid any behavior change.

Validation

./gradlew build passes end to end: unit tests, detekt, ktlint, apiCheck, the
aggregate Kover coverage floor, and the sdk-shrink-test R8 shrink-survival guard.

Readability cleanups across sdk-core and the adapter modules, replacing
"Java-in-Kotlin" patterns with their idiomatic equivalents. Every change is
body-only or adds a private member: no public signature changes (apiCheck
unchanged), and the full gated build — tests, detekt, ktlint, binary
compatibility, the Kover floor, and the R8 shrink guard — passes.

Collection operators replace hand-rolled loops and accumulators:
- HttpPipelineBuilder / AsyncHttpPipelineBuilder: arrayOfNulls + index-fill +
  unchecked cast -> Array(size) { ordered[it] }, dropping two
  @Suppress("UNCHECKED_CAST") and an obsolete KDoc workaround note.
- DigestChallengeHandler.pickChallenge: a five-continue accumulation loop ->
  challenges.mapNotNull(::toCandidate), with the per-challenge validation moved
  into an extracted toCandidate() that keeps one early return per gate.
- DefaultRedirectStep (filter/forEach), UrlRedactor (mapTo), ResponsePipeline
  (fold), Annotations (filterIsInstance), LinkHeaderPaginationStrategy
  (sequence/mapNotNull/firstOrNull), TristateModule (forEachIndexed).

Control flow and casts:
- RetryStep: when (val readyState = prepareNextAttempt(...)) subject form;
  data object Proceed.
- JdkHttpTransport: exhaustive when over the two-constant HttpVersion enum
  (no else, so a future constant fails to compile).
- WriteAllInto: subject when (read).
- RequestAdapter: capture request.body into a local so it smart-casts, dropping
  the cross-module !!.
- AuthChallengeParser: long || punctuation chains -> private Set<Char> membership.
- PageNumberPaginationStrategy: try/catch(NumberFormatException) ->
  raw?.toIntOrNull() ?: fallback.
- LoggableResponseBody.contentLength: nested if -> a single Elvis fold.

Boilerplate de-duplicated via private helpers:
- MdcAwareExecutor: the capture/wrap pair repeated across seven overrides ->
  two MdcSnapshot.wrap helpers (capture stays at the call site).
- SpanLoggingExtensions: per-key MDC restore -> restoreMdc().
- TeeSink: the tap-budget clamp arithmetic -> tapAllowance().
- Io.installProvider: hand-thrown IllegalStateException -> check(...) with a
  lazily built message.

Idiom polish:
- MediaType: manual StringBuilder -> buildString (capacity hint kept).
- DispatchContext: string concatenation -> templates.
- Configuration.parseDuration: Character.toUpperCase -> Char.uppercaseChar().
- RequestRebuilder: drop @Suppress("UNUSED_VARIABLE") via catch (ignored: ...).
- ThrowOnHttpErrorStep: explicit return type on an anonymous mediaType() override.
- ForeignSinkAdapter / ForeignSourceAdapter: return Timeout.NONE inline.
- RestrictedHeaders (okhttp + jdkhttp): drop the no-op .lowercase() on
  already-lower-case literals.
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