From 95d42803e1b25787d304ecca7e1bed52125c2dc6 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 16:58:38 -0400 Subject: [PATCH] NEO-67: address code review nits on recipe registry Delegate TryGetDefinition to catalog.TryGetRecipe, cache ordered defs at construction, and assert unknown recipeId in host DI test. --- docs/reviews/2026-05-24-NEO-67.md | 6 +++--- .../Crafting/RecipeDefinitionRegistryTests.cs | 3 +++ .../Game/Crafting/RecipeDefinitionRegistry.cs | 15 ++++++--------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/reviews/2026-05-24-NEO-67.md b/docs/reviews/2026-05-24-NEO-67.md index 57ab41b..966e272 100644 --- a/docs/reviews/2026-05-24-NEO-67.md +++ b/docs/reviews/2026-05-24-NEO-67.md @@ -37,11 +37,11 @@ None. ## Nits -- Nit: `RecipeDefinitionRegistry.TryGetDefinition` reads `catalog.ById` directly instead of delegating to `catalog.TryGetRecipe` — consistent with `ItemDefinitionRegistry` (NEO-52), but both bypass the catalog’s public lookup helper; harmless for immutable data. +- ~~Nit: `RecipeDefinitionRegistry.TryGetDefinition` reads `catalog.ById` directly instead of delegating to `catalog.TryGetRecipe` — consistent with `ItemDefinitionRegistry` (NEO-52), but both bypass the catalog’s public lookup helper; harmless for immutable data.~~ **Done.** Registry now delegates to `catalog.TryGetRecipe`. -- Nit: `GetDefinitionsInIdOrder()` allocates a new sorted list on every call — acceptable for prototype HTTP/craft volume; optional future optimization is to cache the ordered list at catalog/registry construction time if hot-path profiling warrants it (same as item registry). +- ~~Nit: `GetDefinitionsInIdOrder()` allocates a new sorted list on every call — acceptable for prototype HTTP/craft volume; optional future optimization is to cache the ordered list at catalog/registry construction time if hot-path profiling warrants it (same as item registry).~~ **Done.** Ordered list built once in `RecipeDefinitionRegistry` constructor. -- Nit: `Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` asserts a known id and enumeration count but not an unknown id in the host context — unit tests cover unknown/null; host test could add one line for symmetry with E3M2-03 acceptance wording (“distinguishable from valid ids in tests”). +- ~~Nit: `Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` asserts a known id and enumeration count but not an unknown id in the host context — unit tests cover unknown/null; host test could add one line for symmetry with E3M2-03 acceptance wording (“distinguishable from valid ids in tests”).~~ **Done.** Host test asserts unknown `recipeId` returns false without throw. ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionRegistryTests.cs b/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionRegistryTests.cs index 8a204d6..4a20715 100644 --- a/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionRegistryTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionRegistryTests.cs @@ -243,11 +243,14 @@ public class RecipeDefinitionRegistryTests // Act var registry = factory.Services.GetRequiredService(); var found = registry.TryGetDefinition("refine_scrap_standard", out var refine); + var unknown = registry.TryGetDefinition("not_a_real_recipe", out var missing); var list = registry.GetDefinitionsInIdOrder(); // Assert Assert.True(found); Assert.NotNull(refine); Assert.Equal("process", refine.RecipeKind); + Assert.False(unknown); + Assert.Null(missing); Assert.Equal(8, list.Count); Assert.Contains(list, r => r.Id == "make_field_stim_mk0" && r.RecipeKind == "make"); } diff --git a/server/NeonSprawl.Server/Game/Crafting/RecipeDefinitionRegistry.cs b/server/NeonSprawl.Server/Game/Crafting/RecipeDefinitionRegistry.cs index 441a0f5..7b84018 100644 --- a/server/NeonSprawl.Server/Game/Crafting/RecipeDefinitionRegistry.cs +++ b/server/NeonSprawl.Server/Game/Crafting/RecipeDefinitionRegistry.cs @@ -5,6 +5,8 @@ namespace NeonSprawl.Server.Game.Crafting; /// Adapter over (NEO-67). public sealed class RecipeDefinitionRegistry(RecipeDefinitionCatalog catalog) : IRecipeDefinitionRegistry { + private readonly IReadOnlyList _definitionsInIdOrder = BuildDefinitionsInIdOrder(catalog); + /// public bool TryGetDefinition(string? recipeId, [NotNullWhen(true)] out RecipeDefRow? definition) { @@ -14,18 +16,13 @@ public sealed class RecipeDefinitionRegistry(RecipeDefinitionCatalog catalog) : return false; } - if (catalog.ById.TryGetValue(recipeId, out var row)) - { - definition = row; - return true; - } - - definition = null; - return false; + return catalog.TryGetRecipe(recipeId, out definition); } /// - public IReadOnlyList GetDefinitionsInIdOrder() + public IReadOnlyList GetDefinitionsInIdOrder() => _definitionsInIdOrder; + + private static IReadOnlyList BuildDefinitionsInIdOrder(RecipeDefinitionCatalog catalog) { var ids = catalog.ById.Keys.OrderBy(k => k, StringComparer.Ordinal).ToArray(); var list = new List(ids.Length);