🔍 GW2 Crafting Calculator - Comprehensive Code Refactoring Analysis ## 1. Top 5 Critical Issues Matrix | Priority | Issue | Impact | Files Affected | |---|---|---|---| | 🔴 CRITICAL | Singleton Anti-Pattern with Public Static Instance | Thread-safety, testability, tight coupling | Form1.cs, PreparedRecipeData.cs | | 🔴 CRITICAL | Lack of Dependency Injection | Hard to test, tight coupling, poor maintainability | Form1.cs, ApiService.cs | | 🔴 CRITICAL | No Exception Handling Strategy | Silent failures, poor user experience | All Service files | | 🟡 HIGH | Poor Naming Conventions (Polish + English Mix) | Code readability, maintainability | All files | | 🟡 HIGH | Magic Numbers and Hardcoded Values | Configuration inflexibility, portability issues | ApiService.cs, ProfitCalculator.cs | ## 2. Codebase Overview & Architecture **Technology Stack:** C# (.NET 9.0), Windows Forms. **Current State:** Attempted 3-tier (Models, Services, UI) but severely coupled. Form1.cs acts as a "God Class" orchestrating over 600 lines of logic, directly accessing data models and bypassing service abstraction. ## 3. Cross-File Dependencies & Coupling graph TD Program[Program.cs] --> Form1[Form1.cs] Form1 --> ApiService[ApiService.cs] Form1 --> RecipeService[RecipeService.cs] Form1 --> ProfitCalculator[ProfitCalculator.cs] PreparedRecipeData[PreparedRecipeData.cs] -.->|Static reference| Form1 ApiService --> GW2Item[GW2Item.cs] **Critical Issue:** Bidirectional Dependency. PreparedRecipeData.cs references Form1.Instance, while Form1.cs creates PreparedRecipeData instances. This violates dependency inversion and makes testing impossible. ## 4. Code Duplication & DRY Violations **Ingredient Name Resolution Logic:** Duplicated across 3 locations (Form1.cs, PreparedRecipeData.cs). **Suggested Fix:** Extract to a shared service IngredientNameResolver. ## 5. Readability & Naming Convention Issues - **Mixed Languages:** Properties in English (availableIngredientsIds), but UI labels and status messages in Polish. - **Generic Naming:** button1_Click, button2_Click. Must be refactored to btnFetchItemIds_Click, etc. ## 6. SOLID Principle Violations **Single Responsibility Principle (SRP) Violation in Form1.cs:** Currently handles UI events, data caching, file I/O, business logic, and progress reporting. It must be split into MainForm.cs (UI), DataManager.cs, and WorkflowOrchestrator.cs. ## 7. Refactoring Recommendations **🔴 Remove Singleton Anti-Pattern** *Before:* // PreparedRecipeData.cs var availableIds = UI.Form1.Instance?.availableIngredientsIds?.Values.ToHashSet() // Form1.cs public static Form1 Instance { get; private set; } *After (Dependency Injection):* public class RecipeDisplayFormatter { private readonly IIngredientRepository _ingredientRepository; public RecipeDisplayFormatter(IIngredientRepository ingredientRepository) { _ingredientRepository = ingredientRepository; } public string FormatIngredients(PreparedRecipeData recipe) { var availableIds = _ingredientRepository.GetAvailableIds(); // formatting logic } } ## 8. Phased Implementation Plan - **Phase 1: Foundation (Low Risk):** Set up DI Container, extract config to appsettings.json. - **Phase 2: Services (Medium Risk):** Extract IApiClient interfaces, add global exception handling. - **Phase 3: Form1 Decomposition (High Risk):** Create WorkflowOrchestrator.cs and move logic out of Form1 incrementally. ## 9. Summary & Quick Wins Action for today: Add appsettings.json to externalize URLs and magic numbers, rename generic buttons in the designer, and create interfaces for all existing services to prepare for Dependency Injection.