refactor: modular architecture with unified error handling and DashMap concurrency

- Break monolithic records.rs into commands/, output/, parsing/ modules - Add lib.rs with shared types (AppError, Car, BestCarRecord, etc.) - Replace Arc<Mutex<HashMap>> with DashMap for concurrent access - Add unified AppError enum with thiserror - Add configurable CLI paths (--my-cars, --cars-db, --times-dir, --tracks-db) - Add serde deserialization for Car struct - Add CSV escaping for fields with commas/quotes/newlines - Fix track name splitting: split_once -> rsplit_once for _a handling - Add integration tests, proptest property-based tests, and unit tests - Add rustfmt.toml configuration - Update documentation (API, ARCHITECTURE, DATA_STRUCTURES, README)
2026-05-08 16:19:01 +02:00 · 2026-05-08 16:19:01 +02:00 · a9a46ee551
commit a9a46ee551
parent 9d57b18a4e
24 changed files with 3541 additions and 1470 deletions
--- a/plans/code-review.md
+++ b/plans/code-review.md
@ -0,0 +1,396 @@
+# Top Drives Records Analyzer — Code Review
+
+## Executive Summary
+
+The project is a well-structured Rust CLI tool for analyzing car performance data from the Top Drives racing game. The code is generally clean, well-documented, and follows good Rust practices. The recent v0.3.0 refactor (moving from global mutable state to `AnalysisContext`, adding `CarCounts`, and introducing the `all` command) shows a clear improvement trajectory.
+
+However, there are several areas where the codebase could be significantly improved: **error handling robustness**, **test coverage depth**, **documentation accuracy**, **performance optimizations**, and **code organization**.
+
+---
+
+## 1. Code Structure & Design
+
+### 1.1 ✅ What's Done Well
+
+- **Clean separation** between library ([`lib.rs`](src/lib.rs)) and binary ([`records.rs`](src/records.rs))
+- **Good use of `AnalysisContext`** to eliminate global mutable state — this is a solid architectural improvement
+- **`CarCounts` struct** centralizes derived data computation, avoiding redundant calculations
+- **`write_sorted_csv` generic helper** eliminates CSV writing boilerplate across output functions
+- **Bounded concurrency** via `Semaphore(20)` prevents resource exhaustion
+- **`clap` derive macros** for CLI parsing — idiomatic and maintainable
+
+### 1.2 🔧 Suggested Improvements
+
+#### A. Monolithic `records.rs` — Break Into Modules
+
+[`records.rs`](src/records.rs) is 923 lines with parsing logic, output logic, fetch logic, CLI definition, and tests all in one file. This should be split into a module tree:
+
+```
+src/
+├── lib.rs
+├── records.rs          # main() + CLI only
+├── commands/
+│   ├── mod.rs          # Command dispatch
+│   ├── fetch.rs        # Fetch logic
+│   ├── best.rs         # Best/very_best output
+│   ├── trash.rs        # Trash output
+│   ├── wishlist.rs     # Wishlist/add/remove output
+│   ├── details.rs      # Details output
+│   └── track.rs        # Track-specific output
+├── parsing/
+│   ├── mod.rs          # parse_all_times, parse_car_times
+│   └── car_list.rs     # get_car_list
+└── output/
+    ├── mod.rs
+    └── csv_writer.rs   # write_sorted_csv, format_counted_row
+```
+
+#### B. `parse_car_times` Is Async But Does No Async Work
+
+[`parse_car_times()`](src/records.rs:491) is declared `async` but only performs synchronous file I/O and JSON parsing. It's called inside `tokio::task::spawn`, which means it's running on the Tokio runtime but blocking it with `fs::read_to_string` and `serde_json::from_str`.
+
+**Recommendation**: Either use `tokio::task::spawn_blocking` for the synchronous work, or use `tokio::fs::read_to_string` for truly async file I/O. The current approach blocks the Tokio runtime threads.
+
+#### C. `update_best_car` Allocates Excessively
+
+[`update_best_car()`](src/records.rs:571) clones `new_record` for every RQ level from `car_rq` to `MAX_RQ` (potentially 28 clones for an RQ 85 car). It also allocates a new `String` for each key on every iteration.
+
+**Recommendation**: Consider building the key once and reusing it, or restructuring to reduce allocations in this hot path.
+
+#### D. `AnalysisContext` Re-construction in `parse_all_times`
+
+In [`parse_all_times()`](src/records.rs:625), a new `AnalysisContext` is constructed from the cloned `Arc`:
+```rust
+parse_car_times(car, &my_cars, &AnalysisContext { best_cars }).await;
+```
+This works but is semantically odd — it creates a new struct just to pass the same `Arc<Mutex>`. Consider having `parse_car_times` accept `&Arc<Mutex<...>>` directly, or pass `&AnalysisContext` by cloning the Arc.
+
+---
+
+## 2. Error Handling
+
+### 2.1 🔴 Critical: Silent Failures
+
+Several functions silently swallow errors and continue with empty data:
+
+- [`get_car_list()`](src/records.rs:688) returns `Vec::new()` on file-not-found or parse errors — if `cars_repo.json` is missing, the entire analysis runs on zero cars with no indication
+- [`parse_car_times()`](src/records.rs:497) returns `()` and silently skips cars with unreadable files
+- [`load_my_cars_data()`](src/lib.rs:172) returns empty sets on failure
+
+**Recommendation**: Use `Result<T, E>` return types consistently. For the binary, consider using `anyhow` or `thiserror` for ergonomic error handling. At minimum, distinguish between "file missing" (warn) and "file corrupt" (error).
+
+### 2.2 🔴 Mixed Error Handling Strategies
+
+The codebase mixes:
+- `Result<(), String>` for async functions ([`get_file()`](src/lib.rs:242))
+- `std::io::Result<()>` for output functions
+- `eprintln!` + early return for input parsing
+- `expect()` for mutex poisoning
+
+**Recommendation**: Define a custom error type using `thiserror`:
+```rust
+#[derive(Debug, thiserror::Error)]
+enum AppError {
+    #[error("IO error: {0}")]
+    Io(#[from] std::io::Error),
+    #[error("Network error: {0}")]
+    Network(String),
+    #[error("Parse error in {file}: {reason}")]
+    Parse { file: String, reason: String },
+}
+```
+
+### 2.3 🟡 `unwrap()` vs `expect()` Inconsistency
+
+- [`update_best_car()`](src/records.rs:581) uses `best_cars.lock().unwrap()`
+- [`compute_car_counts()`](src/lib.rs:117) uses `.expect("best_cars mutex poisoned...")`
+
+**Recommendation**: Use `.expect()` consistently with descriptive messages, or better yet, use a helper method on `AnalysisContext` that centralizes lock acquisition.
+
+---
+
+## 3. Test Coverage
+
+### 3.1 🔴 Critical: Minimal Test Coverage
+
+The test suite in [`records.rs:736-923`](src/records.rs:736) has only **8 tests**, and several are superficial:
+
+| Test | What It Actually Tests | Quality |
+|------|----------------------|---------|
+| `test_split_once` | Standard library `split_once` behavior | ❌ Trivial — tests stdlib |
+| `test_translate_track_name` | Track name lookup | ✅ Good |
+| `test_translate_track_type` | Condition code lookup | ✅ Good |
+| `test_update_best_car_logic` | Only checks struct field values | ❌ Doesn't test `update_best_car` |
+| `test_rq_range_analysis` | RQ range insertion | ✅ Good |
+| `test_time_validation` | Two float comparisons | ❌ Trivial — tests nothing |
+| `test_track_name_completeness` | Sample of track name mappings | ✅ Good |
+| `test_condition_completeness` | Sample of condition mappings | ✅ Good |
+| `test_time_tolerance_constant` | Tie-breaking logic | ✅ Best test in the suite |
+
+### 3.2 🔧 Missing Test Categories
+
+**No tests for**:
+- [`compute_car_counts()`](src/lib.rs:115) — critical aggregation logic
+- [`write_sorted_csv()`](src/records.rs:202) — output formatting
+- [`write_best_csv()`](src/records.rs:232), [`write_trash_csv()`](src/records.rs:260), [`write_wishlist_files()`](src/records.rs:296) — all output functions
+- [`compute_ideal_wishlist()`](src/records.rs:277) — wishlist algorithm
+- [`get_car_list()`](src/records.rs:688) — car database parsing
+- [`load_my_cars_data()`](src/lib.rs:172) — user data parsing
+- [`compare_cars()`](src/lib.rs:232), [`compare_records()`](src/records.rs:191) — sort comparators
+- [`parse_car_times()`](src/records.rs:491) — core parsing logic
+- CSV output correctness (field ordering, escaping, headers)
+- Edge cases: empty data, single car, all cars owned, no cars owned
+
+### 3.3 🔧 Recommended Test Improvements
+
+1. **Add integration tests** in a `tests/` directory that exercise the full pipeline with fixture data
+2. **Add property-based tests** using `proptest` for the sorting and comparison functions
+3. **Test `update_best_car`** more thoroughly: test replacement, test tie-breaking, test multiple cars at different RQ levels
+4. **Test CSV output** by writing to a buffer and comparing against expected output
+5. **Remove trivial tests** that only test standard library behavior (`test_split_once`, `test_time_validation`)
+6. **Add test utilities** for creating fixture `Car`, `BestCarRecord`, and `AnalysisContext` instances
+
+---
+
+## 4. Performance
+
+### 4.1 🟡 `update_best_car` Hot Path
+
+[`update_best_car()`](src/records.rs:571) is called for every track × condition × car combination (potentially millions of calls). Current issues:
+
+- **Holds mutex lock while iterating RQ range** — lock is held for `MAX_RQ - car_rq + 1` iterations
+- **Clones `new_record` for each RQ level** — up to 28 clones per call
+- **Allocates `String` keys in a loop** — heap allocation per iteration
+
+**Recommendation**: 
+- Pre-compute a batch of updates and apply them under a single lock
+- Use `String` interning or `&str` references for track IDs and conditions to reduce allocations
+- Consider `DashMap` instead of `Mutex<HashMap>` for finer-grained locking
+
+### 4.2 🟡 `parse_car_times` Does Synchronous I/O on Async Runtime
+
+As noted in §1.2.B, `fs::read_to_string` blocks the Tokio runtime. With 1000+ cars and a semaphore of 20, this means 20 Tokio threads can be blocked simultaneously on file I/O.
+
+### 4.3 🟡 `compute_car_counts` Clones All Records
+
+[`compute_car_counts()`](src/lib.rs:115) clones every `BestCarRecord` when building `by_rid`. For large datasets, this is significant memory allocation.
+
+### 4.4 ✅ Good: Semaphore-Based Concurrency
+
+The `Semaphore(20)` in both [`parse_all_times()`](src/records.rs:616) and [`fetch_times()`](src/records.rs:644) is a good pattern for controlling concurrency.
+
+---
+
+## 5. Documentation Issues
+
+### 5.1 🔴 Critical: `docs/API.md` Is Severely Outdated
+
+[`docs/API.md`](docs/API.md) references:
+- `print_best_cars(filter_out_owned: bool)` — **removed in v0.3.0**
+- `print_best_times()` — **removed/renamed**
+- `print_trash()` — **removed in v0.3.0**
+- `missing` command — **removed in v0.3.0**
+- `full_script` command — **never existed in current form**
+- Old function signatures that no longer match the code
+
+**This is the most critical documentation issue.** The API docs are actively misleading.
+
+### 5.2 🔴 `docs/ARCHITECTURE.md` References Removed Code
+
+[`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md:96) still references:
+```
+static BEST_CARS: Lazy<Mutex<HashMap<TripletKey, Vec<BestCarRecord>>>>
+```
+This global static was replaced by `AnalysisContext` in v0.3.0. The doc also lists `sorted.csv` as an output file, which doesn't appear in the current code.
+
+### 5.3 🟡 `README.md` Dependency List Is Wrong
+
+[`README.md`](README.md:289) lists `once_cell` as a dependency, but it's not in [`Cargo.toml`](Cargo.toml). The code uses `std::sync::OnceLock` instead (which is correct — the README is outdated).
+
+### 5.4 🟡 `BestCarRecord` Missing `prize` Field in README
+
+The [`README.md` data structure listing](README.md:319) for `BestCarRecord` doesn't include the `prize` field that exists in the [actual struct](src/lib.rs:69).
+
+### 5.5 🟡 `docs/DATA_STRUCTURES.md` Code Examples Are Outdated
+
+[`docs/DATA_STRUCTURES.md`](docs/DATA_STRUCTURES.md:237) shows code examples using `unwrap_or("")` patterns, while the actual code in [`get_car_list()`](src/records.rs:706) uses `filter_map` with `?` operator.
+
+### 5.6 ✅ Good: Inline Documentation
+
+The inline rustdoc comments are generally excellent — well-structured with purpose, arguments, examples, and special cases. The module-level doc comments are informative.
+
+---
+
+## 6. Specific Code Issues
+
+### 6.1 🟡 Hardcoded File Paths
+
+Multiple functions use hardcoded relative file paths:
+- `"my_cars.json"` in [`load_my_cars_data()`](src/lib.rs:173)
+- `"cars_repo.json"` in [`get_car_list()`](src/records.rs:689)
+- `"times/"` prefix in [`parse_car_times()`](src/records.rs:494) and [`fetch_car()`](src/records.rs:679)
+- Output file names: `"best.csv"`, `"trash.csv"`, etc.
+
+**Recommendation**: Define constants or accept configurable paths via CLI flags.
+
+### 6.2 🟡 Hardcoded URLs
+
+[`get_tracks_file()`](src/records.rs:727) and [`get_cars_file()`](src/records.rs:732) have hardcoded GitHub URLs. These should be constants at minimum.
+
+### 6.3 🟡 CSV Escaping
+
+[`format_counted_row()`](src/records.rs:225) and other output functions don't handle CSV escaping. If car brand or model names contain commas, quotes, or newlines, the CSV output will be malformed.
+
+**Recommendation**: Use the `csv` crate, or at minimum implement proper CSV escaping.
+
+### 6.4 🟡 `Test Bowl` Magic Number
+
+The Test Bowl time inversion `2000.0 / time` at [line 533](src/records.rs:533) uses a magic number with no explanation of why 2000.0 is the correct constant.
+
+### 6.5 🟢 Minor: `year as u16` and `rq as u8` Casting
+
+In [`get_car_list()`](src/records.rs:715), `year` and `rq` are cast from `u64` to `u16`/`u8` without bounds checking. If the JSON contains values outside the valid range, this will silently truncate.
+
+### 6.6 🟢 Minor: `test_update_best_car_logic` Test Name Is Misleading
+
+The test at [line 777](src/records.rs:777) is named `test_update_best_car_logic` but never actually calls `update_best_car()`. It only checks struct field values.
+
+---
+
+## 7. Dependency Concerns
+
+### 7.1 🟡 `tokio` with `features = ["full"]`
+
+[`Cargo.toml`](Cargo.toml:16) enables all Tokio features. For a CLI tool, only `rt-multi-thread`, `macros`, and `fs` are likely needed. Using `full` increases compile time and binary size.
+
+### 7.2 🟡 No `serde` Derive for Data Structures
+
+`Car` and `BestCarRecord` use manual JSON field extraction via `serde_json::Value`. If the JSON schemas are stable, deriving `serde::Deserialize` would be safer and more performant.
+
+### 7.3 🟢 Missing `csv` Crate
+
+Output is hand-formatted CSV. The `csv` crate would handle escaping, quoting, and provide a type-safe writing interface.
+
+---
+
+## 8. `.gitignore` and Project Configuration
+
+### 8.1 🟡 Output CSV Files Not in `.gitignore`
+
+[`.gitignore`](.gitignore) ignores `times/`, `cars_repo.json`, `tracks_repo.json`, and `my_cars.json`, but doesn't ignore generated output files: `best.csv`, `very_best.csv`, `trash.csv`, `wishlist.csv`, `add.csv`, `remove.csv`, `details.csv`.
+
+### 8.2 🟡 No `rustfmt.toml` or `clippy.toml`
+
+No configuration for code formatting or linting standards.
+
+---
+
+## 9. Proposed Improvement Plan
+
+### Priority 1 — High Impact
+
+| # | Improvement | Rationale |
+|---|-------------|-----------|
+| 1 | Fix outdated `docs/API.md` | Currently misleading — references removed functions |
+| 2 | Fix outdated `docs/ARCHITECTURE.md` | References removed global static |
+| 3 | Fix `README.md` dependency list and struct definitions | Lists wrong deps and missing fields |
+| 4 | Add output CSV files to `.gitignore` | Prevent accidental commits |
+| 5 | Add tests for `compute_car_counts` | Critical untested aggregation logic |
+| 6 | Add tests for `compute_ideal_wishlist` | Wishlist algorithm has no coverage |
+| 7 | Add tests for CSV output functions | Output correctness is unverified |
+
+### Priority 2 — Medium Impact
+
+| # | Improvement | Rationale |
+|---|-------------|-----------|
+| 8 | Introduce custom error type with `thiserror` | Consistent error handling |
+| 9 | Use `Result` returns instead of silent failures | Prevent hidden data loss |
+| 10 | Add integration tests with fixture data | End-to-end correctness |
+| 11 | Fix `parse_car_times` async/blocking mismatch | Prevents runtime thread starvation |
+| 12 | Add CSV escaping or use `csv` crate | Data correctness for edge cases |
+| 13 | Extract hardcoded URLs as constants | Maintainability |
+| 14 | Remove trivial tests, rename misleading ones | Test suite quality |
+
+### Priority 3 — Lower Impact / Refactoring
+
+| # | Improvement | Rationale |
+|---|-------------|-----------|
+| 15 | Split `records.rs` into module tree | Maintainability as project grows |
+| 16 | Consider `DashMap` for concurrent HashMap | Performance under high contention |
+| 17 | Reduce allocations in `update_best_car` | Hot path optimization |
+| 18 | Narrow `tokio` features from `full` | Compile time and binary size |
+| 19 | Add `serde` derive for data structures | Type-safe parsing |
+| 20 | Add `rustfmt.toml` and `clippy.toml` | Code consistency |
+| 21 | Add configurable file paths via CLI | Flexibility |
+
+---
+
+## 10. Architecture Diagram — Current vs Proposed
+
+### Current Architecture
+
+```mermaid
+graph TD
+    A[main - records.rs] --> B[CLI parsing - clap]
+    B --> C[Fetch]
+    B --> D[Best]
+    B --> E[Trash]
+    B --> F[Wishlist]
+    B --> G[All]
+    B --> H[Details]
+    B --> I[Track]
+    
+    C --> J[get_file - lib.rs]
+    D --> K[run_analysis]
+    E --> K
+    F --> K
+    G --> K
+    H --> K
+    I --> K
+    
+    K --> L[parse_all_times]
+    L --> M[parse_car_times x N]
+    M --> N[update_best_car]
+    N --> O[AnalysisContext.best_cars]
+    
+    K --> P[compute_car_counts]
+    P --> Q[CarCounts]
+    
+    Q --> R[write_sorted_csv]
+    R --> S[CSV files]
+    
+    style A fill:#f9f,stroke:#333
+    style O fill:#ff9,stroke:#333
+    style Q fill:#9f9,stroke:#333
+```
+
+### Proposed Module Structure
+
+```mermaid
+graph TD
+    A[main.rs - CLI only] --> B[commands/mod.rs]
+    B --> C[fetch.rs]
+    B --> D[best.rs]
+    B --> E[trash.rs]
+    B --> F[wishlist.rs]
+    B --> G[details.rs]
+    B --> H[track.rs]
+    
+    I[lib.rs] --> J[types.rs - Car, BestCarRecord, etc.]
+    I --> K[analysis.rs - AnalysisContext, CarCounts]
+    I --> L[translate.rs - track name/type]
+    I --> M[io.rs - get_file, load_my_cars_data]
+    
+    D --> N[parsing/mod.rs]
+    E --> N
+    F --> N
+    G --> N
+    H --> N
+    N --> O[parse_all_times]
+    O --> P[parse_car_times]
+    
+    Q[output/mod.rs] --> R[csv_writer.rs]
+    D --> Q
+    E --> Q
+    F --> Q
+    G --> Q