Refactor: Code organization and bounds-checked range combinators

[arcaputo3]

Summary

This PR improves code organization by splitting monolithic files into focused modules and adds robust bounds checking to range combinators.

Code Organization

xl-macros Module

• Deleted: macros.scala (298 lines monolithic file)
• Created:
  • CellRangeLiterals.scala - cell/range/fx compile-time literals
  • BatchPutMacro.scala - batch put macro for elegant syntax
  • FormattedLiterals.scala - money/percent/date/accounting literals

xl-core Module

• Split: sheet.scala (reduced from 450 → 328 lines)
• Created:
  • SheetProperties.scala (18 lines) - ColumnProperties, RowProperties
  • SheetExtensions.scala (217 lines) - All Sheet extension methods
  • workbook.scala (129 lines) - Workbook and WorkbookMetadata (previous commit)

xl-core/style Package (Previous Commit)

• Split: style.scala (440 lines) into 10 focused files:
  • units.scala, color.scala, numfmt.scala, font.scala, fill.scala
  • border.scala, alignment.scala, cellstyle.scala, patch.scala, registry.scala

Functional Improvements

Bounds Checking

• Added Column.MaxIndex0 (16383) and Row.MaxIndex0 (1048575) constants
• Enhanced range combinators with validation:
  • putRange - validates value count matches range size
  • putRow - enforces Excel column limits (A-XFD)
  • putCol - enforces Excel row limits (1-1,048,576)

Error Handling

• Added XLError.ValueCountMismatch for clearer error messages
• All three methods now return XLResult[Sheet] instead of Sheet

Breaking Changes

⚠️ Range combinators now return XLResult[Sheet]:

• Sheet.putRange(range, values): XLResult[Sheet]
• Sheet.putRow(row, startCol, values): XLResult[Sheet]
• Sheet.putCol(col, startRow, values): XLResult[Sheet]

Callers must handle the result explicitly with .getOrElse(), pattern matching, or for-comprehensions.

Testing

• ✅ All 267 tests passing
• ✅ Added RangeCombinatorsSpec.scala with 4 new tests for error handling
• ✅ Code formatted and validated

Impact

File Size Reductions:

• style.scala: 440 → 82 lines max (81% reduction)
• sheet.scala: 450 → 328 lines (27% reduction)
• macros.scala: 298 → 147 lines max (51% reduction)

Benefits:

• Easier navigation with focused, single-purpose files
• Better error messages for out-of-bounds operations
• Prevents silent data loss when writing beyond Excel limits

🤖 Generated with Claude Code

[claude]

Pull Request Review: Code Organization and Bounds-Checked Range Combinators

Overall Assessment: Excellent ✅

This is a well-executed refactoring that significantly improves code organization and adds critical safety features. The changes align perfectly with the project's purity charter and demonstrate strong attention to detail.

---

Strengths

1. Code Organization 🎯

The file splitting is thoughtful and follows clear separation of concerns:

• xl-macros: Split from monolithic 298 lines into 3 focused files (CellRangeLiterals, BatchPutMacro, FormattedLiterals)
• xl-core: Extracted SheetExtensions (217 lines) and SheetProperties (18 lines) from sheet.scala
• style package: Previous split into 10 focused files is excellent

Impact: File sizes reduced by 27-81%, making navigation and maintenance significantly easier.

2. Safety Improvements 🛡️

The bounds checking additions are critical for production use:

• Added Column.MaxIndex0 = 16383 and Row.MaxIndex0 = 1048575 constants
• putRange, putRow, putCol now return XLResult[Sheet] with proper error handling
• New XLError.ValueCountMismatch provides clear error messages

Benefits: Prevents silent data loss when writing beyond Excel's limits (XFD column, row 1,048,576).

3. Test Coverage ✅

Excellent test additions in RangeCombinatorsSpec.scala:

• Tests for successful operations and error conditions
• Verifies exact error types and messages
• Tests boundary conditions (MaxIndex0)

267 tests passing demonstrates stability.

4. Documentation 📚

• CHANGELOG.md clearly documents breaking changes
• README.md updated with error handling examples
• Comprehensive STATUS.md updates showing progress
• Documentation reorganization (archive/, design/, reference/) is very clean

---

Issues & Recommendations

Minor: Bounds Checking Clarity 💭

Location: xl-core/src/com/tjclp/xl/SheetExtensions.scala:128,161

Current Implementation: The bounds check uses toLong arithmetic which works correctly but could be more explicit about what's being validated.

