🔍 GW2 Crafting Calculator - Comprehensive Code Refactoring Analysis 📊 Top 5 Critical Issues PriorityIssueImpactFiles Affected🔴 CRITICALSingleton Anti-Pattern with Public Static InstanceThread-safety, testability, tight couplingForm1.cs, PreparedRecipeData.cs🔴 CRITICALLack of Dependency InjectionHard to test, tight coupling, poor maintainabilityForm1.cs, ApiService.cs🔴 CRITICALNo Exception Handling StrategySilent failures, poor user experienceAll Service files🟡 HIGHPoor Naming Conventions (Polish + English Mix)Code readability, maintainabilityAll files🟡 HIGHMagic Numbers and Hardcoded ValuesConfiguration inflexibility, portability issuesApiService.cs, ProfitCalculator.cs 1. 📁 Codebase Overview & Architecture Directory Structure GW2CraftingCalculator/ ├── Models/ # Data models (8 files) │ ├── GW2Item.cs │ ├── GW2Recipe.cs │ ├── GW2TPResponse.cs │ ├── Ingredient.cs │ ├── PreparedRecipeData.cs │ ├── PriceData.cs │ ├── ProfitResult.cs │ └── UniqueUnavailableItem.cs ├── Services/ # Business logic (3 files) │ ├── ApiService.cs │ ├── ProfitCalculator.cs │ └── RecipeService.cs ├── UI/ # User interface (3 files) │ ├── DataGridViewHelper.cs │ ├── Form1.cs │ └── Form1.Designer.cs ├── Utils/ # Utilities (3 files) │ ├── CurrencyFormatter.cs │ ├── FileHelper.cs │ └── SortableBindingList.cs └── Program.cs # Entry point Technology Stack Language: C# (.NET 9.0) Framework: Windows Forms Architecture Pattern: Attempted 3-tier (Models, Services, UI) Data Binding: BindingList with custom sorting Component Purposes ComponentPurposeLines of CodeModelsData transfer objects, API responses~200ServicesAPI communication, business logic~400UIWindows Forms interface, DataGridView setup~600UtilsHelper functions, formatting~150 2. 🔗 Cross-File Dependencies & Coupling Dependency Graph mermaidgraph TD Program[Program.cs] --> Form1[Form1.cs] Form1 --> ApiService[ApiService.cs] Form1 --> RecipeService[RecipeService.cs] Form1 --> ProfitCalculator[ProfitCalculator.cs] Form1 --> FileHelper[FileHelper.cs] Form1 --> DataGridViewHelper[DataGridViewHelper.cs] Form1 --> CurrencyFormatter[CurrencyFormatter.cs] Form1 --> SortableBindingList[SortableBindingList.cs] PreparedRecipeData[PreparedRecipeData.cs] -.->|Static reference| Form1 ApiService --> GW2Item[GW2Item.cs] ApiService --> GW2Recipe[GW2Recipe.cs] ApiService --> GW2TPResponse[GW2TPResponse.cs] ApiService --> PriceData[PriceData.cs] RecipeService --> GW2Recipe RecipeService --> Ingredient[Ingredient.cs] ProfitCalculator --> PreparedRecipeData ProfitCalculator --> PriceData ProfitCalculator --> ProfitResult[ProfitResult.cs] DataGridViewHelper --> ProfitResult DataGridViewHelper --> PreparedRecipeData DataGridViewHelper --> UniqueUnavailableItem[UniqueUnavailableItem.cs] 🔴 Critical Coupling Issues 1. Bidirectional Dependency (Circular Reference) csharp// PreparedRecipeData.cs references Form1 var availableIds = UI.Form1.Instance?.availableIngredientsIds?.Values.ToHashSet() // Form1.cs creates PreparedRecipeData instances var preparedRecipe = new PreparedRecipeData { ... } Impact: Violates dependency inversion, makes testing impossible, creates tight coupling. 2. God Class: Form1.cs Dependencies: 8 different classes Responsibilities: UI, data management, orchestration, file I/O Lines: 600+ Everything depends on Form1 3. Static Instance Anti-Pattern csharppublic static Form1 Instance { get; private set; } Problems: Global state Thread-unsafe Untestable Violates OOP principles 3. 🔄 Code Duplication Across Files Critical Duplications 1. Ingredient Name Resolution Logic (3 locations) Files: Form1.cs (GetIngredientNames), PreparedRecipeData.cs (ComputeFormattedDisplay) csharp// Duplicated in PreparedRecipeData.cs var name = IngredientNames.TryGetValue(ingredient.id, out var n) ? n : $"ID:{ingredient.id}"; // Similar pattern in Form1.cs var kvp = availableIngredientsIds.FirstOrDefault(x => x.Value == ingredient.id); if (!kvp.Equals(default(KeyValuePair<string, int>))) { ingredientNames[ingredient.id] = kvp.Key; } Suggested Fix: Extract to shared service IngredientNameResolver 2. DataGridView Setup Pattern (6 times) File: DataGridViewHelper.cs csharp// Repeated pattern across all Setup methods grid.AutoGenerateColumns = false; grid.AllowUserToOrderColumns = true; grid.Columns.Clear(); // ... column definitions Suggested Fix: Extract base setup method with column configuration as parameter 3. File Loading Pattern (Multiple locations) Files: Form1.cs, FileHelper.cs csharp// Pattern repeated with different file names if (!File.Exists(fileName)) return default; var json = File.ReadAllText(fileName); return JsonSerializer.Deserialize<T>(json); Suggested Fix: Already centralized in FileHelper, but Form1 has redundant checks 4. 📖 Readability Issues Critical Readability Problems 1. Mixed Language (Polish + English) csharp// Polish property names public string BaseIngredientsFormattedDisplay // English public Dictionary<string, int> availableIngredientsIds // English private string _baseIngredientsFormattedDisplay // English // But UI labels in Polish HeaderText = "Nazwa" // Polish for "Name" HeaderText = "Składniki Bazowe" // Polish for "Base Ingredients" labelStatus.Text = "Pobieranie danych..." // Polish status messages Impact: Confusing for international developers, inconsistent codebase 2. Unclear Button Naming csharpprivate void button1_Click // What does button1 do? private void button2_Click // What does button2 do? // ... button3, button4, button5, button6, button7, button8 Better Names: btnFetchItemIds_Click, btnLoadItems_Click, etc. 3. Complex Nested Logic Without Comments csharp// RecipeService.cs - No explanation of complex recursion private async Task DecomposeIngredient( int itemId, int count, Dictionary<int, GW2Recipe> recipesDict, HashSet<int> availableIds, Dictionary<int, int> baseIngredients, HashSet<int> visitedRecipes) { // 80 lines of complex recursive logic with minimal comments } 5. 🏷️ Naming Convention Violations Specific Issues with Fixes Current NameIssueSuggested FixLocationbutton1 - button8Non-descriptivebtnFetchItemIds, btnLoadRecipes, etc.Form1.Designer.csdataGridView1 - dataGridView6GenericdgvAvailableIngredients, dgvProfits, etc.Form1.Designer.cstabPage1 - tabPage6GenerictabAvailableIngredients, tabProfits, etc.Form1.Designer.csallItemsToo genericcachedGW2ItemsForm1.csallRecipesToo genericcachedGW2RecipesForm1.cs_baseIngredientsFormattedDisplayVerbose_formattedDisplayPreparedRecipeData.csavailableIngredientsIdsMixed caseavailableIngredientIds (singular)Form1.csseven_d_buy_soldSnake case in C#SevenDayBuySoldPriceData.cs Inconsistent Casing csharp// Models use lowercase properties (wrong for C#) public class GW2Item { public int id { get; set; } // Should be Id public string name { get; set; } // Should be Name public string type { get; set; } // Should be Type } // But other classes use PascalCase correctly public class PreparedRecipeData { public int RecipeId { get; set; } // Correct public string OutputItemName { get; set; } // Correct } 6. 🔨 KISS Principle Violations Overcomplicated Logic 1. PreparedRecipeData Caching Logic csharpprivate string _baseIngredientsFormattedDisplay; public string BaseIngredientsFormattedDisplay { get { if (_baseIngredientsFormattedDisplay != null) return _baseIngredientsFormattedDisplay; return _baseIngredientsFormattedDisplay = ComputeFormattedDisplay(); } } public void PreComputeFormattedDisplay(HashSet<int> availableIds) { if (_baseIngredientsFormattedDisplay == null) { _baseIngredientsFormattedDisplay = ComputeFormattedDisplay(availableIds); } } Problem: Two different compute paths, one with parameters, one without. Confusing caching strategy. Simpler Approach: csharppublic string GetFormattedDisplay(HashSet<int> availableIds) { return ComputeFormattedDisplay(availableIds); } Cache at a higher level if needed. 2. Ingredient Name Resolution csharp// Overly complex with multiple fallbacks var kvp = availableIngredientsIds.FirstOrDefault(x => x.Value == ingredient.id); if (!kvp.Equals(default(KeyValuePair<string, int>))) { ingredientNames[ingredient.id] = kvp.Key; } else { kvp = excludedIngredientsIds.FirstOrDefault(x => x.Value == ingredient.id); if (!kvp.Equals(default(KeyValuePair<string, int>))) { ingredientNames[ingredient.id] = kvp.Key; } else if (itemsDict.TryGetValue(ingredient.id, out var item)) { ingredientNames[ingredient.id] = item.name; } else { ingredientNames[ingredient.id] = $"[Waluta/Nieznany] ID:{ingredient.id}"; } } Simpler: csharpprivate string GetIngredientName(int id) { return availableIngredientIdToName.TryGetValue(id, out var name) ? name : excludedIngredientIdToName.TryGetValue(id, out name) ? name : itemsDict.TryGetValue(id, out var item) ? item.name : $"[Unknown] ID:{id}"; } 7. 🔁 DRY Principle Violations Major Code Repetition 1. Progress Bar Pattern csharp// Repeated 5+ times across Form1.cs progressBar1.Maximum = items.Count; progressBar1.Value = 0; for (int i = 0; i < items.Count; i++) { // ... processing progressBar1.Value = i + 1; if (i % 100 == 0) await Task.Delay(1); } Extract to: csharpawait ProcessWithProgress(items, async (item, progress) => { // processing logic }, updateInterval: 100); 2. DataGridView Column Creation csharp// Repeated in DataGridViewHelper for each grid type grid.Columns.Add(new DataGridViewTextBoxColumn { Name = "...", HeaderText = "...", DataPropertyName = "...", Width = ... }); Extract to: csharpprivate static void AddColumn(DataGridView grid, string name, string header, string property, int width) { grid.Columns.Add(new DataGridViewTextBoxColumn { Name = name, HeaderText = header, DataPropertyName = property, Width = width }); } 3. File Existence Checks csharp// Repeated in FileHelper and Form1 if (!File.Exists(fileName)) return default; ``` Already centralized in FileHelper but still checked redundantly in Form1. --- ## 8. 📦 Single Responsibility Principle Violations ### God Classes and Mixed Responsibilities #### 1. **Form1.cs** (600+ lines) - Violates SRP Severely **Current Responsibilities**: 1. UI event handling 2. Data management (caching) 3. File I/O orchestration 4. Business logic orchestration 5. Progress reporting 6. DataGridView configuration 7. Data transformation 8. Validation **Should Be Split Into**: ``` - Form1.cs (UI only) - DataManager.cs (data caching) - WorkflowOrchestrator.cs (business flow) - ProgressReporter.cs (progress updates) 2. PreparedRecipeData.cs - Mixed Concerns Current Responsibilities: Data storage Display formatting Caching logic UI concern (accessing Form1.Instance) Should Be: csharp// Pure data model public class PreparedRecipeData { public int RecipeId { get; set; } public List<Ingredient> AllBaseIngredients { get; set; } // ... only data } // Separate formatter public class RecipeDisplayFormatter { public string FormatIngredients(PreparedRecipeData recipe, HashSet<int> availableIds) { // formatting logic } } ``` #### 3. **ApiService.cs** - Multiple APIs **Current**: - GW2 Official API - GW2TP API - DataWars2 API **Should Be**: ``` - IGW2ApiClient - IGW2TPApiClient - IDataWars2ApiClient - ApiServiceFactory 9. 🔀 Nesting Complexity Issues Deep Nesting Examples 1. Form1.button4_Click - 5 Levels Deep csharpawait ExecuteWithProgress(sender as Button, async () => { for (int i = 0; i < allRecipes.Count; i++) // Level 2 { if (itemsDict.TryGetValue(recipe.output_item_id, out var outputItem)) // Level 3 { if (outputItem.flags != null && // Level 4 (outputItem.flags.Contains("NoSell") || outputItem.flags.Contains("AccountBound"))) // Level 5 { progressBar1.Value = i + 1; continue; } } // more nested logic } }); Refactored with Guard Clauses: csharpprivate bool ShouldSkipRecipe(GW2Recipe recipe) { if (!itemsDict.TryGetValue(recipe.output_item_id, out var outputItem)) return false; if (outputItem.flags == null) return false; return outputItem.flags.Contains("NoSell") || outputItem.flags.Contains("AccountBound"); } // In loop if (ShouldSkipRecipe(recipe)) continue; 2. RecipeService.DecomposeIngredient - 6 Levels Deep csharpprivate async Task DecomposeIngredient(...) { if (availableIds.Contains(itemId)) // Level 1 { // ... } if (recipesDict.TryGetValue(itemId, out var subRecipe)) // Level 1 { if (visitedRecipes.Contains(subRecipe.id)) // Level 2 { // ... } if (subRecipe.ingredients == null || subRecipe.ingredients.Count == 0) // Level 2 { // ... } foreach (var subIngredient in subRecipe.ingredients) // Level 2 { // Level 3, 4, 5, 6... } } } Refactored: csharpprivate async Task DecomposeIngredient(...) { if (TryAddAvailableIngredient(itemId, count, baseIngredients)) return; if (!recipesDict.TryGetValue(itemId, out var subRecipe)) { AddUnavailableIngredient(itemId, count, baseIngredients); return; } if (IsCircularDependency(subRecipe, visitedRecipes)) { AddUnavailableIngredient(itemId, count, baseIngredients); return; } await ProcessSubRecipe(subRecipe, count, ...); } 10. 🌍 Global Variables and State Management Global State Issues 1. Singleton Instance csharppublic static Form1 Instance { get; private set; } Problem: Global mutable state Impact: Thread-unsafe, untestable, tight coupling Used by: PreparedRecipeData.cs 2. Form1 Public Fields csharppublic Dictionary<string, int> availableIngredientsIds = new Dictionary<string, int>(); public Dictionary<string, int> excludedIngredientsIds = new Dictionary<string, int>(); Problem: Public mutable dictionaries Impact: Anyone can modify, no encapsulation Better: Private with public readonly interface 3. Unused Log Field csharpprivate string logFileName = $"debug_log_{DateTime.Now:yyyyMMdd_HHmmss}.txt"; Problem: Field created but never used Impact: Dead code, misleading Suggested State Management csharppublic interface IIngredientRepository { bool IsAvailable(int ingredientId); string GetName(int ingredientId); IReadOnlyDictionary<int, string> GetAvailableIngredients(); } public class IngredientRepository : IIngredientRepository { private readonly Dictionary<string, int> _availableIngredients; private readonly Dictionary<string, int> _excludedIngredients; // Encapsulated state with controlled access } 11. 🖥️ Portability Issues Platform-Specific Dependencies 1. Hardcoded File Paths csharpFileHelper.LoadIngredientsFromFile("available_ingredients_ids.txt"); FileHelper.SaveJsonToFile("all_items.json", allItems); Problem: Assumes current directory Impact: Breaks in different deployment scenarios Solution: csharppublic class AppPaths { public static string DataDirectory => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Data"); public static string GetDataFile(string fileName) => Path.Combine(DataDirectory, fileName); } 2. Hardcoded API URLs csharpprivate const string GW2TP_ITEMS_NAMES_URL = "https://api.gw2tp.com/1/bulk/items-names.json"; private const string GW2_ITEMS_BASE_URL = "https://api.guildwars2.com/v2/items"; Problem: Cannot change endpoints without recompiling Impact: Testing, environment changes Solution: Configuration file json{ "ApiEndpoints": { "GW2Official": "https://api.guildwars2.com/v2", "GW2TP": "https://api.gw2tp.com/1", "DataWars2": "https://api.datawars2.ie/gw2/v1" } } 3. Magic Numbers csharpconst int batchSize = 200; // API limit var outputSellValue = outputPrice.sell_price * 0.85; // TP fee await Task.Delay(50); // Rate limiting Solution: Configuration class csharppublic class GameConstants { public const int TradingPostFeePercent = 15; public const int ApiBatchSize = 200; public const int ApiDelayMilliseconds = 50; } 12. ♻️ Reusability Concerns Tight Coupling Issues 1. UI-Specific Models csharppublic class PreparedRecipeData { // UI-specific property in data model public string BaseIngredientsFormattedDisplay { get; } // Accesses UI directly var availableIds = UI.Form1.Instance?.availableIngredientsIds?.Values.ToHashSet() } Problem: Model tightly coupled to UI, cannot be reused in: Console application Web API Unit tests Solution: Separate concerns csharp// Pure data model public class PreparedRecipeData { public int RecipeId { get; set; } public List<Ingredient> Ingredients { get; set; } } // Separate formatter for UI public interface IRecipeFormatter { string Format(PreparedRecipeData recipe, IIngredientRepository ingredients); } 2. Hard-Coded UI Components csharppublic static class DataGridViewHelper { public static void SetupProfitsGrid(DataGridView grid) { // Specific column setup } } Problem: Cannot reuse for different grid configurations Solution: Configuration-based approach csharppublic class GridColumnConfig { public string Name { get; set; } public string HeaderText { get; set; } public int Width { get; set; } } public static void SetupGrid(DataGridView grid, List<GridColumnConfig> columns) { // Generic setup } 3. Service Dependencies csharppublic class ApiService { private readonly HttpClient httpClient = new HttpClient(); // Not injectable } Problem: Cannot mock for testing, cannot swap implementations Solution: Dependency Injection csharppublic class ApiService : IApiService { private readonly HttpClient _httpClient; public ApiService(HttpClient httpClient) { _httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient)); } } ``` --- ## 13. 📂 Module Organization & Structure ### Current Organization Issues #### Problems: 1. **Models folder contains domain logic** - `PreparedRecipeData.cs` has formatting methods - Should be pure data 2. **No clear separation of concerns** - UI helpers in UI folder - But Form1 does business orchestration 3. **Utils is a catch-all** - Currency formatting (domain logic) - File I/O (infrastructure) - Custom collections (framework extension) ### Suggested Reorganization ``` GW2CraftingCalculator/ ├── Core/ │ ├── Domain/ │ │ ├── Models/ │ │ │ ├── GW2Item.cs │ │ │ ├── Recipe.cs │ │ │ └── Ingredient.cs │ │ ├── Services/ │ │ │ ├── IRecipeDecomposer.cs │ │ │ ├── IProfitCalculator.cs │ │ │ └── RecipeDecomposer.cs │ │ └── ValueObjects/ │ │ └── GW2Currency.cs │ │ │ ├── Application/ │ │ ├── Interfaces/ │ │ │ ├── IApiClient.cs │ │ │ ├── IDataRepository.cs │ │ │ └── IWorkflowOrchestrator.cs │ │ ├── Services/ │ │ │ ├── RecipeWorkflow.cs │ │ │ └── ProfitWorkflow.cs │ │ └── DTOs/ │ │ ├── PreparedRecipeDto.cs │ │ └── ProfitResultDto.cs │ │ │ └── Infrastructure/ │ ├── Api/ │ │ ├── GW2OfficialApiClient.cs │ │ ├── GW2TPApiClient.cs │ │ └── DataWars2ApiClient.cs │ ├── Persistence/ │ │ ├── JsonFileRepository.cs │ │ └── IngredientRepository.cs │ └── Configuration/ │ ├── AppSettings.cs │ └── ApiConfiguration.cs │ └── Presentation/ ├── WinForms/ │ ├── Forms/ │ │ ├── MainForm.cs │ │ └── MainForm.Designer.cs │ ├── ViewModels/ │ │ ├── IngredientViewModel.cs │ │ ├── RecipeViewModel.cs │ │ └── ProfitViewModel.cs │ ├── Formatters/ │ │ ├── CurrencyFormatter.cs │ │ └── RecipeDisplayFormatter.cs │ └── Controls/ │ ├── GridConfigurator.cs │ └── ProgressReporter.cs │ └── Program.cs Benefits: Clear separation: Domain, Application, Infrastructure, Presentation Testable: Each layer can be tested independently Flexible: Easy to add new presentation layers (Web, Console) Maintainable: Clear where to add new features 14. 🔌 Interface & API Design Missing Abstractions 1. No Service Interfaces Current: csharpprivate readonly ApiService apiService; private readonly RecipeService recipeService; private readonly ProfitCalculator profitCalculator; Should Be: csharpprivate readonly IApiClient _apiClient; private readonly IRecipeProcessor _recipeProcessor; private readonly IProfitCalculator _profitCalculator; Benefits: Mockable for testing Swappable implementations Clear contracts 2. Inconsistent Return Types csharp// Returns empty list on error public async Task<List<GW2Item>> GetAllItems() { if (itemIds == null || itemIds.Count == 0) return new List<GW2Item>(); // Silent failure } // Returns tuple public async Task<(List<Ingredient> availableIngredients, List<Ingredient> allIngredients)> DecomposeRecipe() Better Pattern: csharppublic class Result<T> { public bool Success { get; set; } public T Data { get; set; } public string Error { get; set; } } public async Task<Result<List<GW2Item>>> GetAllItemsAsync() 3. Missing DTOs Models used directly in UI: csharpdataGridView5.DataSource = profitResults; // Domain model directly in UI Should Use DTOs: csharppublic class ProfitResultViewModel { public string ItemName { get; set; } public string SellPrice { get; set; } // Pre-formatted public string Profit { get; set; } // Pre-formatted // UI-specific properties } 15. 🛠️ Refactoring Recommendations Priority Matrix PriorityCategoryEffortRiskImpact🔴 P1Remove Form1.Instance singletonMediumMediumHigh🔴 P1Add dependency injectionHighMediumHigh🔴 P1Fix naming conventions (Polish→English)HighLowMedium🟡 P2Extract business logic from Form1HighMediumHigh🟡 P2Add exception handling strategyMediumLowHigh🟡 P2Externalize configurationLowLowMedium🟢 P3Reduce nesting complexityMediumLowMedium🟢 P3Add service interfacesMediumLowHigh🟢 P3Extract display formatting from modelsMediumLowMedium Detailed Recommendations 🔴 CRITICAL 1: Remove Singleton Anti-Pattern Before: csharp// PreparedRecipeData.cs var availableIds = UI.Form1.Instance?.availableIngredientsIds?.Values.ToHashSet() // Form1.cs public static Form1 Instance { get; private set; } After: csharp// Pass dependencies explicitly public class RecipeDisplayFormatter { private readonly IIngredientRepository _ingredientRepository; public RecipeDisplayFormatter(IIngredientRepository ingredientRepository) { _ingredientRepository = ingredientRepository; } public string FormatIngredients(PreparedRecipeData recipe) { var availableIds = _ingredientRepository.GetAvailableIds(); // formatting logic } } Benefits: Testable, no global state, follows SOLID 🔴 CRITICAL 2: Implement Dependency Injection Create DI Container Setup: csharp// Program.cs using Microsoft.Extensions.DependencyInjection; static class Program { [STAThread] static void Main() { var services = new ServiceCollection(); ConfigureServices(services); var serviceProvider = services.BuildServiceProvider(); ApplicationConfiguration.Initialize(); Application.Run(serviceProvider.GetRequiredService<MainForm>()); } private static void ConfigureServices(IServiceCollection services) { // Configuration services.AddSingleton<IConfiguration>(LoadConfiguration()); // Infrastructure services.AddHttpClient<IGW2ApiClient, GW2ApiClient>(); services.AddSingleton<IFileRepository, JsonFileRepository>(); services.AddSingleton<IIngredientRepository, IngredientRepository>(); // Services services.AddTransient<IRecipeProcessor, RecipeProcessor>(); services.AddTransient<IProfitCalculator, ProfitCalculator>(); services.AddTransient<IWorkflowOrchestrator, WorkflowOrchestrator>(); // UI services.AddTransient<MainForm>(); services.AddTransient<IRecipeFormatter, RecipeDisplayFormatter>(); } } Update Form1: csharppublic partial class MainForm : Form { private readonly IWorkflowOrchestrator _orchestrator; private readonly IRecipeFormatter _formatter; private readonly IIngredientRepository _ingredientRepository; public MainForm( IWorkflowOrchestrator orchestrator, IRecipeFormatter formatter, IIngredientRepository ingredientRepository) { _orchestrator = orchestrator; _formatter = formatter; _ingredientRepository = ingredientRepository; InitializeComponent(); } } Benefits: Testable, loosely coupled, easy to extend 🟡 HIGH 3: Extract Business Logic from Form1 Create Orchestrator: csharppublic interface IWorkflowOrchestrator { Task<Result<List<PreparedRecipeData>>> PrepareRecipesAsync( IProgress<ProgressInfo> progress, CancellationToken cancellationToken); Task<Result<List<ProfitResult>>> CalculateProfitsAsync( List<PreparedRecipeData> recipes, IProgress<ProgressInfo> progress, CancellationToken cancellationToken); } public class WorkflowOrchestrator : IWorkflowOrchestrator { private readonly IGW2ApiClient _apiClient; private readonly IRecipeProcessor _recipeProcessor; private readonly IProfitCalculator _profitCalculator; private readonly IFileRepository _fileRepository; public WorkflowOrchestrator( IGW2ApiClient apiClient, IRecipeProcessor recipeProcessor, IProfitCalculator profitCalculator, IFileRepository fileRepository) { _apiClient = apiClient; _recipeProcessor = recipeProcessor; _profitCalculator = profitCalculator; _fileRepository = fileRepository; } public async Task<Result<List<PreparedRecipeData>>> PrepareRecipesAsync( IProgress<ProgressInfo> progress, CancellationToken cancellationToken) { try { progress?.Report(new ProgressInfo("Loading recipes...")); var recipes = await _fileRepository.LoadRecipesAsync(); progress?.Report(new ProgressInfo("Loading items...")); var items = await _fileRepository.LoadItemsAsync(); // Business logic here, not in Form1 return Result<List<PreparedRecipeData>>.Success(preparedRecipes); } catch (Exception ex) { return Result<List<PreparedRecipeData>>.Failure(ex.Message); } } } Simplified Form1: csharpprivate async void btnPrepareRecipes_Click(object sender, EventArgs e) { var progress = new Progress<ProgressInfo>(info => { lblStatus.Text = info.Message; progressBar.Value = info.Percentage; }); var result = await _orchestrator.PrepareRecipesAsync(progress, CancellationToken.None); if (result.Success) { dgvRecipes.DataSource = result.Data; MessageBox.Show($"Prepared {result.Data.Count} recipes"); } else { MessageBox.Show($"Error: {result.Error}", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error); } } Benefits: Form1 becomes thin UI layer, business logic is testable 🟡 HIGH 4: Add Comprehensive Exception Handling Create Exception Strategy: csharppublic class ApiException : Exception { public ApiException(string message, Exception innerException = null) : base(message, innerException) { } } public class DataException : Exception { public DataException(string message, Exception innerException = null) : base(message, innerException) { } } public interface IExceptionHandler { void Handle(Exception ex); } public class UserFriendlyExceptionHandler : IExceptionHandler { public void Handle(Exception ex) { var message = ex switch { ApiException apiEx => $"API Error: {apiEx.Message}\nPlease check your internet connection.", DataException dataEx => $"Data Error: {dataEx.Message}\nPlease verify your data files.", UnauthorizedAccessException => "Access denied. Please check file permissions.", _ => $"Unexpected error: {ex.Message}" }; MessageBox.Show(message, "Error", MessageBoxButtons.OK, MessageBoxIcon.Error); // Log the full exception Logger.LogError(ex); } } Update ApiService: csharppublic async Task<List<GW2Item>> GetAllItemsAsync() { try { var response = await _httpClient.GetStringAsync(_config.ItemsUrl); var itemIds = JsonSerializer.Deserialize<List<int>>(response); if (itemIds == null || itemIds.Count == 0) throw new ApiException("No items returned from API"); // ... rest of logic return allItems; } catch (HttpRequestException ex) { throw new ApiException("Failed to fetch items from GW2 API", ex); } catch (JsonException ex) { throw new ApiException("Failed to parse API response", ex); } } 🟢 MEDIUM 5: Externalize Configuration Create appsettings.json: json{ "Api": { "GW2": { "BaseUrl": "https://api.guildwars2.com/v2", "Version": "2022-03-09T02:00:00.000Z", "Language": "en", "BatchSize": 200, "DelayMilliseconds": 50 }, "GW2TP": { "BaseUrl": "https://api.gw2tp.com/1" }, "DataWars2": { "BaseUrl": "https://api.datawars2.ie/gw2/v1" } }, "Game": { "TradingPostFeePercent": 15, "TradingPostListingFeePercent": 5 }, "Files": { "DataDirectory": "Data", "RecipesFile": "all_recipes.json", "ItemsFile": "all_items.json", "AvailableIngredientsFile": "available_ingredients_ids.txt" } } Configuration Classes: csharppublic class ApiConfiguration { public GW2ApiConfig GW2 { get; set; } public GW2TPApiConfig GW2TP { get; set; } public DataWars2ApiConfig DataWars2 { get; set; } } public class GW2ApiConfig { public string BaseUrl { get; set; } public string Version { get; set; } public string Language { get; set; } public int BatchSize { get; set; } public int DelayMilliseconds { get; set; } } public class GameConfiguration { public int TradingPostFeePercent { get; set; } public int TradingPostListingFeePercent { get; set; } } Update ApiService: csharppublic class GW2ApiClient : IGW2ApiClient { private readonly HttpClient _httpClient; private readonly GW2ApiConfig _config; public GW2ApiClient(HttpClient httpClient, IOptions<ApiConfiguration> apiConfig) { _httpClient = httpClient; _config = apiConfig.Value.GW2; } public async Task<List<GW2Item>> GetAllItemsAsync() { var url = $"{_config.BaseUrl}/items?v={_config.Version}&lang={_config.Language}"; // ... use _config.BatchSize, _config.DelayMilliseconds } } 🟢 MEDIUM 6: Fix Naming Conventions Step 1: Rename UI Controls (use VS Refactoring tool) csharp// Before button1 → btnFetchItemIds button2 → btnLoadItems button3 → btnLoadRecipes button4 → btnPrepareRecipes button5 → btnCalculateProfits button6 → btnFilterProfits button7 → btnGenerateUnavailable button8 → btnExportSelected dataGridView1 → dgvAvailableIngredients dataGridView2 → dgvPreparedRecipes dataGridView3 → dgvFilteredRecipes dataGridView4 → dgvExcludedIngredients dataGridView5 → dgvProfits dataGridView6 → dgvUnavailableItems tabPage1 → tabAvailableIngredients // ... etc Step 2: Fix Model Properties csharp// Before public class GW2Item { public int id { get; set; } public string name { get; set; } } // After public class GW2Item { [JsonPropertyName("id")] public int Id { get; set; } [JsonPropertyName("name")] public string Name { get; set; } } Step 3: Translate Polish Comments/Strings csharp// Before labelStatus.Text = "Pobieranie danych..."; // Właściwość pomocnicza dla kompatybilności z resztą kodu // After labelStatus.Text = "Loading data..."; // Helper property for compatibility with the rest of the codebase 16. 📋 Implementation Plan Phase 1: Foundation (Week 1) - LOW RISK 🟢 Goal: Prepare infrastructure without breaking existing functionality TaskComplexityRiskFiles AffectedCreate configuration systemSimpleLow+AppSettings.json, +Configuration/Add service interfacesSimpleLow+Interfaces/Set up DI containerModerateLowProgram.csCreate Result<T> patternSimpleLow+Core/Common/ Testing Strategy: Add unit test project Test new interfaces with mocks Verify configuration loading Phase 2: Services Refactoring (Week 2) - MEDIUM RISK 🟡 Goal: Refactor services to use DI and interfaces TaskComplexityRiskFiles AffectedExtract IApiClient interfacesSimpleLowServices/Refactor ApiService to use configModerateMediumApiService.csAdd exception handling to servicesModerateLowAll Services/Create IngredientRepositoryModerateMedium+IngredientRepository.cs, Form1.cs Testing Strategy: Unit tests for each service Integration tests with real APIs (optional) Mock API responses for testing Phase 3: Form1 Decomposition (Week 3) - HIGH RISK 🔴 Goal: Extract business logic from Form1 TaskComplexityRiskFiles AffectedCreate WorkflowOrchestratorComplexHigh+WorkflowOrchestrator.cs, Form1.csExtract display formattingModerateMedium+RecipeDisplayFormatter.cs, PreparedRecipeData.csRemove Form1.Instance singletonModerateHighForm1.cs, PreparedRecipeData.csRefactor button click handlersComplexHighForm1.cs Testing Strategy: Keep old Form1 temporarily as "Form1_Old" Test new MainForm side-by-side Gradual migration, one button at a time Verify UI behavior matches old version Phase 4: Model & Data Layer (Week 4) - MEDIUM RISK 🟡 Goal: Clean up models and data access TaskComplexityRiskFiles AffectedFix model property naming (PascalCase)SimpleMediumAll Models/Create ViewModels for UIModerateLow+ViewModels/Extract formatting from PreparedRecipeDataModerateMediumPreparedRecipeData.cs, +Formatters/Create FileRepositoryModerateLow+FileRepository.cs, FileHelper.cs Testing Strategy: Test JSON deserialization with new names Verify UI binding works with ViewModels Test file repository with temp files Phase 5: Polish & Optimization (Week 5) - LOW RISK 🟢 Goal: Final touches and optimization TaskComplexityRiskFiles AffectedRename all UI controlsSimpleLowForm1.Designer.csTranslate Polish → EnglishSimpleLowAll filesReduce nesting with guard clausesModerateLowRecipeService.cs, Form1.csAdd XML documentationSimpleLowAll public APIs Testing Strategy: Full regression testing User acceptance testing Performance testing Rollback Strategy For Each Phase: Git Branch: Create feature branch for each phase Backup: Keep old code in _Legacy folder Feature Flags: Use conditional compilation for major changes csharp#if USE_NEW_ORCHESTRATOR var orchestrator = serviceProvider.GetRequiredService<IWorkflowOrchestrator>(); #else // Old inline code #endif Incremental: Deploy one tab at a time, not all at once Testing Checklist Before Deployment: All unit tests pass Integration tests with real APIs work UI matches old behavior Performance is equal or better No memory leaks Exception handling works Configuration loads correctly File I/O works on different paths DataGridView sorting works Progress bars update correctly 📊 File Impact Matrix FileP1 ChangesP2 ChangesP3 ChangesTotal ImpactForm1.cs🔴🔴🔴🟡🟡🟢CRITICALPreparedRecipeData.cs🔴🔴🟡🟢HIGHApiService.cs🔴🟡🟡🟢HIGHRecipeService.cs🔴🟡🟢🟢MEDIUMProfitCalculator.cs🔴🟡🟢MEDIUMProgram.cs🔴--MEDIUMAll Models-🟡🟢🟢MEDIUMFileHelper.cs-🟡🟢LOWDataGridViewHelper.cs--🟢🟢LOW 🎯 Summary & Quick Wins Immediate Actions (< 1 Day) Add appsettings.json - Externalize all magic numbers and URLs Create service interfaces - Prepare for DI Rename buttons - Use descriptive names Add XML docs - Document public APIs Extract constants - Create GameConstants class Week 1 Goals Set up DI container Create all service interfaces Add configuration system Begin unit test project Success Metrics Code Coverage: > 70% for business logic Cyclomatic Complexity: < 10 for all methods Maintainability Index: > 80 Technical Debt: Reduced by 50% 🚨 Critical Warnings DO NOT DON'T refactor everything at once DON'T change UI and business logic simultaneously DON'T deploy without thorough testing DON'T remove old code until new code is proven DO DO use feature branches DO write tests first for critical paths DO refactor incrementally DO keep communication open with users DO maintain backwards compatibility during transition This comprehensive analysis provides a roadmap for transforming the GW2 Crafting Calculator from a tightly-coupled Windows Forms application into a maintainable, testable, and extensible solution following SOLID principles and clean code best practices. The phased approach ensures minimal risk while maximizing long-term benefits.