Refactor: Full architectural rework

[bootuz]

Summary

• Merge calculators — combined StabilityCalculator + DifficultyCalculator into unified MemoryStateCalculator, moved to Algorithm/
• Replace scheduler inheritance with composition — deleted BaseScheduler/BasicScheduler class hierarchy, replaced with SchedulingPipeline struct + ShortTermScheduler/LongTermScheduler structs composed via pipeline
• Rename public API — repeat → schedule(card:at:), next → schedule(card:at:rating:), getRetrievability → retrievability(of:at:), PartialFSRSParameters → FSRSConfiguration
• Clean up FSRS facade — extracted reschedule orchestration into RescheduleService, FSRS.swift reduced from 243 to 189 lines
• Remove dead code & restructure — deleted unused ScheduledInterval, Factory.swift; eliminated Core/ directory; organized into Algorithm/, Scheduling/, Models/, etc.
• Add focused unit tests — 21 new tests for MemoryStateCalculator and SchedulingPipeline (146 total, all passing)

Test plan

• All 146 tests pass (swift test)
• Build succeeds (swift build)
• Behavioral equivalence verified — each step committed separately with full test suite passing
• Verify examples compile against new API names

[claude]

PR Review: Full Architectural Rework

Overall this is a solid refactor. The shift from inheritance (BaseScheduler hierarchy) to composition (SchedulingPipeline) is the right direction for Swift, and the API renames eliminate the repeat keyword collision cleanly. Below are observations organized by severity.

---

Issues

1. Breaking public API without deprecation annotations

The renamed methods and types (repeat/next/getRetrievability/getRetrievabilityValue/PartialFSRSParameters) have no @available(*, deprecated, renamed:...) annotations. Any existing consumers of the library will get compile errors with no migration guidance. Swift's renaming mechanism is designed exactly for this:

// ParameterManagement.swift
@available(*, deprecated, renamed: "FSRSConfiguration")
public typealias PartialFSRSParameters = FSRSConfiguration

// FSRS.swift
@available(*, deprecated, renamed: "schedule(card:at:)")
public func repeat(card: Card, now: Date) throws -> RecordLog<Card> {
    try schedule(card: card, at: now)
}

This makes the transition non-breaking and compiler-guided. Even for a minor version bump, signalling intent is good practice.

2. Oversized tuple in computeConstrainedReviewIntervals

