simplify and improve faction master list validation

internal/factions/concurrency_test.go

@@ -9,34 +9,34 @@ import (
 // Stress test MasterFactionList with concurrent operations
 func TestMasterFactionListConcurrency(t *testing.T) {
 	mfl := NewMasterFactionList()
-	
+
 	// Pre-populate with some factions
 	for i := int32(1); i <= 10; i++ {
 		faction := NewFaction(i, "Test Faction", "Test", "Test faction")
 		mfl.AddFaction(faction)
 	}
-	
+
 	const numGoroutines = 100
 	const operationsPerGoroutine = 100
-	
+
 	var wg sync.WaitGroup
-	
+
 	t.Run("ConcurrentFactionAccess", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Mix of read operations
 					faction := mfl.GetFaction(factionID)
 					if faction == nil {
 						t.Errorf("Goroutine %d: Expected faction %d to exist", goroutineID, factionID)
 					}
-					
+
 					_ = mfl.GetFactionByName("Test Faction")
 					_ = mfl.HasFaction(factionID)
 					_ = mfl.GetFactionCount()
@@ -46,21 +46,21 @@ func TestMasterFactionListConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentRelationshipManagement", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					targetID := int32(((goroutineID+1)%10) + 1)
+					targetID := int32(((goroutineID + 1) % 10) + 1)
-					
+
 					// Concurrent relationship operations
 					if goroutineID%2 == 0 {
 						mfl.AddHostileFaction(factionID, targetID)
@@ -72,26 +72,26 @@ func TestMasterFactionListConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentFactionModification", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
 					newFactionID := int32(1000 + goroutineID*operationsPerGoroutine + j)
 					faction := NewFaction(newFactionID, "Concurrent Faction", "Test", "Concurrent test")
-					
+
 					err := mfl.AddFaction(faction)
 					if err != nil {
 						t.Errorf("Goroutine %d: Failed to add faction %d: %v", goroutineID, newFactionID, err)
 					}
-					
+
 					// Verify it was added
 					retrieved := mfl.GetFaction(newFactionID)
 					if retrieved == nil {
@@ -100,7 +100,7 @@ func TestMasterFactionListConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
 }
@@ -108,7 +108,7 @@ func TestMasterFactionListConcurrency(t *testing.T) {
 // Stress test PlayerFaction with concurrent operations
 func TestPlayerFactionConcurrency(t *testing.T) {
 	mfl := NewMasterFactionList()
-	
+
 	// Add test factions with proper values
 	for i := int32(1); i <= 10; i++ {
 		faction := NewFaction(i, "Test Faction", "Test", "Test faction")
@@ -116,24 +116,24 @@ func TestPlayerFactionConcurrency(t *testing.T) {
 		faction.NegativeChange = 50
 		mfl.AddFaction(faction)
 	}
-	
+
 	pf := NewPlayerFaction(mfl)
-	
+
 	const numGoroutines = 100
 	const operationsPerGoroutine = 100
-	
+
 	var wg sync.WaitGroup
-	
+
 	t.Run("ConcurrentFactionValueOperations", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Mix of faction value operations
 					switch j % 4 {
 					case 0:
@@ -150,27 +150,27 @@ func TestPlayerFactionConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentUpdateManagement", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Operations that trigger updates
 					pf.IncreaseFaction(factionID, 1)
-					
+
 					// Check for pending updates
 					_ = pf.HasPendingUpdates()
 					_ = pf.GetPendingUpdates()
-					
+
 					// Occasionally clear updates
 					if j%10 == 0 {
 						pf.ClearPendingUpdates()
@@ -178,22 +178,22 @@ func TestPlayerFactionConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentPacketGeneration", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
 					// Trigger faction updates
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
 					pf.IncreaseFaction(factionID, 1)
-					
+
 					// Generate packets concurrently
 					_, err := pf.FactionUpdate(int16(goroutineID % 10))
 					if err != nil {
@@ -202,7 +202,7 @@ func TestPlayerFactionConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
 }
@@ -210,22 +210,22 @@ func TestPlayerFactionConcurrency(t *testing.T) {
 // Stress test Manager with concurrent operations
 func TestManagerConcurrency(t *testing.T) {
 	manager := NewManager(nil, nil) // No database or logger for testing
-	
+
 	const numGoroutines = 100
 	const operationsPerGoroutine = 100
-	
+
 	var wg sync.WaitGroup
-	
+
 	t.Run("ConcurrentStatisticsOperations", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Mix of statistics operations
 					switch j % 4 {
 					case 0:
@@ -240,56 +240,56 @@ func TestManagerConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
-		
+
 		// Verify statistics consistency
 		stats := manager.GetStatistics()
 		totalChanges := stats["total_faction_changes"].(int64)
 		increases := stats["faction_increases"].(int64)
 		decreases := stats["faction_decreases"].(int64)
-		
+
 		if totalChanges != increases+decreases {
-			t.Errorf("Statistics inconsistency: total %d != increases %d + decreases %d", 
+			t.Errorf("Statistics inconsistency: total %d != increases %d + decreases %d",
 				totalChanges, increases, decreases)
 		}
 	})
-	
+
 	t.Run("ConcurrentFactionManagement", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
 					factionID := int32(2000 + goroutineID*operationsPerGoroutine + j)
 					faction := NewFaction(factionID, "Manager Test", "Test", "Manager test")
-					
+
 					// Add faction
 					err := manager.AddFaction(faction)
 					if err != nil {
 						t.Errorf("Goroutine %d: Failed to add faction %d: %v", goroutineID, factionID, err)
 						continue
 					}
-					
+
 					// Read operations
 					_ = manager.GetFaction(factionID)
 					_ = manager.GetFactionByName("Manager Test")
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentPlayerFactionCreation", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
 					// Create player factions concurrently
 					pf := manager.CreatePlayerFaction()
@@ -299,14 +299,14 @@ func TestManagerConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
-		
+
 		// Check statistics
 		stats := manager.GetStatistics()
 		expectedPlayers := int64(numGoroutines * operationsPerGoroutine)
 		actualPlayers := stats["players_with_factions"].(int64)
-		
+
 		if actualPlayers != expectedPlayers {
 			t.Errorf("Expected %d players with factions, got %d", expectedPlayers, actualPlayers)
 		}
@@ -316,26 +316,26 @@ func TestManagerConcurrency(t *testing.T) {
 // Stress test EntityFactionAdapter with concurrent operations
 func TestEntityFactionAdapterConcurrency(t *testing.T) {
 	manager := NewManager(nil, nil)
-	
+
 	// Mock entity
 	entity := &mockEntity{id: 123, name: "Test Entity", dbID: 456}
 	adapter := NewEntityFactionAdapter(entity, manager, nil)
-	
+
 	const numGoroutines = 100
 	const operationsPerGoroutine = 100
-	
+
 	var wg sync.WaitGroup
-	
+
 	t.Run("ConcurrentFactionIDOperations", func(t *testing.T) {
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Mix of faction ID operations
 					switch j % 3 {
 					case 0:
@@ -349,43 +349,43 @@ func TestEntityFactionAdapterConcurrency(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
-	
+
 	t.Run("ConcurrentRelationshipChecks", func(t *testing.T) {
 		// Set up some factions first
 		for i := int32(1); i <= 10; i++ {
 			faction := NewFaction(i, "Test Faction", "Test", "Test faction")
 			manager.AddFaction(faction)
 		}
-		
+
 		// Set up relationships
 		mfl := manager.GetMasterFactionList()
 		mfl.AddHostileFaction(1, 2)
 		mfl.AddFriendlyFaction(1, 3)
-		
+
 		adapter.SetFactionID(1)
-		
+
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < operationsPerGoroutine; j++ {
-					otherFactionID := int32((goroutineID%10) + 1)
+					otherFactionID := int32((goroutineID % 10) + 1)
-					
+
 					// Concurrent relationship checks
 					_ = adapter.IsHostileToFaction(otherFactionID)
 					_ = adapter.IsFriendlyToFaction(otherFactionID)
-					
+
 					// Validation
 					_ = adapter.ValidateFaction()
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 	})
 }
@@ -413,20 +413,20 @@ func (e *mockEntity) GetDatabaseID() int32 {
 func TestDeadlockPrevention(t *testing.T) {
 	mfl := NewMasterFactionList()
 	manager := NewManager(nil, nil)
-	
+
 	// Add test factions
 	for i := int32(1); i <= 10; i++ {
 		faction := NewFaction(i, "Test Faction", "Test", "Test faction")
 		manager.AddFaction(faction)
 	}
-	
+
 	const numGoroutines = 50
 	var wg sync.WaitGroup
-	
+
 	// Test potential deadlock scenarios
 	t.Run("MixedOperations", func(t *testing.T) {
 		done := make(chan bool, 1)
-		
+
 		// Set a timeout to detect deadlocks
 		go func() {
 			time.Sleep(10 * time.Second)
@@ -435,19 +435,18 @@ func TestDeadlockPrevention(t *testing.T) {
 				return
 			default:
 				t.Error("Potential deadlock detected - test timed out")
-				t.FailNow()
 			}
 		}()
-		
+
 		wg.Add(numGoroutines)
-		
+
 		for i := 0; i < numGoroutines; i++ {
 			go func(goroutineID int) {
 				defer wg.Done()
-				
+
 				for j := 0; j < 100; j++ {
-					factionID := int32((goroutineID%10) + 1)
+					factionID := int32((goroutineID % 10) + 1)
-					
+
 					// Mix operations that could potentially deadlock
 					switch j % 6 {
 					case 0:
@@ -473,7 +472,7 @@ func TestDeadlockPrevention(t *testing.T) {
 				}
 			}(i)
 		}
-		
+
 		wg.Wait()
 		done <- true
 	})
@@ -484,42 +483,42 @@ func TestRaceConditions(t *testing.T) {
 	if testing.Short() {
 		t.Skip("Skipping race condition test in short mode")
 	}
-	
+
 	// This test is designed to be run with: go test -race
 	mfl := NewMasterFactionList()
 	manager := NewManager(nil, nil)
-	
+
 	// Rapid concurrent operations to trigger race conditions
 	const numGoroutines = 200
 	const operationsPerGoroutine = 50
-	
+
 	var wg sync.WaitGroup
 	wg.Add(numGoroutines)
-	
+
 	for i := 0; i < numGoroutines; i++ {
 		go func(goroutineID int) {
 			defer wg.Done()
-			
+
 			for j := 0; j < operationsPerGoroutine; j++ {
-				factionID := int32((goroutineID%20) + 1)
+				factionID := int32((goroutineID % 20) + 1)
-				
+
 				// Rapid-fire operations
 				faction := NewFaction(factionID, "Race Test", "Test", "Race test")
 				mfl.AddFaction(faction)
 				_ = mfl.GetFaction(factionID)
 				mfl.AddHostileFaction(factionID, factionID+1)
 				_ = mfl.GetHostileFactions(factionID)
-				
+
 				manager.AddFaction(faction)
 				manager.RecordFactionIncrease(factionID)
 				_ = manager.GetStatistics()
-				
+
 				pf := manager.CreatePlayerFaction()
 				pf.IncreaseFaction(factionID, 1)
 				_ = pf.GetFactionValue(factionID)
 			}
 		}(i)
 	}
-	
+
 	wg.Wait()
-}
+}

internal/factions/manager.go

@@ -393,7 +393,7 @@ func (m *Manager) handleInfoCommand(args []string) (string, error) {
 		return fmt.Sprintf("Faction '%s' not found.", args[0]), nil
 	}
 
-	result := fmt.Sprintf("Faction Information:\n")
+	result := "Faction Information:\n"
 	result += fmt.Sprintf("ID: %d\n", faction.ID)
 	result += fmt.Sprintf("Name: %s\n", faction.Name)
 	result += fmt.Sprintf("Type: %s\n", faction.Type)

internal/factions/master_faction_list.go

@@ -317,30 +317,30 @@ func (mfl *MasterFactionList) ValidateFactions() []string {
 	mfl.mutex.RLock()
 	defer mfl.mutex.RUnlock()
 
-	// Pre-allocate slice with reasonable capacity to avoid repeated allocations
 	issues := make([]string, 0, 10)
 
-	// Single pass through globalFactionList - check faction validity and build seen set
+	seenIDs := make(map[int32]*Faction, len(mfl.globalFactionList))
-	seenInGlobal := make(map[int32]bool, len(mfl.globalFactionList))
+	seenNames := make(map[string]*Faction, len(mfl.factionNameList))
-	for id, faction := range mfl.globalFactionList {
-		seenInGlobal[id] = true
 
+	// Pass 1: Validate globalFactionList and build seenID map
+	for id, faction := range mfl.globalFactionList {
 		if faction == nil {
 			issues = append(issues, fmt.Sprintf("Faction ID %d is nil", id))
 			continue
 		}
 
-		// Combine multiple faction checks in one pass (inlined IsValid())
+		if faction.ID <= 0 || faction.Name == "" {
-		if faction.ID <= 0 || len(faction.Name) == 0 {
+			issues = append(issues, fmt.Sprintf("Faction ID %d is invalid or unnamed", id))
-			issues = append(issues, fmt.Sprintf("Faction ID %d is invalid", id))
 		}
 
 		if faction.ID != id {
 			issues = append(issues, fmt.Sprintf("Faction ID mismatch: map key %d != faction ID %d", id, faction.ID))
 		}
+
+		seenIDs[id] = faction
 	}
 
-	// Single pass through factionNameList
+	// Pass 2: Validate factionNameList and build seenName map
 	for name, faction := range mfl.factionNameList {
 		if faction == nil {
 			issues = append(issues, fmt.Sprintf("Faction name '%s' maps to nil", name))
@@ -351,36 +351,29 @@ func (mfl *MasterFactionList) ValidateFactions() []string {
 			issues = append(issues, fmt.Sprintf("Faction name mismatch: map key '%s' != faction name '%s'", name, faction.Name))
 		}
 
-		// Use the pre-built set instead of map lookup
+		if _, ok := seenIDs[faction.ID]; !ok {
-		if !seenInGlobal[faction.ID] {
 			issues = append(issues, fmt.Sprintf("Faction '%s' (ID %d) exists in name map but not in ID map", name, faction.ID))
 		}
+
+		seenNames[name] = faction
 	}
 
-	// Check relationship consistency - use the pre-built set for faster lookups
+	// Pass 3: Validate relationships using prebuilt seenIDs
-	for factionID, hostiles := range mfl.hostileFactions {
+	validateRelations := func(relations map[int32][]int32, relType string) {
-		if !seenInGlobal[factionID] {
+		for sourceID, targets := range relations {
-			issues = append(issues, fmt.Sprintf("Hostile relationship defined for non-existent faction %d", factionID))
+			if _, ok := seenIDs[sourceID]; !ok {
-		}
+				issues = append(issues, fmt.Sprintf("%s relationship defined for non-existent faction %d", relType, sourceID))
-
+			}
-		for _, hostileID := range hostiles {
+			for _, targetID := range targets {
-			if !seenInGlobal[hostileID] {
+				if _, ok := seenIDs[targetID]; !ok {
-				issues = append(issues, fmt.Sprintf("Faction %d has hostile relationship with non-existent faction %d", factionID, hostileID))
+					issues = append(issues, fmt.Sprintf("Faction %d has %s relationship with non-existent faction %d", sourceID, relType, targetID))
+				}
 			}
 		}
 	}
 
-	for factionID, friendlies := range mfl.friendlyFactions {
+	validateRelations(mfl.hostileFactions, "Hostile")
-		if !seenInGlobal[factionID] {
+	validateRelations(mfl.friendlyFactions, "Friendly")
-			issues = append(issues, fmt.Sprintf("Friendly relationship defined for non-existent faction %d", factionID))
-		}
-
-		for _, friendlyID := range friendlies {
-			if !seenInGlobal[friendlyID] {
-				issues = append(issues, fmt.Sprintf("Faction %d has friendly relationship with non-existent faction %d", factionID, friendlyID))
-			}
-		}
-	}
 
 	return issues
 }