Suggestion: Consider making the validation logic clearer by computing size > maxAllowed directly rather than computing lastIndex and comparing.

Impact: Low - current code is functionally correct, this just improves clarity.

Minor: Documentation Enhancement 📝

Location: xl-core/src/com/tjclp/xl/SheetExtensions.scala:95-98

Suggestion: Add rationale for strict size matching:

/**
 * Put cells in a range in row-major order.
 *
 * The number of supplied values must exactly match the range size to prevent
 * silent data loss from truncation or unintended empty cells from padding.
 */

Minor: Consistent Error Message Format 📐

Observation: Error messages use slightly different styles:

• Line 134: mentions "max X columns"
• Line 166: mentions "Excel row limit"

Suggestion: Use consistent format for both (either mention limits for both or neither).

---

Security Considerations 🔒

Good: Bounds checking prevents buffer overflows and out-of-memory issues from malicious/malformed inputs.

Consider: Add fuzz testing for edge cases:

• Empty value sequences
• Single-cell ranges
• Maximum size ranges (A1:XFD1048576)

---

Performance Considerations ⚡

Excellent:

• Opaque types maintain zero-overhead abstractions
• Bounds checks are O(1)
• Early validation prevents unnecessary allocations

Note: putRange materializes the values collection to validate size, which is appropriate for correctness.

---

Test Coverage Assessment 🧪

Excellent coverage in RangeCombinatorsSpec.scala:

• ✅ Success cases for all three methods
• ✅ Error cases for value count mismatch
• ✅ Boundary overflow tests

Missing tests (nice-to-have):

• Edge case: Empty value sequences
• Edge case: Single-cell ranges
• Property test: Round-trip with fillBy

---

Breaking Changes Analysis 📋

Changes: putRange, putRow, putCol return type changed from Sheet to XLResult[Sheet]

Migration path: Well-documented in CHANGELOG and README with clear examples.

---

Code Quality 💎

Excellent:

• ✅ Consistent with purity charter (total functions, explicit errors)
• ✅ Well-documented with ScalaDoc
• ✅ Uses @TargetNAME to prevent erasure conflicts
• ✅ Proper imports and package structure
• ✅ Formatted per project style guide
• ✅ Zero-overhead opaque types maintained

---

Final Recommendation

Approve ✅

This PR represents high-quality work that significantly improves the codebase. All suggestions above are minor improvements for clarity and consistency.

Action Items (All Optional):

1. Clarify bounds checking logic in putRow/putCol
2. Add rationale to putRange documentation
3. Consistent error message format
4. Add property-based tests for edge cases

Key Wins:

• 27-81% reduction in file sizes through smart modularization
• Critical safety features prevent data loss at Excel boundaries
• Comprehensive test coverage with 267 tests passing
• Excellent documentation updates

---

Reviewed by: Claude Code
Date: 2025-11-10

🤖 Generated with Claude Code

[claude]

PR Review: Code Organization and Bounds-Checked Range Combinators

Thank you for this well-structured refactoring PR! Overall, this is excellent work that significantly improves code maintainability and adds critical safety features. The changes align well with the project's philosophy of purity, totality, and safety.

---

✅ Strengths

1. Excellent Code Organization

The file splits are well-motivated and follow single-responsibility principle:

• xl-macros: Breaking the 298-line monolithic macros.scala into CellRangeLiterals.scala, BatchPutMacro.scala, and FormattedLiterals.scala makes each macro feature independently maintainable
• xl-core: Extracting SheetExtensions.scala (230 lines) from sheet.scala separates core data structures from behavior
• style package: Splitting the 440-line style.scala into 10 focused files is a huge win for navigability

2. Critical Safety Improvements

Adding bounds checking to range combinators addresses a real safety gap:

• MaxIndex0 constants (Column: 16383, Row: 1048575) now properly enforce Excel limits
• putRange validating value count prevents silent data loss from truncation
• putRow/putCol returning XLResult[Sheet] makes bounds violations explicit

3. Appropriate Breaking Changes

The return type changes (Sheet → XLResult[Sheet]) are justified:

