fix: address PR review feedback for Epic 4.8

- Consolidate class progress updates into single variable
- Extract MasterRankLevel constant to ClassProgressCollection
- Add missing using statement for ClassProgressCollection

Refs #39

Author: martincjarvis

src/FantasyRpgWorld.Core/Domain/Components/ClassProgressCollection.cs

@@ -8,6 +8,11 @@ namespace FantasyRpgWorld.Core.Domain.Components;
 /// </summary>
 public readonly record struct ClassProgressCollection
 {
+    /// <summary>
+    /// Master rank level threshold. Achieved when creating a Masterwork item.
+    /// </summary>
+    public const int MasterRankLevel = 10;
+
     private readonly IReadOnlyDictionary<ClassId, ClassProgress> _progressByClass;
 
     public ClassProgressCollection()

src/FantasyRpgWorld.Simulation/Systems/CraftingSystem.cs

@@ -1,4 +1,5 @@
 using FantasyRpgWorld.Core.Definitions;
+using FantasyRpgWorld.Core.Domain.Components;
 using FantasyRpgWorld.Core.Domain.ValueObjects;
 using FantasyRpgWorld.Core.Domain.World;
 using FantasyRpgWorld.Core.Events;
@@ -62,10 +63,11 @@ public SystemUpdateResult Update(WorldState worldState, GameTime currentTime)
                 activeCraft.WorkspaceModifier,
                 _random);
 
-            // Award XP if item was produced
+            // Consolidate all class progress updates
             int xpAwarded = 0;
-            var updatedCharacter = character;
+            var classProgress = character.ClassProgress;
 
+            // Award XP if item was produced
             if (outcomeResult.ItemProduced)
             {
                 // Calculate base XP (simple formula: quality / 2)
@@ -76,35 +78,29 @@ public SystemUpdateResult Update(WorldState worldState, GameTime currentTime)
                 xpAwarded = XpCalculator.CalculateXp(baseXp, totalLevels);
 
                 // Award XP to all applicable classes (classes listed in skill requirements)
-                var classProgress = character.ClassProgress;
                 foreach (var skillReq in recipe.SkillRequirements)
                 {
                     if (classProgress.HasClass(skillReq.ClassId))
                     {
                         classProgress = classProgress.AddXp(skillReq.ClassId, xpAwarded, out _);
                     }
                 }
-
-                updatedCharacter = updatedCharacter.WithClassProgress(classProgress);
             }
 
             // Handle Masterwork → Master rank trigger
             if (outcomeResult.Outcome == CraftingOutcome.Masterwork)
             {
-                const int MasterRankLevel = 10;
-                var classProgress = updatedCharacter.ClassProgress;
-
                 foreach (var skillReq in recipe.SkillRequirements)
                 {
                     if (classProgress.HasClass(skillReq.ClassId))
                     {
                         int currentLevel = classProgress.GetLevel(skillReq.ClassId);
-                        if (currentLevel < MasterRankLevel)
+                        if (currentLevel < ClassProgressCollection.MasterRankLevel)
                         {
                             // Set character to Master rank (level 10)
-                            // Calculate XP needed to reach level 10
+                            // Calculate XP needed to reach Master rank
                             int xpToAdd = 0;
-                            for (int level = currentLevel; level < MasterRankLevel; level++)
+                            for (int level = currentLevel; level < ClassProgressCollection.MasterRankLevel; level++)
                             {
                                 xpToAdd += 100 * level; // XP formula from ClassProgressCollection
                             }
@@ -120,12 +116,12 @@ public SystemUpdateResult Update(WorldState worldState, GameTime currentTime)
                         }
                     }
                 }
-
-                updatedCharacter = updatedCharacter.WithClassProgress(classProgress);
             }
 
-            // Clear the active craft
-            updatedCharacter = updatedCharacter.WithActiveCraft(null);
+            // Apply all class progress changes at once
+            var updatedCharacter = character
+                .WithClassProgress(classProgress)
+                .WithActiveCraft(null);
 
             // Update world state
             updatedWorld = updatedWorld.UpdateCharacter(updatedCharacter);

tests/FantasyRpgWorld.Simulation.Tests/Systems/CraftingSystemMasterworkTests.cs

@@ -14,7 +14,6 @@ namespace FantasyRpgWorld.Simulation.Tests.Systems;
 /// </summary>
 public class CraftingSystemMasterworkTests
 {
-    private const int MasterRankLevel = 10;
 
     private static Character CreateTestCharacter(string name, ClassProgressCollection? classProgress = null)
     {
@@ -92,7 +91,7 @@ public void MasterworkCraft_GrantsMasterRankToNovice()
         var updatedCharacter = result.UpdatedWorld.GetCharacter(character.Id);
         Assert.NotNull(updatedCharacter);
         var blacksmithLevel = updatedCharacter.ClassProgress.GetLevel(new ClassId("blacksmith"));
-        Assert.Equal(MasterRankLevel, blacksmithLevel);
+        Assert.Equal(ClassProgressCollection.MasterRankLevel, blacksmithLevel);
     }
 
     [Fact]
@@ -146,7 +145,7 @@ public void MasterworkCraft_WhenAlreadyMaster_DoesNotEmitDuplicateEvent()
         // Character with blacksmith at level 10
         var classProgress = new ClassProgressCollection().AddClass(new ClassId("blacksmith"));
         // Level up to 10
-        for (int i = 1; i < MasterRankLevel; i++)
+        for (int i = 1; i < ClassProgressCollection.MasterRankLevel; i++)
         {
             int xpNeeded = 100 * i; // Based on ClassProgressCollection formula
             classProgress = classProgress.AddXp(new ClassId("blacksmith"), xpNeeded, out _);