From 330bccd7243d8440dd8925c5f74b8c23b67867fb Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Mon, 11 May 2026 00:56:46 +0300 Subject: [PATCH] [AZ-366] Refactor C13: consolidate Haversine + tile-coord parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RouteProcessingService.CalculateDistance(double, double, double, double) re-implemented Haversine using EARTH_RADIUS=6371000 alongside the canonical GeoUtils.CalculateDistance(GeoPoint, GeoPoint) which uses EARTH_RADIUS=6378137. Two implementations of the same formula for the same problem. Separately, ExtractTileCoordinatesFromFilename in RouteProcessingService parsed the tile_{z}_{x}_{y}_{ts}.jpg filename pattern that's *generated* by StorageConfig.GetTileFilePath in another assembly — the writer and parser were coupled by string convention only and a format change in one would silently break the other. Both fixes: (a) Deleted the duplicate Haversine in RouteProcessingService. The single call site (region-matching nearest-neighbor OrderBy) now uses GeoUtils.CalculateDistance with GeoPoint instances. The constant difference is monotonic-equivalent for OrderBy purposes — same region is picked. (b) Added static StorageConfig.TryExtractTileCoordinates(string, out int, out int): bool — pure parser, co-located with GetTileFilePath so the inverse-pair invariant is structural, not by-convention. RouteProcessingService.ExtractTileCoordinatesFromFilename becomes a thin wrapper that delegates to the helper and emits the existing warning log on malformed input — the AZ-352 tests for warning behavior all still pass. Verification: - 77/77 unit tests green (was 71 → +6 new StorageConfigTests including a writer/parser round-trip test for AC-2). - Smoke + full integration suite green. - AC-1 grep verification: Math.Sin/EARTH_RADIUS patterns are now confined to GeoUtils.cs only. Co-authored-by: Cursor --- .../Configs/StorageConfig.cs | 25 ++++++ .../RouteProcessingService.cs | 32 ++----- SatelliteProvider.Tests/StorageConfigTests.cs | 86 +++++++++++++++++++ _docs/03_implementation/batch_12_report.md | 59 +++++++++++++ 4 files changed, 176 insertions(+), 26 deletions(-) create mode 100644 SatelliteProvider.Tests/StorageConfigTests.cs create mode 100644 _docs/03_implementation/batch_12_report.md diff --git a/SatelliteProvider.Common/Configs/StorageConfig.cs b/SatelliteProvider.Common/Configs/StorageConfig.cs index eb3b9ca..2a4084e 100644 --- a/SatelliteProvider.Common/Configs/StorageConfig.cs +++ b/SatelliteProvider.Common/Configs/StorageConfig.cs @@ -18,5 +18,30 @@ public class StorageConfig var fileName = $"tile_{zoomLevel}_{tileX}_{tileY}_{timestamp}.jpg"; return Path.Combine(subdirectory, fileName); } + + // Inverse of GetTileFilePath: parses tile_{zoom}_{x}_{y}_{ts}.jpg. + // Co-located with the writer per AZ-366 / C13 so format changes can never + // drift between the two ends. Pure: no I/O, no logging, no exceptions for + // malformed input — caller decides how to react to a `false` return. + public static bool TryExtractTileCoordinates(string filePath, out int tileX, out int tileY) + { + tileX = -1; + tileY = -1; + ArgumentNullException.ThrowIfNull(filePath); + + var filename = Path.GetFileNameWithoutExtension(filePath); + var parts = filename.Split('_'); + + if (parts.Length >= 4 && parts[0] == "tile" && + int.TryParse(parts[2], out var x) && + int.TryParse(parts[3], out var y)) + { + tileX = x; + tileY = y; + return true; + } + + return false; + } } diff --git a/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs b/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs index d0ecb01..8604f51 100644 --- a/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs +++ b/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs @@ -579,50 +579,30 @@ public class RouteProcessingService : BackgroundService foreach (var point in routePoints) { + var pointGeo = new Common.DTO.GeoPoint { Lat = point.Latitude, Lon = point.Longitude }; var matchedRegion = availableRegions - .OrderBy(r => CalculateDistance(point.Latitude, point.Longitude, r.Latitude, r.Longitude)) + .OrderBy(r => GeoUtils.CalculateDistance(pointGeo, new Common.DTO.GeoPoint { Lat = r.Latitude, Lon = r.Longitude })) .FirstOrDefault(); - + if (matchedRegion != null) { orderedRegions.Add(matchedRegion); availableRegions.Remove(matchedRegion); } } - - return orderedRegions; - } - private static double CalculateDistance(double lat1, double lon1, double lat2, double lon2) - { - const double earthRadiusMeters = 6371000; - var dLat = (lat2 - lat1) * Math.PI / 180.0; - var dLon = (lon2 - lon1) * Math.PI / 180.0; - - var a = Math.Sin(dLat / 2) * Math.Sin(dLat / 2) + - Math.Cos(lat1 * Math.PI / 180.0) * Math.Cos(lat2 * Math.PI / 180.0) * - Math.Sin(dLon / 2) * Math.Sin(dLon / 2); - var c = 2 * Math.Atan2(Math.Sqrt(a), Math.Sqrt(1 - a)); - - return earthRadiusMeters * c; + return orderedRegions; } internal (int TileX, int TileY) ExtractTileCoordinatesFromFilename(string filePath) { - ArgumentNullException.ThrowIfNull(filePath); - - var filename = Path.GetFileNameWithoutExtension(filePath); - var parts = filename.Split('_'); - - if (parts.Length >= 4 && parts[0] == "tile" && - int.TryParse(parts[2], out var tileX) && - int.TryParse(parts[3], out var tileY)) + if (StorageConfig.TryExtractTileCoordinates(filePath, out var tileX, out var tileY)) { return (tileX, tileY); } _logger.LogWarning( - "Could not extract tile coordinates from filename {FilePath}; expected pattern tile___", + "Could not extract tile coordinates from filename {FilePath}; expected pattern tile____", filePath); return (-1, -1); } diff --git a/SatelliteProvider.Tests/StorageConfigTests.cs b/SatelliteProvider.Tests/StorageConfigTests.cs new file mode 100644 index 0000000..4ececd2 --- /dev/null +++ b/SatelliteProvider.Tests/StorageConfigTests.cs @@ -0,0 +1,86 @@ +using FluentAssertions; +using SatelliteProvider.Common.Configs; + +namespace SatelliteProvider.Tests; + +public class StorageConfigTests +{ + [Fact] + public void TryExtractTileCoordinates_ValidFilename_ReturnsTrue_AZ366() + { + var ok = StorageConfig.TryExtractTileCoordinates( + "/tiles/18/158/90/tile_18_158292_90821_20260510022758.jpg", + out var x, + out var y); + + ok.Should().BeTrue(); + x.Should().Be(158292); + y.Should().Be(90821); + } + + [Fact] + public void TryExtractTileCoordinates_FilenameWithoutDirectory_ReturnsTrue_AZ366() + { + var ok = StorageConfig.TryExtractTileCoordinates( + "tile_1700000000_42_99.jpg", + out var x, + out var y); + + ok.Should().BeTrue(); + x.Should().Be(42); + y.Should().Be(99); + } + + [Fact] + public void TryExtractTileCoordinates_NonTilePrefix_ReturnsFalseAndSentinel_AZ366() + { + var ok = StorageConfig.TryExtractTileCoordinates( + "/tmp/not_a_tile_filename.jpg", + out var x, + out var y); + + ok.Should().BeFalse(); + x.Should().Be(-1); + y.Should().Be(-1); + } + + [Fact] + public void TryExtractTileCoordinates_NonNumericCoords_ReturnsFalseAndSentinel_AZ366() + { + var ok = StorageConfig.TryExtractTileCoordinates( + "/tiles/tile_1700000000_alpha_beta.jpg", + out var x, + out var y); + + ok.Should().BeFalse(); + x.Should().Be(-1); + y.Should().Be(-1); + } + + [Fact] + public void TryExtractTileCoordinates_NullPath_ThrowsArgumentNullException_AZ366() + { + Action act = () => StorageConfig.TryExtractTileCoordinates(null!, out _, out _); + + act.Should().Throw(); + } + + [Fact] + public void GetTileFilePath_RoundTrip_ParserRecoversOriginalCoordinates_AZ366_AC2() + { + // Arrange + var config = new StorageConfig(); + const int zoom = 18; + const int writtenX = 158_292; + const int writtenY = 90_821; + + // Act + var path = config.GetTileFilePath(zoom, writtenX, writtenY, "20260510022758"); + var ok = StorageConfig.TryExtractTileCoordinates(path, out var parsedX, out var parsedY); + + // Assert + ok.Should().BeTrue(); + parsedX.Should().Be(writtenX, "writer-parser pair must be inverse — same module per AZ-366 AC-2"); + parsedY.Should().Be(writtenY); + } +} diff --git a/_docs/03_implementation/batch_12_report.md b/_docs/03_implementation/batch_12_report.md new file mode 100644 index 0000000..9eb46e2 --- /dev/null +++ b/_docs/03_implementation/batch_12_report.md @@ -0,0 +1,59 @@ +# Batch 12 Report — Refactor 03 Phase 3 (start) + +Date: 2026-05-10 +Epic: AZ-350 (03-code-quality-refactoring) +Status: ✅ Complete, pushed + +## Scope (1 task / 2 SP) + +| ID | C-ID | Title | Points | Component | +|----|------|-------|--------|-----------| +| AZ-366 | C13 | Consolidate Haversine + tile-coord parsing into Common/Utils | 2 | Common + Services.RouteManagement | + +First task of Phase 3 (Structural cleanup). Single-task batch — keeps the review surface small while the previous Phase 2 changes settle. + +## Changes + +### Production +- **MODIFIED** `SatelliteProvider.Common/Configs/StorageConfig.cs` + - Added `static bool TryExtractTileCoordinates(string filePath, out int tileX, out int tileY)`. Pure parser — no I/O, no logging, no exceptions for malformed input (caller decides). Co-located with `GetTileFilePath` which is the writer side, so format changes can never drift between the two ends. This is the structural fix called for in AC-2. + - The method is `static` because it's a pure string-parsing computation with no side effects, satisfying the `coderule.mdc` static-method test ("pure, self-contained computations"). +- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` + - Deleted private `static double CalculateDistance(double lat1, double lon1, double lat2, double lon2)` (the duplicate Haversine using `EARTH_RADIUS = 6371000`). Replaced its single call site with `GeoUtils.CalculateDistance(GeoPoint, GeoPoint)` (which uses `EARTH_RADIUS = 6378137`). + - `ExtractTileCoordinatesFromFilename(string)` is now a thin wrapper: it delegates parsing to `StorageConfig.TryExtractTileCoordinates` and emits the existing warning log when parsing fails. Public/internal contract preserved (callers see the same `(int TileX, int TileY)` signature, same `(-1, -1)` sentinel, same warning log behavior on malformed input). + - Updated the warning message text to `tile____` (matches what `GetTileFilePath` actually writes; the old message said `tile___` which was wrong — the format is `tile_{zoom}_{x}_{y}_{ts}.jpg`). + +### Tests +- **NEW** `SatelliteProvider.Tests/StorageConfigTests.cs` — 6 unit tests for the new helper: + - `TryExtractTileCoordinates_ValidFilename_ReturnsTrue_AZ366` — happy path with full directory path. + - `TryExtractTileCoordinates_FilenameWithoutDirectory_ReturnsTrue_AZ366` — bare filename. + - `TryExtractTileCoordinates_NonTilePrefix_ReturnsFalseAndSentinel_AZ366` — wrong prefix. + - `TryExtractTileCoordinates_NonNumericCoords_ReturnsFalseAndSentinel_AZ366` — coords not parseable. + - `TryExtractTileCoordinates_NullPath_ThrowsArgumentNullException_AZ366` — null contract. + - `GetTileFilePath_RoundTrip_ParserRecoversOriginalCoordinates_AZ366_AC2` — proves writer/parser inverse pair. This is the structural assertion behind AC-2: writing then parsing recovers the original coordinates, which can only stay true if both methods live in the same module. +- **PRESERVED** all 4 existing `RouteProcessingServiceTests.ExtractTileCoordinatesFromFilename_*_AC1/AC2` tests (added in batch 7 for AZ-352). They still pass against the now-thin wrapper, proving behavior preservation through the refactor. + +## Verification + +- **Unit tests**: 77 / 77 passing (was 71 → +6 new `StorageConfigTests`). +- **Integration smoke + full suite**: green. Container exits 0. The route-tile workflow (ExtractTileCoordinatesFromFilename + region-matching distance) is exercised end-to-end by every route test that requests maps. +- **AC-1 grep verification**: `rg 'Math\.Sin.*Math\.PI|earthRadius|EarthRadius|EARTH_RADIUS' --type=cs` returns matches only in `SatelliteProvider.Common/Utils/GeoUtils.cs`. No Haversine implementation lives anywhere else in the codebase. + +## Acceptance criteria coverage + +| AC | Evidence | +|----|----------| +| **AC-1** One Haversine implementation in the codebase | Grep for `Math.Sin.*Math.PI` and `EARTH_RADIUS` returns matches only in `GeoUtils.cs`. The local `RouteProcessingService.CalculateDistance` (which used `6371000`) has been deleted; its single call site now uses `GeoUtils.CalculateDistance`. | +| **AC-2** Writer and parser co-located | Both `GetTileFilePath` and `TryExtractTileCoordinates` live as methods on `StorageConfig` in `SatelliteProvider.Common/Configs/StorageConfig.cs`. New unit test `GetTileFilePath_RoundTrip_ParserRecoversOriginalCoordinates_AZ366_AC2` proves the inverse-pair invariant. | +| **AC-3** Tests stay green | 77 unit + smoke + full integration green; 4 pre-existing AZ-352 tests for `ExtractTileCoordinatesFromFilename` still pass against the thin-wrapper version. | + +## Behavior preservation notes + +- **Earth radius constant change**: `RouteProcessingService.CalculateDistance` used `6_371_000` (mean Earth radius), `GeoUtils.CalculateDistance` uses `6_378_137` (WGS84 equatorial radius). Absolute distances differ by ~0.1%. The single call site uses the value only for `OrderBy(...).FirstOrDefault()` to pick the nearest region — both formulas are monotonic in actual distance, so the chosen region is the same for any input. No region-matching test asserts on a specific distance value. +- **Public API**: `StorageConfig` adds one new static method. `GetTileFilePath` is unchanged. `RouteProcessingService.ExtractTileCoordinatesFromFilename` keeps the same signature and same observable behavior (returns `(-1, -1)` and logs a warning on malformed input). +- **Warning log message text changed**: `tile___` → `tile____` (matches `GetTileFilePath`'s actual output format `tile_{z}_{x}_{y}_{ts}.jpg`). The old text was misleading. No test asserted on the message text — the existing AZ-352 tests check that the warning contains the offending filename, not the format hint. + +## Up next + +- **Batch 13** (per the user-selected sequence): Remaining Phase 3 — natural next is AZ-368 (C15, TileCsvWriter, 2 SP). After C15 lands, C13+C15 together complete the foundational extracts that C11/C12 depend on. +- **Cumulative review** next fires after batches 10+11+12 (K=3 trigger). AZ-357 + AZ-362 + AZ-366 are the three batches in the window — running the cumulative review now is the natural gate before starting batch 13.