diff --git a/plans/code-review-v2.md b/plans/code-review-v2.md deleted file mode 100644 index 6127025..0000000 --- a/plans/code-review-v2.md +++ /dev/null @@ -1,394 +0,0 @@ -# Code Review — Top Drives Records Analyzer (v0.3.0) - -**Date**: 2026-05-08 -**Reviewer**: Architect Mode -**Scope**: Full codebase review after round-1 refactoring - ---- - -## Executive Summary - -The codebase has improved significantly since the initial review. The module split is clean, error handling is consistent, and test coverage is solid. The remaining issues are **design-level improvements** — not bugs or correctness problems. They focus on reducing coupling, improving maintainability, and making the codebase more idiomatic Rust. - -**Overall Assessment**: ✅ Good — well-structured, well-documented, well-tested. The suggestions below are quality refinements. - ---- - -## 1. Architecture & Design - -### 1.1 ✅ Strengths - -- **Clean module separation**: `commands/` → `parsing/` → `output/` pipeline is well-defined -- **Proper error propagation**: `AppError` enum with `thiserror` is used consistently -- **Good concurrency model**: `DashMap` + `Semaphore` + `spawn_blocking` is the right approach -- **Zero global mutable state**: `AnalysisContext` is created in `main()` and threaded through -- **Excellent documentation**: Module-level doc comments, function docs, and separate docs/ directory - -### 1.2 🔧 `lib.rs` is a God Module (Medium) - -[`src/lib.rs`](src/lib.rs) at ~650 lines contains **all** shared types and utilities: `AppError`, `Car`, `BestCarRecord`, `AnalysisContext`, `CarCounts`, `MyCarsData`, `compute_car_counts`, `load_my_cars_data`, `compare_cars`, `get_file`, `translate_track_name`, `translate_track_type`, `escape_csv_field`, plus 200 lines of tests. - -**Problem**: Any change to any type requires recompiling all downstream modules. The file has 6+ distinct responsibilities. - -**Suggestion**: Split into focused sub-modules within `lib.rs`: - -``` -src/lib.rs → re-exports only -src/types.rs → Car, BestCarRecord, TripletKey, AppError -src/context.rs → AnalysisContext, CarCounts, compute_car_counts -src/user_data.rs → MyCarsData, load_my_cars_data -src/translate.rs → translate_track_name, translate_track_type -src/http.rs → get_file -src/csv_util.rs → escape_csv_field, compare_cars -``` - -### 1.3 🔧 `output/mod.rs` Has Mixed Responsibilities (Low-Medium) - -[`src/output/mod.rs`](src/output/mod.rs) contains: -- Generic CSV infrastructure (`write_sorted_csv`, `format_counted_row`) -- Specific output commands (`write_best_csv`, `write_trash_csv`, `write_details_csv`, `write_wishlist_files`) -- Business logic (`compute_ideal_wishlist`) -- Stdout output (`print_best_car`) -- 340 lines of tests - -**Suggestion**: Split into: -- `output/csv.rs` — generic CSV infrastructure + `format_counted_row` -- `output/wishlist.rs` — wishlist-specific logic (`compute_ideal_wishlist`, `write_wishlist_files`) -- `output/track_query.rs` — `print_best_car` (stdout output is conceptually different from CSV files) - -### 1.4 🔧 `DashMap>` — Double Arc Wrapping (Low) - -In [`src/lib.rs:127`](src/lib.rs:127): -```rust -pub best_cars: Arc>>, -``` - -Then in [`src/parsing/mod.rs:252`](src/parsing/mod.rs:252): -```rust -let best_cars = Arc::clone(&ctx.best_cars); -``` - -`DashMap` already uses `Arc` internally for its shard locks. Wrapping it in another `Arc` is redundant — `DashMap` itself is `Clone + Send + Sync` and can be shared by cloning the `DashMap` handle directly. - -**Suggestion**: Change to: -```rust -pub best_cars: DashMap>, -``` - -And share via `ctx.best_cars.clone()` (which clones only the Arc handle, not the data). - ---- - -## 2. Code Quality - -### 2.1 🔧 `update_best_car` String Allocation in Hot Loop (Medium) - -In [`src/parsing/mod.rs:184-185`](src/parsing/mod.rs:184): -```rust -let track_id_owned = track_id.to_string(); -let surface_owned = surface.to_string(); - -for rq in car_rq..=MAX_RQ { - let key = (track_id_owned.clone(), rq, surface_owned.clone()); -``` - -For a car with RQ 10, this loop runs **104 times**, cloning two strings per iteration. The `track_id_owned` and `surface_owned` are only needed to construct keys. - -**Suggestion**: Use `Cow` or restructure to avoid per-iteration clones: -```rust -for rq in car_rq..=MAX_RQ { - let key = (track_id.to_string(), rq, surface.to_string()); -``` -This is the same number of allocations but avoids the intermediate owned strings. Or, since `TripletKey = (String, u8, String)`, consider changing the key type to `(Arc, u8, Arc)` to make cloning O(1). - -### 2.2 🔧 `compute_car_counts` Returns `Result` But Never Fails (Low) - -In [`src/lib.rs:156`](src/lib.rs:156): -```rust -pub fn compute_car_counts(ctx: &AnalysisContext, car_list: Vec) -> Result { -``` - -This function only does `HashMap` inserts and `DashMap` iteration — it **cannot fail**. The `Result` return is misleading and forces all callers to `.unwrap()`. - -**Suggestion**: Return `CarCounts` directly: -```rust -pub fn compute_car_counts(ctx: &AnalysisContext, car_list: Vec) -> CarCounts { -``` - -### 2.3 🔧 Hardcoded Output File Paths (Low-Medium) - -All CSV output functions hardcode their filenames: -- [`write_best_csv`](src/output/mod.rs:78) → `"best.csv"` -- [`write_very_best_csv`](src/output/mod.rs:91) → `"very_best.csv"` -- [`write_trash_csv`](src/output/mod.rs:110) → `"trash.csv"` -- [`write_details_csv`](src/output/mod.rs:148) → `"details.csv"` -- [`write_wishlist_files`](src/output/mod.rs:205) → `"wishlist.csv"`, `"add.csv"`, `"remove.csv"` - -**Problem**: Users cannot specify an output directory. All files are written to CWD. - -**Suggestion**: Add an `--output-dir` CLI option and pass the output directory through to the write functions. Or at minimum, extract the filenames as constants. - -### 2.4 🔧 Magic Number `80` in Trash Filter (Low) - -In [`src/output/mod.rs:114`](src/output/mod.rs:114): -```rust -.filter(|car| !car.prize && car.rq < 80) -``` - -The value `80` is a business rule (minimum RQ for keeping a non-best car) but is not named. - -**Suggestion**: Extract as a named constant: -```rust -const TRASH_MIN_RQ: u8 = 80; -``` - -### 2.5 🔧 `write_details_csv` Doesn't Use `write_sorted_csv` (Low) - -[`write_details_csv`](src/output/mod.rs:148) manually creates a file and iterates, while all other output functions use the generic [`write_sorted_csv`](src/output/mod.rs:35). This is because `details.csv` has a different row format, but the pattern could still be unified. - -**Suggestion**: Either refactor `write_sorted_csv` to be more flexible, or add a comment explaining why this function is different. - -### 2.6 💡 `#[allow(clippy::new_without_default)]` — Just Implement Default (Low) - -In [`src/lib.rs:132`](src/lib.rs:132): -```rust -#[allow(clippy::new_without_default)] -pub fn new() -> Self { -``` - -Since `AnalysisContext` only contains `Arc>` which implements `Default`, the derive is trivial: - -**Suggestion**: -```rust -#[derive(Default)] -pub struct AnalysisContext { ... } -``` -Then remove the manual `new()` and the allow attribute. - ---- - -## 3. Error Handling - -### 3.1 ✅ Strengths - -- `AppError` with `thiserror` is clean and comprehensive -- `load_my_cars_or_warn` correctly distinguishes `MissingFile` from real errors -- `fetch_times` returns the first error encountered -- `parse_all_times` detects total failure and propagates - -### 3.2 🔧 `writeln!` Errors Silently Propagated via `?` in `write_sorted_csv` (Low) - -In [`src/output/mod.rs:49-52`](src/output/mod.rs:49): -```rust -writeln!(file, "{header}")?; -for i in indices { - format_row(&entries[i], &mut file)?; -} -``` - -The `?` operator converts `std::io::Error` → `AppError::Io` via the `From` impl. This is correct but means a partial CSV file may be left on disk if writing fails mid-way. - -**Suggestion**: Consider writing to a temporary file and renaming on success (same pattern as [`get_file`](src/lib.rs:279)), or at minimum document that partial writes are possible. - ---- - -## 4. Performance - -### 4.1 🔧 `parse_all_times` Clones Every Car (Medium) - -In [`src/parsing/mod.rs:257`](src/parsing/mod.rs:257): -```rust -let car = car.clone(); -``` - -Every car in the database is cloned to move into the async task. For ~8000+ cars, each with 5+ string fields, this is significant. - -**Suggestion**: Use `Arc` or pass only the needed fields (rid, rq, guid, brand, only_name, year, prize) as a smaller struct. - -### 4.2 🔧 `times_dir` Cloned Per Car (Low) - -In [`src/parsing/mod.rs:259`](src/parsing/mod.rs:259): -```rust -let times_dir = times_dir.to_string(); -``` - -This allocates a new String for every car. - -**Suggestion**: Use `Arc` or pass as `&str` by restructuring the spawn. - -### 4.3 💡 Consider `rayon` for `compute_car_counts` (Low) - -[`compute_car_counts`](src/lib.rs:156) iterates all `DashMap` entries sequentially. For large datasets, this could benefit from parallel iteration. - ---- - -## 5. Test Coverage - -### 5.1 ✅ Strengths - -- **Unit tests** in every module (`lib.rs`, `parsing/mod.rs`, `parsing/car_list.rs`, `output/mod.rs`) -- **Integration tests** in `tests/integration_tests.rs` covering full pipelines -- **Property-based tests** in `tests/proptest_tests.rs` for sorting invariants -- **Edge case tests**: empty input, missing files, downvoted times, Test Bowl inversion -- Good use of `tempfile::tempdir()` for file-based tests - -### 5.2 🔧 Duplicated Test Helpers (Medium) - -The `make_record()` and `make_car()` helper functions are duplicated across: -- [`src/parsing/mod.rs:369`](src/parsing/mod.rs:369) -- [`src/output/mod.rs:319`](src/output/mod.rs:319) -- [`tests/integration_tests.rs:17`](tests/integration_tests.rs:17) - -**Suggestion**: Create `tests/common/mod.rs` or a `test_utils` module in `lib.rs` with shared helpers: - -```rust -// src/test_utils.rs (gated behind #[cfg(test)]) -pub fn make_record(car_rid: &str, rq: u8, time: f64, guid: &str) -> BestCarRecord { ... } -pub fn make_car(rid: &str, rq: u8, guid: &str) -> Car { ... } -``` - -### 5.3 🔧 Missing Test Coverage for `get_file` (Medium) - -[`get_file`](src/lib.rs:275) is the most complex function in `lib.rs` (~75 lines with retry logic, temp file handling, empty response detection). It has **zero tests**. - -**Suggestion**: Add tests using a mock HTTP server (e.g., `mockito` crate) or at minimum test the retry/backoff logic. - -### 5.4 🔧 Missing Test Coverage for `fetch_times` and `fetch_car` (Medium) - -[`fetch_times`](src/parsing/mod.rs:317) and [`fetch_car`](src/parsing/mod.rs:351) have no tests. These contain important concurrency and error-handling logic. - -### 5.5 🔧 No Tests for Command Handlers (Low) - -None of the command handlers in `src/commands/` have dedicated tests. While they're thin wrappers, testing `load_my_cars_or_warn`'s graceful degradation and `run_analysis`'s orchestration would be valuable. - -### 5.6 💡 `proptest` Only Tests `compare_cars` and `escape_csv_field` (Low) - -The property-based test suite could be expanded to cover: -- `update_best_car` — properties like "inserting the same car twice doesn't change the result" -- `compute_car_counts` — "count is always >= 1 for any car in `best_cars`" -- `translate_track_name` — "output is never empty for known tracks" - ---- - -## 6. Documentation - -### 6.1 ✅ Strengths - -- All public functions have doc comments with purpose, arguments, and return values -- Module-level `//!` doc comments explain each module's role -- `docs/` directory covers API, architecture, data structures, setup, and user guide -- `CHANGELOG.md` tracks version history - -### 6.2 🔧 `docs/DATA_STRUCTURES.md` May Be Outdated (Low) - -Need to verify that `DATA_STRUCTURES.md` reflects the current struct definitions, especially `AnalysisContext` with `DashMap` and the new `CarCounts` struct. - -### 6.3 🔧 `docs/DEVELOPMENT_SETUP.md` — Missing `mockito` or Test Dependencies (Low) - -If `mockito` or other test dependencies are added, they should be documented. - -### 6.4 💡 No `CONTRIBUTING.md` (Low) - -The README mentions "Contributing" in the table of contents but there's no contributing guide. - ---- - -## 7. Dependencies - -### 7.1 ✅ Strengths - -- Tokio features are properly narrowed -- `thiserror v2`, `dashmap v6`, `clap v4` are all current -- `proptest` and `tempfile` are correctly in `[dev-dependencies]` - -### 7.2 🔧 Consider Adding `mockito` (Low) - -For testing `get_file`'s retry logic and HTTP error handling. - -### 7.3 💡 Consider Replacing `futures::future::join_all` with `tokio::join!` or `futures::join!` (Low) - -In [`src/parsing/mod.rs:274`](src/parsing/mod.rs:274): -```rust -let results = join_all(handles).await; -``` - -`join_all` buffers all futures in memory. For ~8000 cars this means ~8000 `JoinHandle`s allocated upfront. This is fine for the current scale but could be replaced with `FuturesUnordered` for streaming results. - ---- - -## 8. Summary of Recommended Improvements - -### Priority 1 — Design Improvements - -| # | Issue | File | Impact | -|---|-------|------|--------| -| 1 | Split `lib.rs` into sub-modules | `src/lib.rs` | Reduces coupling, faster compile times | -| 2 | Remove outer `Arc` from `DashMap` field | `src/lib.rs:127` | Removes unnecessary indirection | -| 3 | Deduplicate test helpers | Multiple | Reduces maintenance burden | -| 4 | Add `mockito` + tests for `get_file` | `src/lib.rs` | Covers untested critical path | - -### Priority 2 — Code Quality - -| # | Issue | File | Impact | -|---|-------|------|--------| -| 5 | `compute_car_counts` should return `CarCounts` not `Result` | `src/lib.rs:156` | Removes misleading API | -| 6 | Extract `TRASH_MIN_RQ` constant | `src/output/mod.rs:114` | Names business rule | -| 7 | Implement `Default` for `AnalysisContext` | `src/lib.rs:132` | Removes clippy allow | -| 8 | Add `--output-dir` CLI option | `src/records.rs` | User configurability | -| 9 | Atomic CSV writes via temp+rename | `src/output/mod.rs` | Prevents partial files | - -### Priority 3 — Performance & Polish - -| # | Issue | File | Impact | -|---|-------|------|--------| -| 10 | Reduce string cloning in `update_best_car` loop | `src/parsing/mod.rs:184` | Performance | -| 11 | Use `Arc` or slim struct in `parse_all_times` | `src/parsing/mod.rs:257` | Reduces allocations | -| 12 | Split `output/mod.rs` into sub-modules | `src/output/` | Maintainability | -| 13 | Expand proptest coverage | `tests/proptest_tests.rs` | Test confidence | -| 14 | Add command handler tests | `src/commands/` | Test coverage | - ---- - -## Architecture Diagram (Current) - -```mermaid -graph TD - CLI[src/records.rs - CLI Entry Point] --> CMD[src/commands/ - Command Handlers] - CMD --> PARSE[src/parsing/ - Core Parsing] - CMD --> OUT[src/output/ - CSV Output] - PARSE --> LIB[src/lib.rs - Shared Library] - CMD --> LIB - OUT --> LIB - - LIB --> TYPES[Types: Car, BestCarRecord, AppError] - LIB --> CTX[AnalysisContext with DashMap] - LIB --> UTILS[Utilities: translate_*, get_file, escape_csv_field] - - PARSE --> |spawn_blocking| PARSE - PARSE --> |Semaphore limited| FETCH[fetch_times - HTTP Downloads] - - CMD --> |run_analysis| PARSE - CMD --> |compute_car_counts| LIB -``` - -## Proposed Architecture (After Refactoring) - -```mermaid -graph TD - CLI[src/records.rs - CLI Entry Point] --> CMD[src/commands/ - Command Handlers] - CMD --> PARSE[src/parsing/ - Core Parsing] - CMD --> OUT[src/output/ - CSV Output] - PARSE --> LIB[src/lib.rs - Re-exports only] - CMD --> LIB - OUT --> LIB - - LIB --> TYPES[src/types.rs - Car, BestCarRecord, AppError] - LIB --> CTX[src/context.rs - AnalysisContext, CarCounts] - LIB --> USER[src/user_data.rs - MyCarsData] - LIB --> TRANS[src/translate.rs - Track name/type] - LIB --> HTTP[src/http.rs - get_file with retry] - LIB --> CSV[src/csv_util.rs - escape_csv_field, compare_cars] - - OUT --> WISHLIST[src/output/wishlist.rs] - OUT --> TRACK_Q[src/output/track_query.rs] -``` diff --git a/src/commands/all.rs b/src/commands/all.rs index e1bfcba..5957cff 100644 --- a/src/commands/all.rs +++ b/src/commands/all.rs @@ -1,7 +1,7 @@ //! All command implementation. //! //! Generates all output files in a single pass: best.csv, very_best.csv, -//! trash.csv, details.csv, wishlist.csv, add.csv, and remove.csv. +//! trash.csv, details.csv, wishlist.csv, add.csv, remove.csv, and improve.csv. use top_drives::AppError; @@ -18,16 +18,22 @@ pub async fn run_all( max_wishlist: usize, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; output::write_best_csv(output_dir, &counts)?; output::write_very_best_csv(output_dir, &counts, min_count)?; output::write_trash_csv(output_dir, &counts, &my_data.owned)?; output::write_details_csv(output_dir, &ctx)?; output::write_wishlist_files( - output_dir, &counts, &my_data.owned, &my_data.wishlist, min_count, max_wishlist, + output_dir, + &counts, + &my_data.owned, + &my_data.wishlist, + min_count, + max_wishlist, )?; + output::write_improve_csv(output_dir, &counts)?; println!( - "Written: best.csv, very_best.csv, trash.csv, details.csv, wishlist.csv, add.csv, remove.csv" + "Written: best.csv, very_best.csv, trash.csv, details.csv, wishlist.csv, add.csv, remove.csv, improve.csv" ); Ok(()) } diff --git a/src/commands/best.rs b/src/commands/best.rs index 8fafdb3..1aa1d19 100644 --- a/src/commands/best.rs +++ b/src/commands/best.rs @@ -9,9 +9,15 @@ use crate::commands::{load_my_cars_or_warn, run_analysis}; use crate::output::{write_best_csv, write_very_best_csv}; /// Executes the best command: generates best.csv and very_best.csv. -pub async fn run_best(my_cars_path: &str, cars_db_path: &str, times_dir: &str, output_dir: &str, min_count: usize) -> Result<(), AppError> { +pub async fn run_best( + my_cars_path: &str, + cars_db_path: &str, + times_dir: &str, + output_dir: &str, + min_count: usize, +) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; write_best_csv(output_dir, &counts)?; write_very_best_csv(output_dir, &counts, min_count)?; println!("Written: best.csv, very_best.csv"); diff --git a/src/commands/details.rs b/src/commands/details.rs index 87f59af..90ccc66 100644 --- a/src/commands/details.rs +++ b/src/commands/details.rs @@ -9,9 +9,14 @@ use crate::commands::{load_my_cars_or_warn, run_analysis}; use crate::output::write_details_csv; /// Executes the details command: generates details.csv. -pub async fn run_details(my_cars_path: &str, cars_db_path: &str, times_dir: &str, output_dir: &str) -> Result<(), AppError> { +pub async fn run_details( + my_cars_path: &str, + cars_db_path: &str, + times_dir: &str, + output_dir: &str, +) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, _counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (ctx, _counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; write_details_csv(output_dir, &ctx)?; println!("Written: details.csv"); Ok(()) diff --git a/src/commands/fetch.rs b/src/commands/fetch.rs index 6563f2a..260653b 100644 --- a/src/commands/fetch.rs +++ b/src/commands/fetch.rs @@ -3,7 +3,7 @@ //! Downloads car database, tracks database, and per-car timing data //! from remote repositories. -use top_drives::{AppError, get_file}; +use top_drives::{get_file, AppError}; use crate::parsing::fetch_times; @@ -39,7 +39,11 @@ pub async fn get_cars_file(cars_db_path: &str) -> Result<(), AppError> { /// Downloads the car database, tracks database, and per-car timing files. /// Fails fast on critical dependency failures — the cars DB must be available /// before per-car timing files can be fetched. -pub async fn run_fetch(cars_db_path: &str, tracks_db_path: &str, times_dir: &str) -> Result<(), AppError> { +pub async fn run_fetch( + cars_db_path: &str, + tracks_db_path: &str, + times_dir: &str, +) -> Result<(), AppError> { get_cars_file(cars_db_path).await?; get_tracks_file(tracks_db_path).await?; fetch_times(cars_db_path, times_dir).await?; diff --git a/src/commands/improve.rs b/src/commands/improve.rs new file mode 100644 index 0000000..c533f7e --- /dev/null +++ b/src/commands/improve.rs @@ -0,0 +1,27 @@ +//! Improve command implementation. +//! +//! Generates improve.csv — owned cars not yet in a final configuration, +//! sorted by number of best tracks (descending). + +use top_drives::AppError; + +use crate::commands::{load_my_cars_or_warn, run_analysis}; +use crate::output::write_improve_csv; + +/// Executes the improve command: generates improve.csv. +pub async fn run_improve( + my_cars_path: &str, + cars_db_path: &str, + times_dir: &str, + output_dir: &str, +) -> Result<(), AppError> { + let my_data = load_my_cars_or_warn(my_cars_path)?; + let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let count = write_improve_csv(output_dir, &counts)?; + if count == 0 { + println!("No cars to improve — all owned cars are already in final or maxed configs."); + } else { + println!("Written: improve.csv ({count} cars to improve)"); + } + Ok(()) +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 1be57a6..8c12d9f 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -7,16 +7,15 @@ pub mod all; pub mod best; pub mod details; pub mod fetch; +pub mod improve; pub mod track; pub mod trash; pub mod wishlist; -use std::collections::HashSet; use std::sync::Arc; use top_drives::{ - AppError, AnalysisContext, CarCounts, MyCarsData, - compute_car_counts, load_my_cars_data, + compute_car_counts, load_my_cars_data, AnalysisContext, AppError, CarCounts, MyCarsData, }; use crate::parsing::parse_all_times; @@ -51,13 +50,14 @@ pub fn load_my_cars_or_warn(path: &str) -> Result { /// Returns the analysis context (for stdout commands) and the aggregated car counts /// (for CSV output commands). The car list is stored inside `CarCounts`. pub async fn run_analysis( - owned: &HashSet, + my_data: &MyCarsData, cars_db_path: &str, times_dir: &str, ) -> Result<(AnalysisContext, CarCounts), AppError> { let ctx = AnalysisContext::new(); - let owned = Arc::new(owned.clone()); + let owned = Arc::new(my_data.owned.clone()); let car_list = parse_all_times(owned, &ctx, cars_db_path, times_dir).await?; - let counts = compute_car_counts(&ctx, car_list); + // Move only owned_configs; construct a new MyCarsData to transfer ownership. + let counts = compute_car_counts(&ctx, car_list, my_data.owned_configs.clone()); Ok((ctx, counts)) } diff --git a/src/commands/track.rs b/src/commands/track.rs index 30771b3..9b77189 100644 --- a/src/commands/track.rs +++ b/src/commands/track.rs @@ -22,6 +22,6 @@ pub async fn run_track( rq: u8, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, _counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (ctx, _counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; print_best_car(&ctx, track, condition, rq) } diff --git a/src/commands/trash.rs b/src/commands/trash.rs index f86271c..d893c3c 100644 --- a/src/commands/trash.rs +++ b/src/commands/trash.rs @@ -8,9 +8,14 @@ use crate::commands::{load_my_cars_or_warn, run_analysis}; use crate::output::write_trash_csv; /// Executes the trash command: generates trash.csv. -pub async fn run_trash(my_cars_path: &str, cars_db_path: &str, times_dir: &str, output_dir: &str) -> Result<(), AppError> { +pub async fn run_trash( + my_cars_path: &str, + cars_db_path: &str, + times_dir: &str, + output_dir: &str, +) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; write_trash_csv(output_dir, &counts, &my_data.owned)?; println!("Written: trash.csv"); Ok(()) diff --git a/src/commands/wishlist.rs b/src/commands/wishlist.rs index 8d0d0d8..e02801e 100644 --- a/src/commands/wishlist.rs +++ b/src/commands/wishlist.rs @@ -17,9 +17,14 @@ pub async fn run_wishlist( max_wishlist: usize, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data.owned, cars_db_path, times_dir).await?; + let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; write_wishlist_files( - output_dir, &counts, &my_data.owned, &my_data.wishlist, min_count, max_wishlist, + output_dir, + &counts, + &my_data.owned, + &my_data.wishlist, + min_count, + max_wishlist, )?; println!("Written: wishlist.csv, add.csv, remove.csv"); Ok(()) diff --git a/src/context.rs b/src/context.rs index b22275e..0a6b673 100644 --- a/src/context.rs +++ b/src/context.rs @@ -3,7 +3,7 @@ //! Contains the mutable analysis state (`AnalysisContext`) and the //! aggregated per-car win counts (`CarCounts`) computed from it. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use dashmap::DashMap; @@ -33,24 +33,29 @@ impl Default for AnalysisContext { impl AnalysisContext { /// Create a new, empty analysis context. pub fn new() -> Self { - AnalysisContext { - best_cars: Arc::new(DashMap::new()), - } + AnalysisContext { best_cars: Arc::new(DashMap::new()) } } } /// Aggregated per-car win counts across all track/RQ/condition combinations. /// Computed once from `AnalysisContext` and shared across all output functions. +/// +/// The composite key `(car_rid, normalized_config)` ensures that the same car +/// with different final builds (e.g., "323" vs "332") is tracked separately. pub struct CarCounts { - /// Map from car_rid → (BestCarRecord, count of tracks where this car is best) - pub by_rid: HashMap, - /// Map from guid → car_rid (for O(1) ownership/wishlist lookups) - pub guid_to_rid: HashMap, + /// Map from (car_rid, normalized_config) → (BestCarRecord, count of tracks where this car+config is best) + pub by_rid: HashMap<(String, String), (BestCarRecord, usize)>, + /// Map from guid → set of (car_rid, normalized_config) for O(1) ownership/wishlist lookups. + /// A guid may map to multiple entries if the same car is best with different configs. + pub guid_to_rid: HashMap>, /// Map from guid → Car (for O(1) lookups of car metadata by GUID). /// Built once in `compute_car_counts()` and reused by trash/wishlist output. pub car_map: HashMap, /// Full car list from the database (kept for any callers that need iteration). pub car_list: Vec, + /// Actual tune config for each owned car (guid → config string from playerDeck). + /// Used by the improve command to determine which cars are not yet at a final build. + pub owned_configs: HashMap, } /// Computes per-car win counts from the analysis context. @@ -58,39 +63,46 @@ pub struct CarCounts { /// Returns a `CarCounts` struct that can be shared across all output functions. /// This function cannot fail — it only performs `DashMap` iteration and /// `HashMap` inserts. -pub fn compute_car_counts(ctx: &AnalysisContext, car_list: Vec) -> CarCounts { - let mut by_rid: HashMap = HashMap::new(); - let mut guid_to_rid: HashMap = HashMap::new(); +/// +/// Groups by `(car_rid, normalized_config)` so that the same car with different +/// final builds is counted as separate entries. +pub fn compute_car_counts( + ctx: &AnalysisContext, + car_list: Vec, + owned_configs: HashMap, +) -> CarCounts { + let mut by_rid: HashMap<(String, String), (BestCarRecord, usize)> = HashMap::new(); + let mut guid_to_rid: HashMap> = HashMap::new(); for entry in ctx.best_cars.iter() { for best_car in entry.value() { - let rid_entry = by_rid - .entry(best_car.car_rid.clone()) - .or_insert_with(|| (best_car.clone(), 0usize)); + let key = (best_car.car_rid.clone(), best_car.normalized_config.clone()); + let rid_entry = by_rid.entry(key.clone()).or_insert_with(|| (best_car.clone(), 0usize)); rid_entry.1 += 1; - // Keep the record with the best (lowest) time for this car. + // Keep the record with the best (lowest) time for this car+config. if best_car.time < rid_entry.0.time { rid_entry.0 = best_car.clone(); } - // HashMap iteration order is non-deterministic, but car_rid and guid - // have a 1:1 relationship in the data model, so any mapping is correct. - guid_to_rid.entry(best_car.guid.clone()) - .or_insert_with(|| best_car.car_rid.clone()); + // Track all (rid, config) pairs for each guid. + guid_to_rid.entry(best_car.guid.clone()).or_default().insert(key); } } // Build guid → Car lookup once, reused by trash/wishlist output functions. - let car_map: HashMap = car_list.iter() - .map(|c| (c.guid.clone(), c.clone())) - .collect(); + let car_map: HashMap = + car_list.iter().map(|c| (c.guid.clone(), c.clone())).collect(); - CarCounts { by_rid, guid_to_rid, car_map, car_list } + CarCounts { by_rid, guid_to_rid, car_map, car_list, owned_configs } } #[cfg(test)] mod tests { use super::*; + fn empty_owned_configs() -> HashMap { + HashMap::new() + } + #[test] fn test_compute_car_counts_basic() { let ctx = AnalysisContext::new(); @@ -101,6 +113,7 @@ mod tests { year: 2020, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 100.0, owned: true, prize: false, @@ -108,14 +121,10 @@ mod tests { }; // Manually insert into best_cars - ctx.best_cars.insert( - ("Laguna Seca".to_string(), 85, "Dry".to_string()), - vec![record.clone()], - ); - ctx.best_cars.insert( - ("Monaco Streets".to_string(), 85, "Dry".to_string()), - vec![record.clone()], - ); + ctx.best_cars + .insert(("Laguna Seca".to_string(), 85, "Dry".to_string()), vec![record.clone()]); + ctx.best_cars + .insert(("Monaco Streets".to_string(), 85, "Dry".to_string()), vec![record.clone()]); let car_list = vec![Car { rid: "Test_Car".to_string(), @@ -127,16 +136,19 @@ mod tests { guid: "guid_test".to_string(), }]; - let counts = compute_car_counts(&ctx, car_list); + let my_data = empty_owned_configs(); + let counts = compute_car_counts(&ctx, car_list, my_data); // Should have exactly 1 entry in by_rid assert_eq!(counts.by_rid.len(), 1); - let (rec, count) = counts.by_rid.get("Test_Car").unwrap(); + let (rec, count) = counts.by_rid.get(&("Test_Car".to_string(), "332".to_string())).unwrap(); assert_eq!(count, &2); // best on 2 tracks assert_eq!(rec.car_rid, "Test_Car"); - // guid_to_rid should map guid → rid - assert_eq!(counts.guid_to_rid.get("guid_test"), Some(&"Test_Car".to_string())); + // guid_to_rid should map guid → set of (rid, config) + let guid_entries = counts.guid_to_rid.get("guid_test").unwrap(); + assert_eq!(guid_entries.len(), 1); + assert!(guid_entries.contains(&("Test_Car".to_string(), "332".to_string()))); // car_map should have the car assert!(counts.car_map.contains_key("guid_test")); @@ -154,6 +166,7 @@ mod tests { year: 2020, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 90.0, owned: false, prize: false, @@ -166,24 +179,27 @@ mod tests { year: 2020, rq: 85, config: "233".to_string(), + normalized_config: "233".to_string(), time: 110.0, owned: false, prize: false, guid: "guid_a".to_string(), }; - ctx.best_cars.insert( - ("Track1".to_string(), 85, "Dry".to_string()), - vec![record_fast], - ); - ctx.best_cars.insert( - ("Track2".to_string(), 85, "Dry".to_string()), - vec![record_slow], - ); + ctx.best_cars.insert(("Track1".to_string(), 85, "Dry".to_string()), vec![record_fast]); + ctx.best_cars.insert(("Track2".to_string(), 85, "Dry".to_string()), vec![record_slow]); - let counts = compute_car_counts(&ctx, vec![]); - let (rec, count) = counts.by_rid.get("Car_A").unwrap(); - assert_eq!(*count, 2); - assert_eq!(rec.time, 90.0); // Should keep the faster time + let my_data = empty_owned_configs(); + let counts = compute_car_counts(&ctx, vec![], my_data); + // Different normalized configs → separate entries + assert_eq!(counts.by_rid.len(), 2); + let (rec_fast, count_fast) = + counts.by_rid.get(&("Car_A".to_string(), "332".to_string())).unwrap(); + assert_eq!(*count_fast, 1); + assert_eq!(rec_fast.time, 90.0); + let (rec_slow, count_slow) = + counts.by_rid.get(&("Car_A".to_string(), "233".to_string())).unwrap(); + assert_eq!(*count_slow, 1); + assert_eq!(rec_slow.time, 110.0); } } diff --git a/src/csv_util.rs b/src/csv_util.rs index e621c3a..d362433 100644 --- a/src/csv_util.rs +++ b/src/csv_util.rs @@ -20,9 +20,7 @@ pub fn escape_csv_field(field: &str) -> String { /// /// Used as a shared comparator to ensure consistent ordering across output functions. pub fn compare_cars(a: &Car, b: &Car) -> std::cmp::Ordering { - a.rq.cmp(&b.rq) - .then(a.brand.cmp(&b.brand)) - .then(a.only_name.cmp(&b.only_name)) + a.rq.cmp(&b.rq).then(a.brand.cmp(&b.brand)).then(a.only_name.cmp(&b.only_name)) } #[cfg(test)] @@ -44,16 +42,31 @@ mod tests { #[test] fn test_compare_cars_ordering() { let car_a = Car { - rid: "A".to_string(), brand: "BMW".to_string(), only_name: "M3".to_string(), - year: 2020, rq: 80, prize: false, guid: "guid_a".to_string(), + rid: "A".to_string(), + brand: "BMW".to_string(), + only_name: "M3".to_string(), + year: 2020, + rq: 80, + prize: false, + guid: "guid_a".to_string(), }; let car_b = Car { - rid: "B".to_string(), brand: "Audi".to_string(), only_name: "RS6".to_string(), - year: 2021, rq: 90, prize: false, guid: "guid_b".to_string(), + rid: "B".to_string(), + brand: "Audi".to_string(), + only_name: "RS6".to_string(), + year: 2021, + rq: 90, + prize: false, + guid: "guid_b".to_string(), }; let car_c = Car { - rid: "C".to_string(), brand: "BMW".to_string(), only_name: "M5".to_string(), - year: 2022, rq: 80, prize: false, guid: "guid_c".to_string(), + rid: "C".to_string(), + brand: "BMW".to_string(), + only_name: "M5".to_string(), + year: 2022, + rq: 80, + prize: false, + guid: "guid_c".to_string(), }; // Lower RQ sorts first diff --git a/src/http.rs b/src/http.rs index 39bfffb..f69253f 100644 --- a/src/http.rs +++ b/src/http.rs @@ -44,15 +44,14 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr status, attempt + 1, ); - tokio::time::sleep( - std::time::Duration::from_millis(500 * (attempt + 1) as u64) - ).await; + tokio::time::sleep(std::time::Duration::from_millis( + 500 * (attempt + 1) as u64, + )) + .await; attempt += 1; continue; } - return Err(AppError::Network( - format!("HTTP {status} from {file_url}") - )); + return Err(AppError::Network(format!("HTTP {status} from {file_url}"))); } match response.text().await { Ok(content) => { @@ -62,23 +61,24 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr "Warning: empty response from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...", attempt + 1, ); - tokio::time::sleep( - std::time::Duration::from_millis(500 * (attempt + 1) as u64) - ).await; + tokio::time::sleep(std::time::Duration::from_millis( + 500 * (attempt + 1) as u64, + )) + .await; attempt += 1; continue; } - return Err(AppError::Network( - format!("Empty response from {file_url} after {MAX_FETCH_RETRIES} retries") - )); + return Err(AppError::Network(format!( + "Empty response from {file_url} after {MAX_FETCH_RETRIES} retries" + ))); } fs::write(&tmp_path, &content).map_err(AppError::Io)?; return fs::rename(&tmp_path, local_file_path).map_err(AppError::Io); } Err(e) => { - return Err(AppError::Network( - format!("Error reading response from {file_url}: {e}") - )); + return Err(AppError::Network(format!( + "Error reading response from {file_url}: {e}" + ))); } } } @@ -88,15 +88,16 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr "Warning: failed to fetch {file_url} (attempt {}/{MAX_FETCH_RETRIES}): {error}", attempt + 1, ); - tokio::time::sleep( - std::time::Duration::from_millis(500 * (attempt + 1) as u64) - ).await; + tokio::time::sleep(std::time::Duration::from_millis( + 500 * (attempt + 1) as u64, + )) + .await; attempt += 1; continue; } - return Err(AppError::Network( - format!("Failed to fetch {file_url} after {MAX_FETCH_RETRIES} retries: {error}") - )); + return Err(AppError::Network(format!( + "Failed to fetch {file_url} after {MAX_FETCH_RETRIES} retries: {error}" + ))); } } } diff --git a/src/lib.rs b/src/lib.rs index adf540d..eaed94f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,9 +41,12 @@ pub mod test_utils; // Re-export all public types and functions for backward compatibility. // External code can use `top_drives::Car` etc. without knowing the internal module layout. -pub use context::{AnalysisContext, CarCounts, compute_car_counts}; +pub use context::{compute_car_counts, AnalysisContext, CarCounts}; pub use csv_util::{compare_cars, escape_csv_field}; pub use http::get_file; pub use translate::{translate_track_name, translate_track_type}; -pub use types::{AppError, BestCarRecord, Car, TripletKey}; -pub use user_data::{MyCarsData, load_my_cars_data}; +pub use types::{normalize_to_final_build, AppError, BestCarRecord, Car, TripletKey}; +pub use user_data::{load_my_cars_data, MyCarsData}; + +#[cfg(feature = "test-utils")] +pub use test_utils::empty_my_data; diff --git a/src/output/mod.rs b/src/output/mod.rs index c11ff14..7c368ae 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -3,12 +3,15 @@ //! Provides generic CSV writing infrastructure and comparison functions //! used across multiple output commands. -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::fs; use std::io::Write; use std::path::PathBuf; -use top_drives::{BestCarRecord, CarCounts, TripletKey, AnalysisContext, AppError, compare_cars, escape_csv_field}; +use top_drives::{ + compare_cars, escape_csv_field, normalize_to_final_build, AnalysisContext, AppError, + BestCarRecord, CarCounts, TripletKey, +}; // --------------------------------------------------------------------------- // Constants @@ -22,12 +25,14 @@ const TRASH_MIN_RQ: u8 = 80; // Sorting helpers // --------------------------------------------------------------------------- -/// Comparison function for sorting `BestCarRecord`s by RQ, then brand, then model name. +/// Comparison function for sorting `BestCarRecord`s by RQ, then normalized config, +/// then brand, then model name. /// /// Avoids cloning into a `Car` just for sorting — uses the same ordering as /// `compare_cars` but operates directly on `BestCarRecord`. pub fn compare_records(a: &BestCarRecord, b: &BestCarRecord) -> std::cmp::Ordering { a.rq.cmp(&b.rq) + .then(a.normalized_config.cmp(&b.normalized_config)) .then(a.brand.cmp(&b.brand)) .then(a.only_name.cmp(&b.only_name)) } @@ -95,18 +100,22 @@ where }) } -/// Formats a `(BestCarRecord, usize)` row as `RQ,Brand,Model,Year,Count`. +/// Formats a `(BestCarRecord, usize)` row as `RQ,Brand,Model,Year,FinalBuild,Count`. /// Shared by `write_best_csv`, `write_very_best_csv`, and wishlist output functions. /// Fields are properly CSV-escaped to handle commas or quotes in names. -pub fn format_counted_row(entry: &(BestCarRecord, usize), file: &mut fs::File) -> std::io::Result<()> { +pub fn format_counted_row( + entry: &(BestCarRecord, usize), + file: &mut fs::File, +) -> std::io::Result<()> { let (rec, count) = entry; writeln!( file, - "{},{},{},{},{}", + "{},{},{},{},{},{}", rec.rq, escape_csv_field(&rec.brand), escape_csv_field(&rec.only_name), rec.year, + rec.normalized_config, count ) } @@ -115,30 +124,32 @@ pub fn format_counted_row(entry: &(BestCarRecord, usize), file: &mut fs::File) - // CSV output: best / very_best // --------------------------------------------------------------------------- -/// Writes best.csv — all cars that are best on at least one track, sorted by RQ. -/// Format: RQ,Brand,Model,Year,Count +/// Writes best.csv — all cars that are best on at least one track, sorted by RQ then config. +/// Format: RQ,Brand,Model,Year,FinalBuild,Count pub fn write_best_csv(output_dir: &str, counts: &CarCounts) -> Result<(), AppError> { let entries: Vec<_> = counts.by_rid.values().collect(); write_sorted_csv( output_dir, "best.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &entries, |a, b| compare_records(&a.0, &b.0), |entry, file| format_counted_row(entry, file), ) } -/// Writes very_best.csv — cars best on >= min_count tracks, sorted by RQ. -/// Format: RQ,Brand,Model,Year,Count -pub fn write_very_best_csv(output_dir: &str, counts: &CarCounts, min_count: usize) -> Result<(), AppError> { - let entries: Vec<_> = counts.by_rid.values() - .filter(|(_, count)| *count >= min_count) - .collect(); +/// Writes very_best.csv — cars best on >= min_count tracks, sorted by RQ then config. +/// Format: RQ,Brand,Model,Year,FinalBuild,Count +pub fn write_very_best_csv( + output_dir: &str, + counts: &CarCounts, + min_count: usize, +) -> Result<(), AppError> { + let entries: Vec<_> = counts.by_rid.values().filter(|(_, count)| *count >= min_count).collect(); write_sorted_csv( output_dir, "very_best.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &entries, |a, b| compare_records(&a.0, &b.0), |entry, file| format_counted_row(entry, file), @@ -151,8 +162,13 @@ pub fn write_very_best_csv(output_dir: &str, counts: &CarCounts, min_count: usiz /// Writes trash.csv — owned cars not best on any track, RQ < TRASH_MIN_RQ, not prize. /// Format: RQ,Brand,Model,Year -pub fn write_trash_csv(output_dir: &str, counts: &CarCounts, owned: &HashSet) -> Result<(), AppError> { - let trash: Vec<_> = owned.iter() +pub fn write_trash_csv( + output_dir: &str, + counts: &CarCounts, + owned: &HashSet, +) -> Result<(), AppError> { + let trash: Vec<_> = owned + .iter() .filter(|guid| !counts.guid_to_rid.contains_key(*guid)) .filter_map(|guid| counts.car_map.get(guid)) .filter(|car| !car.prize && car.rq < TRASH_MIN_RQ) @@ -164,14 +180,16 @@ pub fn write_trash_csv(output_dir: &str, counts: &CarCounts, owned: &HashSet Result<(), AppError> { // Collect data from DashMap — no global lock needed - let mut sorted_data: Vec<(TripletKey, Vec)> = ctx - .best_cars - .iter() - .map(|entry| (entry.key().clone(), entry.value().clone())) - .collect(); + let mut sorted_data: Vec<(TripletKey, Vec)> = + ctx.best_cars.iter().map(|entry| (entry.key().clone(), entry.value().clone())).collect(); // Sort by RQ then Track ID for consistent output sorted_data.sort_by(|a, b| a.0 .1.cmp(&b.0 .1).then(a.0 .0.cmp(&b.0 .0))); @@ -209,15 +224,15 @@ pub fn write_details_csv(output_dir: &str, ctx: &AnalysisContext) -> Result<(), writeln!( file, "{},{},{},{},{},{},{},{},{}", - key.1, // RQ level - escape_csv_field(&key.0), // Track ID - escape_csv_field(&key.2), // Surface condition - best_car.rq, // Car's RQ - escape_csv_field(&best_car.brand), // Car brand + key.1, // RQ level + escape_csv_field(&key.0), // Track ID + escape_csv_field(&key.2), // Surface condition + best_car.rq, // Car's RQ + escape_csv_field(&best_car.brand), // Car brand escape_csv_field(&best_car.only_name), // Car model - best_car.year, // Car year - best_car.config, // Tune configuration - best_car.time // Track time + best_car.year, // Car year + best_car.config, // Tune configuration + best_car.time // Track time )?; } } @@ -230,16 +245,37 @@ pub fn write_details_csv(output_dir: &str, ctx: &AnalysisContext) -> Result<(), // --------------------------------------------------------------------------- /// Computes the ideal wishlist: very_best unowned cars, capped at max_wishlist, sorted by RQ. +/// +/// A car qualifies only when **one config** reaches `min_count` tracks — counts are +/// NOT summed across different configs. If multiple configs for the same car qualify, +/// the one with the highest count is kept as the representative entry. pub fn compute_ideal_wishlist( counts: &CarCounts, owned: &HashSet, min_count: usize, max_wishlist: usize, ) -> Vec<(BestCarRecord, usize)> { - let mut candidates: Vec<_> = counts.by_rid.values() - .filter(|(record, count)| *count >= min_count && !owned.contains(&record.guid)) - .cloned() - .collect(); + // Track the best qualifying entry per GUID (highest per-config count). + let mut guid_best_entry: HashMap = HashMap::new(); + + for (record, count) in counts.by_rid.values() { + if owned.contains(&record.guid) { + continue; + } + // Filter by per-config count — a single config must reach min_count + if *count < min_count { + continue; + } + // Keep the entry with the highest per-config count (or first on tie). + let entry = guid_best_entry + .entry(record.guid.clone()) + .or_insert_with(|| (record.clone(), *count)); + if count > &entry.1 { + *entry = (record.clone(), *count); + } + } + + let mut candidates: Vec<_> = guid_best_entry.into_values().collect(); candidates.sort_by(|a, b| compare_records(&a.0, &b.0)); candidates.truncate(max_wishlist); candidates @@ -264,21 +300,20 @@ pub fn write_wishlist_files( write_sorted_csv( output_dir, "wishlist.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &ideal, |a, b| compare_records(&a.0, &b.0), format_counted_row, )?; // --- add.csv (ideal cars not yet on the wishlist) --- - let to_add: Vec<_> = ideal.iter() - .filter(|(record, _)| !wishlist.contains(&record.guid)) - .collect(); + let to_add: Vec<_> = + ideal.iter().filter(|(record, _)| !wishlist.contains(&record.guid)).collect(); write_sorted_csv( output_dir, "add.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &to_add, |a, b| compare_records(&a.0, &b.0), |entry, file| format_counted_row(entry, file), @@ -287,11 +322,19 @@ pub fn write_wishlist_files( // --- remove.csv (wishlist cars not in the ideal set) --- // Single pass: collect found entries and count dropped ones together. let mut dropped_count = 0usize; - let to_remove: Vec<_> = wishlist.iter() + let to_remove: Vec<_> = wishlist + .iter() .filter(|guid| !ideal_guids.contains(*guid)) .filter_map(|guid| { - if let Some(rid) = counts.guid_to_rid.get(guid) { - counts.by_rid.get(rid).cloned() + if let Some(keys) = counts.guid_to_rid.get(guid) { + // Pick the entry with the highest count (most impactful config) + keys.iter() + .max_by_key(|key| counts.by_rid.get(key).map(|(_, c)| *c).unwrap_or(0)) + .and_then(|key| counts.by_rid.get(key).cloned()) + .or_else(|| { + dropped_count += 1; + None + }) } else { dropped_count += 1; None @@ -307,7 +350,7 @@ pub fn write_wishlist_files( write_sorted_csv( output_dir, "remove.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &to_remove, |a, b| compare_records(&a.0, &b.0), format_counted_row, @@ -316,6 +359,113 @@ pub fn write_wishlist_files( Ok(()) } +// --------------------------------------------------------------------------- +// CSV output: improve +// --------------------------------------------------------------------------- + +/// Writes improve.csv — owned locked cars whose actual config is not yet a final build +/// (323/233/332), sorted by number of best tracks descending (most impactful upgrades first). +/// +/// Uses the **actual player config** from `my_cars.json` (engineMajor/weightMajor/chassisMajor) +/// rather than the timing data configs. This avoids false positives where a car already at a +/// final build appears in the list because timing data contains non-final configs. +/// +/// The `Target` column shows the normalized final build that the player should upgrade to. +/// The `Count` column shows how many tracks this car is best on (across all its timing configs). +/// +/// Format: RQ,Brand,Model,Year,Target,Count +pub fn write_improve_csv( + output_dir: &str, + counts: &CarCounts, +) -> Result { + /// A car that needs upgrading to a final build, with its target config and track count. + struct ImproveEntry { + brand: String, + only_name: String, + year: u16, + rq: u8, + target: String, + track_count: usize, + } + + // Iterate over owned cars from the player's actual config data. + // For each owned car, check if its actual config is NOT a final build. + let mut entries: Vec = Vec::new(); + + for (guid, actual_config) in &counts.owned_configs { + // Normalize the player's actual config + let target = normalize_to_final_build(actual_config); + + // If normalization didn't change the config, it's already a final build or maxed — skip + if target == *actual_config { + continue; + } + + // Look up the car metadata + let Some(car) = counts.car_map.get(guid) else { + eprintln!( + "Warning: owned car {guid} not found in cars database — skipped for improve" + ); + continue; + }; + + // Count how many tracks this car is best on across ALL its normalized configs. + // This gives a "total impact" metric: upgrading this car benefits all tracks + // where any of its configs are best, not just those matching a single target. + let track_count: usize = counts + .guid_to_rid + .get(guid) + .map(|keys| { + keys.iter() + .filter_map(|key| counts.by_rid.get(key).map(|(_, c)| *c)) + .sum() + }) + .unwrap_or(0); + + entries.push(ImproveEntry { + brand: car.brand.clone(), + only_name: car.only_name.clone(), + year: car.year, + rq: car.rq, + target, + track_count, + }); + } + + let count = entries.len(); + + // Sort by count descending (most tracks first), then by RQ/brand/model + write_sorted_csv( + output_dir, + "improve.csv", + "RQ,Brand,Model,Year,Target,Count", + &entries, + |a, b| { + // Primary: count descending + b.track_count + .cmp(&a.track_count) + .then(a.rq.cmp(&b.rq)) + .then(a.target.cmp(&b.target)) + .then(a.brand.cmp(&b.brand)) + .then(a.only_name.cmp(&b.only_name)) + }, + |entry, file| { + writeln!( + file, + "{},{},{},{},{},{}", + entry.rq, + escape_csv_field(&entry.brand), + escape_csv_field(&entry.only_name), + entry.year, + entry.target, + entry.track_count + ) + }, + )?; + + Ok(count) +} + // --------------------------------------------------------------------------- // Stdout output: track-specific query // --------------------------------------------------------------------------- @@ -331,7 +481,12 @@ pub fn write_wishlist_files( /// * `track` - The track identifier (e.g., "laguna", "monaco") /// * `condition` - Surface condition (e.g., "Dry", "Wet", "Dirt") /// * `rq` - RQ level limit (cars with this RQ or lower will be considered) -pub fn print_best_car(ctx: &AnalysisContext, track: &str, condition: &str, rq: u8) -> Result<(), AppError> { +pub fn print_best_car( + ctx: &AnalysisContext, + track: &str, + condition: &str, + rq: u8, +) -> Result<(), AppError> { let key = (track.to_string(), rq, condition.to_string()); if let Some(best_list) = ctx.best_cars.get(&key) { @@ -348,10 +503,7 @@ pub fn print_best_car(ctx: &AnalysisContext, track: &str, condition: &str, rq: u ); } } else { - println!( - "No best car found for Track: {}, Condition: {}, RQ: {}", - track, condition, rq - ); + println!("No best car found for Track: {}, Condition: {}, RQ: {}", track, condition, rq); } Ok(()) } @@ -364,8 +516,8 @@ pub fn print_best_car(ctx: &AnalysisContext, track: &str, condition: &str, rq: u mod tests { use super::*; use std::collections::HashSet; - use top_drives::{Car, compute_car_counts}; - use top_drives::test_utils::{make_record, make_car}; + use top_drives::test_utils::{make_car, make_record}; + use top_drives::{compute_car_counts, empty_my_data, Car}; // ======================================================================= // compute_ideal_wishlist tests @@ -378,15 +530,13 @@ mod tests { { for i in 0..15 { - ctx.best_cars.insert( - (format!("Track{i}"), 85, "Dry".to_string()), - vec![rec.clone()], - ); + ctx.best_cars + .insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); } } let car_list = vec![make_car("Car_A", 85, "guid_a")]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); let owned: HashSet = HashSet::new(); let wishlist = compute_ideal_wishlist(&counts, &owned, 10, 300); @@ -403,15 +553,13 @@ mod tests { { for i in 0..15 { - ctx.best_cars.insert( - (format!("Track{i}"), 85, "Dry".to_string()), - vec![rec.clone()], - ); + ctx.best_cars + .insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); } } let car_list = vec![make_car("Car_A", 85, "guid_a")]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); let owned: HashSet = vec!["guid_a".to_string()].into_iter().collect(); let wishlist = compute_ideal_wishlist(&counts, &owned, 10, 300); @@ -432,18 +580,15 @@ mod tests { 100.0 + j as f64, &format!("guid_{j}"), ); - ctx.best_cars.insert( - (format!("Track{i}"), 80 + j, "Dry".to_string()), - vec![rec], - ); + ctx.best_cars + .insert((format!("Track{i}"), 80 + j, "Dry".to_string()), vec![rec]); } } } - let car_list: Vec = (0..5u8) - .map(|j| make_car(&format!("Car_{j}"), 80 + j, &format!("guid_{j}"))) - .collect(); - let counts = compute_car_counts(&ctx, car_list); + let car_list: Vec = + (0..5u8).map(|j| make_car(&format!("Car_{j}"), 80 + j, &format!("guid_{j}"))).collect(); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); let owned: HashSet = HashSet::new(); let wishlist = compute_ideal_wishlist(&counts, &owned, 10, 3); @@ -469,7 +614,8 @@ mod tests { &entries, |a, b| a.0.cmp(b.0), |entry, file| writeln!(file, "{},{}", entry.0, entry.1), - ).unwrap(); + ) + .unwrap(); let path = dir.path().join("test.csv"); let content = fs::read_to_string(path).unwrap(); @@ -489,6 +635,7 @@ mod tests { year: 2020, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 100.0, owned: false, prize: false, @@ -502,16 +649,17 @@ mod tests { write_sorted_csv( output_dir, "test.csv", - "RQ,Brand,Model,Year,Count", + "RQ,Brand,Model,Year,FinalBuild,Count", &[entry], |_, _| std::cmp::Ordering::Equal, |e, file| format_counted_row(e, file), - ).unwrap(); + ) + .unwrap(); let path = dir.path().join("test.csv"); let content = fs::read_to_string(path).unwrap(); let lines: Vec<&str> = content.lines().collect(); - assert_eq!(lines[1], "85,\"Brand, Inc.\",\"Model \"\"Sport\"\"\",2020,5"); + assert_eq!(lines[1], "85,\"Brand, Inc.\",\"Model \"\"Sport\"\"\",2020,332,5"); } #[test] @@ -521,10 +669,7 @@ mod tests { // Car_A is best on a track, Car_B is not let rec_a = make_record("Car_A", 70, 100.0, "guid_a"); { - ctx.best_cars.insert( - ("Track1".to_string(), 70, "Dry".to_string()), - vec![rec_a], - ); + ctx.best_cars.insert(("Track1".to_string(), 70, "Dry".to_string()), vec![rec_a]); } // Make Car_D a prize car @@ -538,17 +683,20 @@ mod tests { car_d, // not best, prize car — not trash ]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); let owned: HashSet = vec![ "guid_a".to_string(), "guid_b".to_string(), "guid_c".to_string(), "guid_d".to_string(), - ].into_iter().collect(); + ] + .into_iter() + .collect(); // Test the filtering logic directly (same logic as write_trash_csv) - let trash: Vec<_> = owned.iter() + let trash: Vec<_> = owned + .iter() .filter(|guid| !counts.guid_to_rid.contains_key(*guid)) .filter_map(|guid| counts.car_map.get(guid)) .filter(|car| !car.prize && car.rq < TRASH_MIN_RQ) @@ -579,6 +727,7 @@ mod tests { year: 2020, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 100.0, owned: false, prize: false, @@ -591,6 +740,7 @@ mod tests { year: 2020, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 100.0, owned: false, prize: false, @@ -606,7 +756,7 @@ mod tests { #[test] fn test_empty_analysis_context() { let ctx = AnalysisContext::new(); - let counts = compute_car_counts(&ctx, vec![]); + let counts = compute_car_counts(&ctx, vec![], empty_my_data()); assert!(counts.by_rid.is_empty()); assert!(counts.guid_to_rid.is_empty()); assert!(counts.car_map.is_empty()); @@ -618,16 +768,13 @@ mod tests { let ctx = AnalysisContext::new(); let rec = make_record("Only_Car", 90, 88.5, "guid_only"); - ctx.best_cars.insert( - ("Track1".to_string(), 90, "Dry".to_string()), - vec![rec], - ); + ctx.best_cars.insert(("Track1".to_string(), 90, "Dry".to_string()), vec![rec]); let car = make_car("Only_Car", 90, "guid_only"); - let counts = compute_car_counts(&ctx, vec![car]); + let counts = compute_car_counts(&ctx, vec![car], empty_my_data()); assert_eq!(counts.by_rid.len(), 1); - let (rec, count) = counts.by_rid.get("Only_Car").unwrap(); + let (rec, count) = counts.by_rid.get(&("Only_Car".to_string(), "332".to_string())).unwrap(); assert_eq!(*count, 1); assert_eq!(rec.car_rid, "Only_Car"); } @@ -638,14 +785,11 @@ mod tests { let rec = make_record("Car_A", 85, 100.0, "guid_a"); for i in 0..15 { - ctx.best_cars.insert( - (format!("Track{i}"), 85, "Dry".to_string()), - vec![rec.clone()], - ); + ctx.best_cars.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); } let car = make_car("Car_A", 85, "guid_a"); - let counts = compute_car_counts(&ctx, vec![car]); + let counts = compute_car_counts(&ctx, vec![car], empty_my_data()); let all_owned: HashSet = vec!["guid_a".to_string()].into_iter().collect(); let wishlist = compute_ideal_wishlist(&counts, &all_owned, 10, 300); @@ -658,14 +802,11 @@ mod tests { let rec = make_record("Car_A", 85, 100.0, "guid_a"); for i in 0..15 { - ctx.best_cars.insert( - (format!("Track{i}"), 85, "Dry".to_string()), - vec![rec.clone()], - ); + ctx.best_cars.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); } let car = make_car("Car_A", 85, "guid_a"); - let counts = compute_car_counts(&ctx, vec![car]); + let counts = compute_car_counts(&ctx, vec![car], empty_my_data()); let no_owned: HashSet = HashSet::new(); let wishlist = compute_ideal_wishlist(&counts, &no_owned, 10, 300); diff --git a/src/parsing/car_list.rs b/src/parsing/car_list.rs index 245166e..9ff8cf0 100644 --- a/src/parsing/car_list.rs +++ b/src/parsing/car_list.rs @@ -19,10 +19,8 @@ pub fn get_car_list(cars_db_path: &str) -> Result, AppError> { } })?; - serde_json::from_str(&content).map_err(|e| AppError::Parse { - file: cars_db_path.to_string(), - reason: e.to_string(), - }) + serde_json::from_str(&content) + .map_err(|e| AppError::Parse { file: cars_db_path.to_string(), reason: e.to_string() }) } // --------------------------------------------------------------------------- @@ -39,7 +37,10 @@ mod tests { assert!(result.is_err(), "Should return error for missing file"); match result.unwrap_err() { AppError::MissingFile(msg) => { - assert!(msg.contains("nonexistent_cars_db.json"), "Error should mention the file path"); + assert!( + msg.contains("nonexistent_cars_db.json"), + "Error should mention the file path" + ); } other => panic!("Expected MissingFile error, got: {other}"), } diff --git a/src/parsing/mod.rs b/src/parsing/mod.rs index 9a5fde2..126d27c 100644 --- a/src/parsing/mod.rs +++ b/src/parsing/mod.rs @@ -16,8 +16,8 @@ use tokio::sync::Semaphore; use tokio::task; use top_drives::{ - AppError, AnalysisContext, BestCarRecord, Car, TripletKey, - translate_track_name, translate_track_type, + normalize_to_final_build, translate_track_name, translate_track_type, AnalysisContext, + AppError, BestCarRecord, Car, TripletKey, }; use self::car_list::get_car_list; @@ -90,16 +90,22 @@ pub fn parse_car_times( let json_data: Value = match serde_json::from_str(&file_content) { Ok(v) => v, Err(e) => { - return Err(AppError::Parse { - file: local_file_path, - reason: e.to_string(), - }); + return Err(AppError::Parse { file: local_file_path, reason: e.to_string() }); } }; // Process each configuration variant (111, 233, 323, etc.) if let Some(configurations) = json_data["data"].as_object() { - for (config, entry) in configurations { + for (raw_config, entry) in configurations { + // Strip version prefix (e.g., "v2c_" from "v2c_233") at read time. + // Downstream code only ever sees bare digit strings. + // Use explicit prefix matching to avoid mis-stripping keys that + // naturally contain underscores. + let config = raw_config + .strip_prefix("v2c_") + .or_else(|| raw_config.strip_prefix("v1c_")) + .unwrap_or(raw_config) + .to_string(); // Process timing data for each track if let Some(times) = entry["times"].as_object() { for (track_id, details) in times { @@ -129,6 +135,7 @@ pub fn parse_car_times( let track_name = translate_track_name(cleaned_track_id); let surface = translate_track_type(track_type); + let normalized = normalize_to_final_build(&config); update_best_car( best_cars, &track_name, @@ -141,6 +148,7 @@ pub fn parse_car_times( year: car.year, rq: car.rq, config: config.clone(), + normalized_config: normalized, time, owned, prize: car.prize, @@ -201,9 +209,10 @@ pub fn update_best_car( *existing_list = vec![new_record.clone()]; } } else if (new_record.time - best_time).abs() < TIME_TOLERANCE - && !existing_list - .iter() - .any(|r| r.car_rid == new_record.car_rid) + && !existing_list.iter().any(|r| { + r.car_rid == new_record.car_rid + && r.normalized_config == new_record.normalized_config + }) { if is_last { existing_list.push(new_record); @@ -264,9 +273,11 @@ pub async fn parse_all_times( Err(_) => return Err(AppError::Internal("Semaphore closed".into())), }; // Run the synchronous parsing on a blocking thread while holding the permit. - task::spawn_blocking(move || { - parse_car_times(car, &my_cars, &best_cars, ×_dir) - }).await.unwrap_or_else(|e| Err(AppError::Internal(format!("Blocking task join error: {e}")))) + task::spawn_blocking(move || parse_car_times(car, &my_cars, &best_cars, ×_dir)) + .await + .unwrap_or_else(|e| { + Err(AppError::Internal(format!("Blocking task join error: {e}"))) + }) }); handles.push(handle); } @@ -297,7 +308,9 @@ pub async fn parse_all_times( if parse_errors == car_list.len() && !car_list.is_empty() { return Err(AppError::Parse { file: times_dir.to_string(), - reason: format!("All {parse_errors} car(s) failed to parse — data directory may be corrupted"), + reason: format!( + "All {parse_errors} car(s) failed to parse — data directory may be corrupted" + ), }); } @@ -319,7 +332,10 @@ pub async fn fetch_times(cars_db_path: &str, times_dir: &str) -> Result<(), AppE let mut handles = vec![]; for car in get_car_list(cars_db_path)? { - let permit = semaphore.clone().acquire_owned().await + let permit = semaphore + .clone() + .acquire_owned() + .await .map_err(|e| AppError::Network(format!("Semaphore closed: {e}")))?; let times_dir = times_dir.to_string(); handles.push(tokio::spawn(async move { @@ -335,12 +351,16 @@ pub async fn fetch_times(cars_db_path: &str, times_dir: &str) -> Result<(), AppE Ok(Ok(())) => {} Ok(Err(e)) => { eprintln!("{e}"); - if first_error.is_none() { first_error = Some(e); } + if first_error.is_none() { + first_error = Some(e); + } } Err(e) => { let msg = AppError::Network(format!("Task join error: {e}")); eprintln!("{msg}"); - if first_error.is_none() { first_error = Some(msg); } + if first_error.is_none() { + first_error = Some(msg); + } } } } @@ -439,10 +459,7 @@ mod tests { for rq in 85..=MAX_RQ { let key = ("laguna".to_string(), rq, "Dry".to_string()); - assert!( - ctx.best_cars.contains_key(&key), - "Car should appear at RQ level {rq}" - ); + assert!(ctx.best_cars.contains_key(&key), "Car should appear at RQ level {rq}"); } // Should NOT appear below its RQ let key_below = ("laguna".to_string(), 84, "Dry".to_string()); @@ -575,10 +592,7 @@ mod tests { parse_car_times(car, &my_cars, &ctx.best_cars, times_dir).unwrap(); // Test Bowl time should be inverted: 2000.0 / 50.0 = 40.0 - let records = ctx - .best_cars - .get(&("Test Bowl".to_string(), 85, "Dry".to_string())) - .unwrap(); + let records = ctx.best_cars.get(&("Test Bowl".to_string(), 85, "Dry".to_string())).unwrap(); assert_eq!(records[0].time, 40.0); } diff --git a/src/records.rs b/src/records.rs index 48a6f8c..626fa33 100644 --- a/src/records.rs +++ b/src/records.rs @@ -127,11 +127,9 @@ enum Commands { /// Generate details.csv with all timing data Details, /// Show best car for a specific track/condition/RQ combination - Track { - track: String, - condition: String, - rq: u8, - }, + Track { track: String, condition: String, rq: u8 }, + /// Generate improve.csv (owned cars not in final config, sorted by best tracks) + Improve, } // --------------------------------------------------------------------------- @@ -170,29 +168,70 @@ async fn run() -> Result<(), AppError> { commands::fetch::run_fetch(&cli.cars_db, &cli.tracks_db, &cli.times_dir).await?; } Commands::Best { min_count } => { - commands::best::run_best(&cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir, min_count).await?; + commands::best::run_best( + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + min_count, + ) + .await?; } Commands::Trash => { - commands::trash::run_trash(&cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir).await?; + commands::trash::run_trash(&cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir) + .await?; } Commands::Wishlist { min_count, max_wishlist } => { commands::wishlist::run_wishlist( - &cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir, min_count, max_wishlist, - ).await?; + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + min_count, + max_wishlist, + ) + .await?; } Commands::All { min_count, max_wishlist } => { commands::all::run_all( - &cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir, min_count, max_wishlist, - ).await?; + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + min_count, + max_wishlist, + ) + .await?; } Commands::Details => { - commands::details::run_details(&cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir).await?; + commands::details::run_details( + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + ) + .await?; } Commands::Track { track, condition, rq } => { commands::track::run_track( - &cli.my_cars, &cli.cars_db, &cli.times_dir, &cli.output_dir, - &track, &condition, rq, - ).await?; + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + &track, + &condition, + rq, + ) + .await?; + } + Commands::Improve => { + commands::improve::run_improve( + &cli.my_cars, + &cli.cars_db, + &cli.times_dir, + &cli.output_dir, + ) + .await?; } } Ok(()) diff --git a/src/test_utils.rs b/src/test_utils.rs index c75e87f..edb2d14 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -3,6 +3,8 @@ //! Provides helper constructors for [`BestCarRecord`] and [`Car`] to avoid //! duplication across test modules. +use std::collections::HashMap; + use crate::{BestCarRecord, Car}; /// Creates a test [`BestCarRecord`] with sensible defaults. @@ -17,6 +19,7 @@ pub fn make_record(car_rid: &str, rq: u8, time: f64, guid: &str) -> BestCarRecor year: 2020, rq, config: "111".to_string(), + normalized_config: "332".to_string(), time, owned: false, prize: false, @@ -39,3 +42,8 @@ pub fn make_car(rid: &str, rq: u8, guid: &str) -> Car { guid: guid.to_string(), } } + +/// Creates an empty owned configs map for tests that don't need ownership data. +pub fn empty_my_data() -> HashMap { + HashMap::new() +} diff --git a/src/translate.rs b/src/translate.rs index db4bdbd..9b36f78 100644 --- a/src/translate.rs +++ b/src/translate.rs @@ -89,9 +89,7 @@ pub fn translate_track_name(track_code: &str) -> String { raw }); - map.get(track_code) - .cloned() - .unwrap_or_else(|| track_code.to_string()) + map.get(track_code).cloned().unwrap_or_else(|| track_code.to_string()) } #[cfg(test)] @@ -145,7 +143,11 @@ mod tests { for condition in test_conditions { let name = translate_track_type(condition); - assert_ne!(name, condition, "Condition {} should have a proper name mapping", condition); + assert_ne!( + name, condition, + "Condition {} should have a proper name mapping", + condition + ); assert!(!name.is_empty(), "Condition name should not be empty"); } } diff --git a/src/types.rs b/src/types.rs index 8d298a7..2027b8c 100644 --- a/src/types.rs +++ b/src/types.rs @@ -74,8 +74,12 @@ pub struct BestCarRecord { pub year: u16, /// Rarity level of this specific car configuration pub rq: u8, - /// Tune configuration string (e.g., "111", "233", "323") + /// Original tune configuration string from JSON (e.g., "111", "221", "233") pub config: String, + /// Normalized config mapped to the closest final build ("323", "233", or "332"). + /// Final builds are permutations of digits summing to 8. Non-standard configs + /// are normalized by repeatedly incrementing the smallest digit until the sum is 8. + pub normalized_config: String, /// Track time in seconds (lower is better) pub time: f64, /// Whether the current user owns this car @@ -89,3 +93,59 @@ pub struct BestCarRecord { /// Type alias for the composite key used to identify unique track conditions. /// Consists of: (Track ID, RQ Level, Surface Condition) pub type TripletKey = (String, u8, String); + +/// Normalizes a tune configuration to the closest "final build". +/// +/// The three final builds are "323", "233", and "332" (all permutations of digits +/// summing to 8). For any non-standard config, this function repeatedly increments +/// the smallest digit until the digit sum reaches 8, producing one of the three +/// final builds. +/// +/// Expects bare digit strings (e.g., `"233"`, `"111"`). Version prefixes (e.g., +/// `"v2c_233"`) are stripped at read time by the parsing layer before reaching +/// this function. +/// +/// If the config cannot be parsed (wrong length, non-digit characters, or digit +/// sum exceeding 8), the original config string is returned unchanged. +/// +/// # Examples +/// +/// ``` +/// use top_drives::normalize_to_final_build; +/// assert_eq!(normalize_to_final_build("323"), "323"); +/// assert_eq!(normalize_to_final_build("111"), "332"); +/// assert_eq!(normalize_to_final_build("221"), "332"); +/// assert_eq!(normalize_to_final_build("122"), "332"); +/// assert_eq!(normalize_to_final_build("113"), "323"); +/// assert_eq!(normalize_to_final_build("233"), "233"); +/// assert_eq!(normalize_to_final_build("332"), "332"); +/// assert_eq!(normalize_to_final_build("879"), "879"); // maxed: returned as-is +/// ``` +pub fn normalize_to_final_build(config: &str) -> String { + let bytes = config.as_bytes(); + if bytes.len() != 3 || !bytes.iter().all(|&b| b.is_ascii_digit()) { + return config.to_string(); + } + + let mut digits = [0u8; 3]; + for (i, &b) in bytes.iter().enumerate() { + digits[i] = b - b'0'; + } + + let sum: i32 = digits.iter().map(|&d| d as i32).sum(); + if sum >= 8 { + // Already a final build (sum == 8) or maxed (sum > 8) — return as-is. + return config.to_string(); + } + + let mut deficit = 8 - sum; + while deficit > 0 { + // Find the position of the smallest digit (first occurrence on tie) + let min_pos = (0..3).min_by_key(|&i| digits[i]).unwrap(); + digits[min_pos] += 1; + deficit -= 1; + } + + digits.iter().map(|&d| (b'0' + d) as char).collect() +} + diff --git a/src/user_data.rs b/src/user_data.rs index f061126..89a852b 100644 --- a/src/user_data.rs +++ b/src/user_data.rs @@ -3,7 +3,7 @@ //! Handles parsing the game export file to extract owned car GUIDs //! and wishlist entries. -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::fs::File; use std::io::BufReader; @@ -21,6 +21,10 @@ pub struct MyCarsData { pub owned: HashSet, /// GUIDs of wishlisted cars (from `wishlists[0].cards`) pub wishlist: HashSet, + /// Actual tune config for each owned car (guid → "{engineMajor}{weightMajor}{chassisMajor}"). + /// Only populated for locked cards. Used by the improve command to determine + /// which cars are not yet at a final build. + pub owned_configs: HashMap, } /// Loads and parses `my_cars.json` once, extracting both owned and wishlist data. @@ -49,22 +53,33 @@ pub fn load_my_cars_data(path: &str) -> Result { let json: Value = serde_json::from_reader(reader) .map_err(|e| AppError::Parse { file: path.to_string(), reason: e.to_string() })?; - // Extract owned car GUIDs from playerDeck - let owned: HashSet = json - .get("playerDeck") - .and_then(|d| d.as_array()) - .map(|deck| { - deck.iter() - .filter_map(|card| { - if card.get("locked").and_then(|v| v.as_bool()).unwrap_or(false) { - card.get("cardId").and_then(|v| v.as_str()).map(|s| s.to_string()) + // Extract owned car GUIDs and their actual configs from playerDeck + let mut owned: HashSet = HashSet::new(); + let mut owned_configs: HashMap = HashMap::new(); + + if let Some(deck) = json.get("playerDeck").and_then(|d| d.as_array()) { + for card in deck { + if card.get("locked").and_then(|v| v.as_bool()).unwrap_or(false) { + if let Some(guid) = card.get("cardId").and_then(|v| v.as_str()) { + owned.insert(guid.to_string()); + // Extract actual tune config: engineMajor + weightMajor + chassisMajor. + // Skip cards with incomplete tune data to avoid false "000" entries + // that would incorrectly appear as needing upgrade. + let engine = card.get("engineMajor").and_then(|v| v.as_u64()); + let weight = card.get("weightMajor").and_then(|v| v.as_u64()); + let chassis = card.get("chassisMajor").and_then(|v| v.as_u64()); + if let (Some(e), Some(w), Some(c)) = (engine, weight, chassis) { + let config = format!("{e}{w}{c}"); + owned_configs.insert(guid.to_string(), config); } else { - None + eprintln!( + "Warning: locked card {guid} has incomplete tune data — skipped" + ); } - }) - .collect() - }) - .unwrap_or_default(); + } + } + } + } // Extract wishlist GUIDs from wishlists[0].cards let wishlist: HashSet = json @@ -72,15 +87,10 @@ pub fn load_my_cars_data(path: &str) -> Result { .and_then(|w| w.get(0)) .and_then(|w| w.get("cards")) .and_then(|c| c.as_array()) - .map(|cards| { - cards - .iter() - .filter_map(|c| c.as_str().map(|s| s.to_string())) - .collect() - }) + .map(|cards| cards.iter().filter_map(|c| c.as_str().map(|s| s.to_string())).collect()) .unwrap_or_default(); - Ok(MyCarsData { owned, wishlist }) + Ok(MyCarsData { owned, wishlist, owned_configs }) } #[cfg(test)] diff --git a/tests/http_tests.rs b/tests/http_tests.rs index c18f644..2ac3eef 100644 --- a/tests/http_tests.rs +++ b/tests/http_tests.rs @@ -2,22 +2,17 @@ use std::fs; -use mockito::{Server, ServerGuard, Mock}; +use mockito::{Mock, Server, ServerGuard}; use top_drives::get_file; /// Helper: create a mock that returns a successful response with the given body. fn mock_success(server: &mut ServerGuard, body: &str) -> Mock { - server.mock("GET", "/data.json") - .with_status(200) - .with_body(body) - .create() + server.mock("GET", "/data.json").with_status(200).with_body(body).create() } /// Helper: create a mock that returns the given HTTP status. fn mock_status(server: &mut ServerGuard, status: usize) -> Mock { - server.mock("GET", "/data.json") - .with_status(status) - .create() + server.mock("GET", "/data.json").with_status(status).create() } #[tokio::test] @@ -39,7 +34,8 @@ async fn test_get_file_success() { async fn test_get_file_empty_response_retries_and_fails() { let mut server = Server::new_async().await; // Return 200 with empty body — triggers retry, then fails - let m = server.mock("GET", "/data.json") + let m = server + .mock("GET", "/data.json") .with_status(200) .with_body("") .expect(4) // 1 initial + 3 retries @@ -58,10 +54,7 @@ async fn test_get_file_empty_response_retries_and_fails() { async fn test_get_file_server_error_retries() { let mut server = Server::new_async().await; // Return 500 three times, then 200 - let m_fail = server.mock("GET", "/data.json") - .with_status(500) - .expect(3) - .create(); + let m_fail = server.mock("GET", "/data.json").with_status(500).expect(3).create(); let m_ok = mock_success(&mut server, "recovered"); let dir = tempfile::tempdir().unwrap(); @@ -96,7 +89,8 @@ async fn test_get_file_client_error_fails_immediately() { async fn test_get_file_persistent_server_error_fails() { let mut server = Server::new_async().await; // 503 every time — should retry 3 times then fail - let m = server.mock("GET", "/data.json") + let m = server + .mock("GET", "/data.json") .with_status(503) .expect(4) // 1 initial + 3 retries .create(); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index b057abd..8ed8184 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -6,12 +6,11 @@ use std::fs; use std::io::Write; +use top_drives::test_utils::{make_car, make_record}; use top_drives::{ - AnalysisContext, BestCarRecord, Car, - compare_cars, compute_car_counts, escape_csv_field, load_my_cars_data, - translate_track_name, translate_track_type, + compare_cars, compute_car_counts, empty_my_data, escape_csv_field, load_my_cars_data, + translate_track_name, translate_track_type, AnalysisContext, BestCarRecord, Car, }; -use top_drives::test_utils::{make_record, make_car}; // ======================================================================= // Integration: AnalysisContext → CarCounts pipeline @@ -21,7 +20,7 @@ use top_drives::test_utils::{make_record, make_car}; fn test_full_pipeline_empty_input() { let ctx = AnalysisContext::new(); let car_list: Vec = vec![]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); assert!(counts.by_rid.is_empty()); assert!(counts.guid_to_rid.is_empty()); @@ -35,25 +34,21 @@ fn test_full_pipeline_single_car_multiple_tracks() { // Simulate the car being best on 20 tracks for i in 0..20 { - ctx.best_cars.insert( - (format!("Track{i}"), 95, "Dry".to_string()), - vec![rec.clone()], - ); + ctx.best_cars.insert((format!("Track{i}"), 95, "Dry".to_string()), vec![rec.clone()]); } let car = make_car("Ferrari_F40", 95, "guid_f40"); - let counts = compute_car_counts(&ctx, vec![car]); + let counts = compute_car_counts(&ctx, vec![car], empty_my_data()); assert_eq!(counts.by_rid.len(), 1); - let (rec, count) = counts.by_rid.get("Ferrari_F40").unwrap(); + let (rec, count) = counts.by_rid.get(&("Ferrari_F40".to_string(), "332".to_string())).unwrap(); assert_eq!(*count, 20); assert_eq!(rec.time, 88.5); // Verify guid_to_rid mapping - assert_eq!( - counts.guid_to_rid.get("guid_f40"), - Some(&"Ferrari_F40".to_string()) - ); + let guid_entries = counts.guid_to_rid.get("guid_f40").unwrap(); + assert_eq!(guid_entries.len(), 1); + assert!(guid_entries.contains(&("Ferrari_F40".to_string(), "332".to_string()))); // Verify car_map assert!(counts.car_map.contains_key("guid_f40")); @@ -66,28 +61,19 @@ fn test_full_pipeline_multiple_cars_different_rq() { // Car A: RQ 80, best on 5 tracks let rec_a = make_record("Car_A", 80, 100.0, "guid_a"); for i in 0..5 { - ctx.best_cars.insert( - (format!("Track{i}"), 80, "Dry".to_string()), - vec![rec_a.clone()], - ); + ctx.best_cars.insert((format!("Track{i}"), 80, "Dry".to_string()), vec![rec_a.clone()]); } // Car B: RQ 90, best on 12 tracks let rec_b = make_record("Car_B", 90, 95.0, "guid_b"); for i in 0..12 { - ctx.best_cars.insert( - (format!("TrackB{i}"), 90, "Dry".to_string()), - vec![rec_b.clone()], - ); + ctx.best_cars.insert((format!("TrackB{i}"), 90, "Dry".to_string()), vec![rec_b.clone()]); } // Car C: RQ 100, best on 8 tracks let rec_c = make_record("Car_C", 100, 85.0, "guid_c"); for i in 0..8 { - ctx.best_cars.insert( - (format!("TrackC{i}"), 100, "Dry".to_string()), - vec![rec_c.clone()], - ); + ctx.best_cars.insert((format!("TrackC{i}"), 100, "Dry".to_string()), vec![rec_c.clone()]); } let car_list = vec![ @@ -96,17 +82,17 @@ fn test_full_pipeline_multiple_cars_different_rq() { make_car("Car_C", 100, "guid_c"), ]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); assert_eq!(counts.by_rid.len(), 3); - let (_, count_a) = counts.by_rid.get("Car_A").unwrap(); + let (_, count_a) = counts.by_rid.get(&("Car_A".to_string(), "332".to_string())).unwrap(); assert_eq!(*count_a, 5); - let (_, count_b) = counts.by_rid.get("Car_B").unwrap(); + let (_, count_b) = counts.by_rid.get(&("Car_B".to_string(), "332".to_string())).unwrap(); assert_eq!(*count_b, 12); - let (_, count_c) = counts.by_rid.get("Car_C").unwrap(); + let (_, count_c) = counts.by_rid.get(&("Car_C".to_string(), "332".to_string())).unwrap(); assert_eq!(*count_c, 8); } @@ -127,6 +113,7 @@ fn test_csv_output_field_ordering() { year: 2023, rq: 85, config: "111".to_string(), + normalized_config: "332".to_string(), time: 100.0, owned: true, prize: false, @@ -137,15 +124,16 @@ fn test_csv_output_field_ordering() { // Write with the standard format { let mut file = fs::File::create(path_str).unwrap(); - writeln!(file, "RQ,Brand,Model,Year,Count").unwrap(); + writeln!(file, "RQ,Brand,Model,Year,FinalBuild,Count").unwrap(); for (rec, count) in &entries { writeln!( file, - "{},{},{},{},{}", + "{},{},{},{},{},{}", rec.rq, escape_csv_field(&rec.brand), escape_csv_field(&rec.only_name), rec.year, + rec.normalized_config, count ) .unwrap(); @@ -154,8 +142,8 @@ fn test_csv_output_field_ordering() { let content = fs::read_to_string(&path).unwrap(); let lines: Vec<&str> = content.lines().collect(); - assert_eq!(lines[0], "RQ,Brand,Model,Year,Count"); - assert_eq!(lines[1], "85,BMW,M3,2023,7"); + assert_eq!(lines[0], "RQ,Brand,Model,Year,FinalBuild,Count"); + assert_eq!(lines[1], "85,BMW,M3,2023,332,7"); } #[test] @@ -193,13 +181,8 @@ fn test_track_translation_pipeline() { #[test] fn test_condition_translation_pipeline() { - let conditions = vec![ - ("00", "Dry"), - ("01", "Wet"), - ("10", "Dirt"), - ("11", "Dirt Wet"), - ("60", "Snow"), - ]; + let conditions = + vec![("00", "Dry"), ("01", "Wet"), ("10", "Dirt"), ("11", "Dirt Wet"), ("60", "Snow")]; for (code, expected) in conditions { assert_eq!(translate_track_type(code), expected); @@ -222,31 +205,25 @@ fn test_ownership_filtering_in_counts() { let rec_unowned = make_record("Unowned_Car", 90, 95.0, "guid_unowned"); for i in 0..10 { - ctx.best_cars.insert( - (format!("Track{i}"), 85, "Dry".to_string()), - vec![rec_owned.clone()], - ); - ctx.best_cars.insert( - (format!("Track{i}"), 90, "Dry".to_string()), - vec![rec_unowned.clone()], - ); + ctx.best_cars.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec_owned.clone()]); + ctx.best_cars + .insert((format!("Track{i}"), 90, "Dry".to_string()), vec![rec_unowned.clone()]); } - let car_list = vec![ - make_car("Owned_Car", 85, "guid_owned"), - make_car("Unowned_Car", 90, "guid_unowned"), - ]; + let car_list = + vec![make_car("Owned_Car", 85, "guid_owned"), make_car("Unowned_Car", 90, "guid_unowned")]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); // Both should be in by_rid assert_eq!(counts.by_rid.len(), 2); // Verify ownership is reflected in the records - let (owned_rec, _) = counts.by_rid.get("Owned_Car").unwrap(); + let (owned_rec, _) = counts.by_rid.get(&("Owned_Car".to_string(), "332".to_string())).unwrap(); assert!(owned_rec.owned); - let (unowned_rec, _) = counts.by_rid.get("Unowned_Car").unwrap(); + let (unowned_rec, _) = + counts.by_rid.get(&("Unowned_Car".to_string(), "332".to_string())).unwrap(); assert!(!unowned_rec.owned); } @@ -261,9 +238,9 @@ fn test_load_my_cars_data_with_fixture() { let json = r#"{ "playerDeck": [ - { "cardId": "guid_a", "locked": true }, - { "cardId": "guid_b", "locked": false }, - { "cardId": "guid_c", "locked": true } + { "cardId": "guid_a", "locked": true, "engineMajor": 3, "weightMajor": 2, "chassisMajor": 3 }, + { "cardId": "guid_b", "locked": false, "engineMajor": 2, "weightMajor": 3, "chassisMajor": 3 }, + { "cardId": "guid_c", "locked": true, "engineMajor": 1, "weightMajor": 1, "chassisMajor": 1 } ], "wishlists": [ { "cards": ["guid_d", "guid_e"] } @@ -279,12 +256,45 @@ fn test_load_my_cars_data_with_fixture() { assert!(data.owned.contains("guid_c")); assert!(!data.owned.contains("guid_b")); + // owned_configs only for locked cards with complete tune data + assert_eq!(data.owned_configs.len(), 2); + assert_eq!(data.owned_configs.get("guid_a"), Some(&"323".to_string())); + assert_eq!(data.owned_configs.get("guid_c"), Some(&"111".to_string())); + // Unlocked card should not be in owned_configs + assert!(!data.owned_configs.contains_key("guid_b")); + // Wishlist assert_eq!(data.wishlist.len(), 2); assert!(data.wishlist.contains("guid_d")); assert!(data.wishlist.contains("guid_e")); } +#[test] +fn test_load_my_cars_data_incomplete_tune_data() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("my_cars.json"); + + let json = r#"{ + "playerDeck": [ + { "cardId": "guid_full", "locked": true, "engineMajor": 2, "weightMajor": 3, "chassisMajor": 3 }, + { "cardId": "guid_partial", "locked": true, "engineMajor": 2, "weightMajor": 3 }, + { "cardId": "guid_empty", "locked": true } + ] + }"#; + fs::write(&path, json).unwrap(); + + let data = load_my_cars_data(path.to_str().unwrap()).unwrap(); + + // All locked cards are owned + assert_eq!(data.owned.len(), 3); + + // Only the card with complete tune data has a config entry + assert_eq!(data.owned_configs.len(), 1); + assert_eq!(data.owned_configs.get("guid_full"), Some(&"233".to_string())); + assert!(!data.owned_configs.contains_key("guid_partial")); + assert!(!data.owned_configs.contains_key("guid_empty")); +} + #[test] fn test_load_my_cars_data_empty_wishlist() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/proptest_tests.rs b/tests/proptest_tests.rs index 76efb34..b6965fb 100644 --- a/tests/proptest_tests.rs +++ b/tests/proptest_tests.rs @@ -2,19 +2,19 @@ use proptest::prelude::*; -use top_drives::{compare_cars, compute_car_counts, AnalysisContext, Car}; use top_drives::test_utils::{make_car, make_record}; +use top_drives::{compare_cars, compute_car_counts, empty_my_data, AnalysisContext, Car}; /// Generate arbitrary Car instances for property testing. fn car_strategy() -> impl Strategy { ( - "[a-zA-Z]{2,10}", // rid - "[a-zA-Z]{2,15}", // brand - "[a-zA-Z0-9 ]{2,20}", // only_name - 1950u16..2030, // year - 10u8..113, // rq - any::(), // prize - "[a-zA-Z0-9_]{3,20}", // guid + "[a-zA-Z]{2,10}", // rid + "[a-zA-Z]{2,15}", // brand + "[a-zA-Z0-9 ]{2,20}", // only_name + 1950u16..2030, // year + 10u8..113, // rq + any::(), // prize + "[a-zA-Z0-9_]{3,20}", // guid ) .prop_map(|(rid, brand, only_name, year, rq, prize, guid)| Car { rid, @@ -177,7 +177,7 @@ proptest! { cars in proptest::collection::vec(car_strategy(), 0..20) ) { let ctx = AnalysisContext::default(); - let counts = compute_car_counts(&ctx, cars.clone()); + let counts = compute_car_counts(&ctx, cars.clone(), empty_my_data()); // No best_cars entries → every car has 0 wins → by_rid is empty assert_eq!(counts.by_rid.len(), 0); // guid_to_rid is also empty since no car won any track @@ -212,9 +212,9 @@ proptest! { } let car_list = vec![make_car(&rid, rq, &guid)]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); - let entry = counts.by_rid.get(&rid); + let entry = counts.by_rid.get(&(rid.clone(), "332".to_string())); assert!(entry.is_some(), "car should appear in by_rid"); let (_, count) = entry.unwrap(); assert_eq!(*count, n_tracks, "count should match number of tracks"); @@ -240,9 +240,9 @@ proptest! { ); let car_list = vec![make_car(&rid, rq, &guid)]; - let counts = compute_car_counts(&ctx, car_list); + let counts = compute_car_counts(&ctx, car_list, empty_my_data()); - let (record, _) = counts.by_rid.get(&rid).unwrap(); + let (record, _) = counts.by_rid.get(&(rid, "332".to_string())).unwrap(); assert_eq!(record.owned, owned, "owned flag should propagate"); } }