The 6-element tuple return (suppressed with // swiftlint:disable:this large_tuple) is a code smell — it's a struct waiting to be named:

// Instead of:
func computeConstrainedReviewIntervals(...) throws -> (hard: Int, good: Int, easy: Int, hardState: MemoryState, goodState: MemoryState, easyState: MemoryState)

// Consider:
struct ConstrainedReviewIntervals {
    let hard: Int; let good: Int; let easy: Int
    let hardState: MemoryState; let goodState: MemoryState; let easyState: MemoryState
}

3. SchedulingPipeline exposes too much as public

SchedulingPipeline.logger, memoryStateCalculator, intervalCalculator, elapsedDaysValue, and currentCard are all public. Since ShortTermScheduler and LongTermScheduler live in the same module, internal access is sufficient for all of these. Marking them public unnecessarily grows the library's API surface and locks in implementation details.

4. Redundant assignment in RescheduleService.reschedule

var emptyCard = card
emptyCard.due = card.due   // no-op, already copied from card
emptyCard.stability = 0

The emptyCard.due = card.due line can be removed.

5. Schedulers access pipeline internals directly

LongTermScheduler reaches into pipeline.logger, pipeline.currentCard, pipeline.reviewTime, pipeline.elapsedDaysValue, pipeline.algorithm, pipeline.intervalCalculator throughout its implementation. If SchedulingPipeline is meant to be an encapsulated unit of shared state, the schedulers should consume it through its public methods (computeMemoryState, computeRetrievability, etc.) rather than as a property bag. The current design gives composition the shape of inheritance — just accessing a bag of state with a different name.

---

Minor / Nits

6. formattedRetrievability naming ambiguity

The distinction between formattedRetrievability(of:at:) -> String and retrievability(of:at:) -> Double may not be obvious at call sites. Consider retrievabilityString(of:at:) or at minimum a doc comment on formattedRetrievability calling out that it returns a percentage-formatted string like "85.00%".

7. swiftlint:disable:next function_body_length in ShortTermScheduler

Two functions carry this suppression. The reviewState function in particular has an if rating == .again branch that could be a separate relearningState helper, both reducing the body length and making the branching intent explicit.

8. README "Verify examples compile" is still unchecked

The PR description's own checklist item — [ ] Verify examples compile against new API names — is unresolved before merge.

9. SchedulingPipeline.buildLog captures pre-update state

The pipeline increments currentCard.reps at init, but stability/difficulty are not yet updated when buildLog is called (schedulers update a local nextCard copy). This means the log entry records the pre-review stability/difficulty values. That appears intentional (log what the card was at review time), but deserves a doc comment explaining the deliberate choice, since it's easy to misread as a bug.

---

What's Good

• Composition over inheritance — the SchedulingPipeline approach is genuinely cleaner than the BaseScheduler/BasicScheduler hierarchy for a value-oriented language.
• MemoryStateCalculator unification — merging StabilityCalculator + DifficultyCalculator makes sense since the two are tightly coupled in the FSRS formulas.
• FSRS.swift slim-down — delegating orchestration to RescheduleService is the right call; 243 -> 189 lines shows real extraction rather than cosmetic cleanup.
• Duplicate doc comment removed (FSRS.init had a duplicated - logger: parameter line).
• Focused new tests — MemoryStateCalculatorTests and SchedulingPipelineTests cover the newly extracted types directly rather than only testing through the top-level API.
• Dead code removal — ScheduledInterval, Factory.swift, invalidScheduledInterval error case are all gone without backwards-compat shims.

[claude]

PR Review: Full Architectural Rework

Overall this is a well-motivated refactoring with clear architectural goals. The move from class inheritance to struct composition is idiomatic Swift, and the API renaming resolves real issues (backtick-quoted repeat, Java-style getter names). Here's detailed feedback:

---

Strengths

• Composition over inheritance — Replacing BaseScheduler/BasicScheduler class hierarchy with SchedulingPipeline + ShortTermScheduler/LongTermScheduler structs is the right approach for Swift. Value semantics, no reference cycles, no fatalError template methods.
• Eliminating fatalError template methods — BaseScheduler required subclasses to override scheduleNewCard, scheduleLearningCard, scheduleReviewCard or crash at runtime. Removing this is a meaningful safety improvement.
• MemoryStateCalculator unification — StabilityCalculator + DifficultyCalculator was unnecessary fragmentation. Merging them with nextMemoryState() as the primary entry point is cleaner.
• API naming — schedule(card:at:) and schedule(card:at:rating:) as overloads is clean. retrievability(of:at:) follows Swift's of: convention for primary subjects. FSRSConfiguration is clearly better than PartialFSRSParameters.
• RescheduleService owns orchestration — Extracting the reschedule body from FSRS.swift into RescheduleService is the right separation.
• New focused tests — MemoryStateCalculatorTests and SchedulingPipelineTests test the units directly rather than only through integration. The mathematical property tests (forgetting curve monotonicity, difficulty ordering) are particularly good.

---

Issues

1. Breaking changes with no migration path (major)

This PR makes multiple breaking changes with no deprecation shims:

• PartialFSRSParameters → FSRSConfiguration (type rename)
• repeat(card:now:) → schedule(card:at:)
• next(card:now:rating:) → schedule(card:at:rating:)
• getRetrievability(card:now:) → formattedRetrievability(of:at:)
• getRetrievabilityValue(card:now:) → retrievability(of:at:)
• forget(card:now:) → forget(card:at:)
• reschedule(currentCard:reviews:options:) → reschedule(card:reviews:options:)
• fsrs<Card>(params:) free function removed entirely

Any existing consumer of this library gets compilation failures with no guidance. For a library, the standard approach is to add @available(*, deprecated, renamed: "schedule(card:at:)") annotations first so users get a deprecation warning (not a hard error), then remove in the next major version. Alternatively, a MIGRATION.md covering the before/after for each renamed symbol would help.

2. Large tuple return from computeConstrainedReviewIntervals (moderate)

// swiftlint:disable:this large_tuple
public func computeConstrainedReviewIntervals(...) throws -> (hard: Int, good: Int, easy: Int, hardState: MemoryState, goodState: MemoryState, easyState: MemoryState)

The lint suppression is the code telling you something. A named struct (e.g., ConstrainedReviewResult) would be cleaner and make call sites more readable.

3. MemoryStateCalculator over-exposes internals as public (moderate)

The following methods are marked public but are implementation details that external callers shouldn't need to call individually:

• initStability(for:), initDifficulty(for:)
• nextRecallStability(...), nextForgetStability(...), nextShortTermStability(...)
• nextDifficulty(...)

Only forgettingCurve(elapsedDays:stability:), computeDecayFactor(_:), and nextMemoryState(...) have a clear case for being public. The individual step methods could be internal, with @testable import covering the existing tests.

4. Implicit internal access in SchedulingPipeline (minor)

// These have no explicit access modifier (defaults to internal):
let logger: (any FSRSLogger)?
let memoryStateCalculator: MemoryStateCalculator
let intervalCalculator: IntervalCalculator

While internal is correct here (cross-struct access within the module), the inconsistency with the explicitly public properties above is confusing. Either mark them internal explicitly or add a comment explaining the intent.

5. buildLog(rating:) public on schedulers (minor)

Both ShortTermScheduler and LongTermScheduler expose buildLog(rating:) -> ReviewLog as public. If this isn't part of SchedulerProtocol, it shouldn't be public — it's an implementation detail. If it is part of the protocol, that should be visible in the diff.

6. Docstring regression in MemoryStateCalculator (minor)

StabilityCalculator and DifficultyCalculator had mathematical notation with Unicode subscripts in docstrings:

/// S₀(G) = weights[G-1], S₀ = max{S₀, 0.1}
/// D'(D,R) = weights[7] × D₀(4) + (1 - weights[7]) × next_d

The new MemoryStateCalculator dropped these. They're helpful for anyone verifying the implementation against the FSRS paper.

7. Planning document in source tree (minor)

docs/plans/2026-02-22-refactoring-design.md is an internal design artifact. Committing it creates date-stamped noise in the repo history. Consider either removing it or keeping such documents in a separate branch/wiki.

---

Test Coverage Notes

• The 21 new tests for MemoryStateCalculator and SchedulingPipeline are well-structured.
• testForgettingCurveAtZero correctly validates the boundary condition R(0,S) = 1.0.
• Consider adding a test for nextMemoryState with elapsedDays = 0 and enableShortTerm = true to explicitly cover the short-term path through nextShortTermStability.
• The replayReviews method changed from public to internal — worth confirming this is intentional and any external callers are aware.

---

Summary

The architecture direction is sound. The main thing to address before merging is the breaking API changes — either add deprecation annotations for a graceful migration path, or document the breaking changes explicitly in the PR description/CHANGELOG so library consumers can plan accordingly. The large tuple and public surface area of MemoryStateCalculator are worth cleaning up but are less urgent.