• Forces callers to handle errors explicitly (matches project's totality principle)
• Clear upgrade path documented in PR description
• New ValueCountMismatch error provides actionable diagnostics

4. Strong Test Coverage

RangeCombinatorsSpec.scala with 4 new tests covering:

• Happy path (value count matches)
• Error cases (mismatched counts, out-of-bounds)
• All 267 tests passing demonstrates backward compatibility

---

🔍 Observations & Minor Issues

1. Potential Edge Case in putRow/putCol (xl-core/src/com/tjclp/xl/SheetExtensions.scala:134-187)

The bounds check logic uses MaxIndex0 - startCol.index0 + 1, which is correct. However, consider this edge case:

// Line 140: maxAllowedColumns calculation
val maxAllowedColumns = Column.MaxIndex0 - startCol.index0 + 1

Issue: If startCol.index0 is already at MaxIndex0, this allows writing 1 value (correct), but if you try to write 0 values, it returns Right(sheet) on line 136. This is probably fine, but worth confirming the empty collection behavior is intentional.

Recommendation: Add a test case for writing empty collections at boundary positions to document expected behavior.

2. Documentation Clarity

The PR description mentions "prevents silent data loss when writing beyond Excel limits" but doesn't clarify what the old behavior was. Did previous versions:

• Silently truncate?
• Wrap around?
• Crash?

Recommendation: Add a comment in the code or PR description explaining what bug this fixes (if any).

3. Style Consistency in Error Messages (xl-core/src/com/tjclp/xl/error.scala:82-83)

The ValueCountMismatch error message format differs slightly from others:

case ValueCountMismatch(expected, actual, context) =>
  s"Expected $expected values for $context but received $actual"

vs.

case TypeMismatch(expected, actual, ref) =>
  s"Type mismatch at $ref: expected $expected, got $actual"

Minor issue: "but received" vs. "got" - consider standardizing verb choice for consistency.

4. Potential Performance Consideration

In SheetExtensions.scala:134-154 (putRow) and 169-187 (putCol), you call .toVector on the Iterable[CellValue]:

val buffered = values.toVector

Question: If a user passes a lazy sequence or stream, this forces materialization. Is this intentional for deterministic size checking?

Recommendation: Document in the scaladoc that these methods materialize the entire collection for safety (which aligns with the project's determinism goals).

---

🎯 Suggestions for Future Work

1. Consider Adding putRangeLenient

For users who want the old "fill until you run out of values" behavior, consider adding:

def putRangeLenient(range: CellRange, values: Iterable[CellValue]): Sheet =
  range.cells.zip(values).map { case (ref, value) => Cell(ref, value) } |> putAll

This gives users an explicit opt-in for the less safe behavior.

2. Consolidate Bounds Constants

Currently Column.MaxIndex0 and Row.MaxIndex0 are scattered. Consider creating an ExcelLimits object:

object ExcelLimits:
  val MaxColumn: Int = 16383  // XFD
  val MaxRow: Int = 1048575   // 1,048,576
  val MaxCellsPerSheet: Long = MaxColumn.toLong * MaxRow.toLong

Improves discoverability and reduces magic numbers.

3. Property-Based Tests for Bounds

Consider adding ScalaCheck property tests:

property("putRow never exceeds Excel column limit") {
  forAll { (startCol: Column, values: List[CellValue]) =>
    sheet.putRow(Row.from1(1), startCol, values) match
      case Right(_) => startCol.index0 + values.size <= Column.MaxIndex0 + 1
      case Left(_) => startCol.index0 + values.size > Column.MaxIndex0 + 1
  }
}

---

🔒 Security Considerations

No concerns. The bounds checking actually improves security by:

• Preventing out-of-bounds writes that could corrupt workbook state
• Making resource exhaustion (via huge ranges) more explicit and catchable

---

📊 Performance Impact

Positive impact:

• Smaller compilation units (split files) may improve incremental compilation
• Early bounds checks prevent wasted computation on invalid ranges

Negligible overhead:

• .toVector materialization is already required for deterministic size checking
• Arithmetic bounds checks are O(1)

---

✅ Final Verdict

Approve with minor suggestions. This PR:

• ✅ Improves code organization significantly (81% reduction in style.scala size!)
• ✅ Adds critical safety features aligned with project philosophy
• ✅ Maintains backward compatibility where possible, breaks it intentionally where needed
• ✅ Includes comprehensive tests (267 passing)
• ✅ Well-documented in PR description and CHANGELOG

Action Items (Non-Blocking)

1. Add test for empty collection at boundary (documents behavior)
2. Clarify old behavior in comments (what bug does this fix?)
3. Consider standardizing error message verbs ("got" vs. "received")
4. Add scaladoc note about collection materialization in putRow/putCol

Recommendation: Merge after addressing documentation clarity suggestions (items 1-2). Items 3-4 can be follow-up PRs.

---

Reviewed according to CLAUDE.md guidelines. Great work! 🎉