diff --git a/.claude/settings.local.json b/.claude/settings.local.json new file mode 100644 index 0000000..4a1edf1 --- /dev/null +++ b/.claude/settings.local.json @@ -0,0 +1,7 @@ +{ + "permissions": { + "allow": [ + "Bash(grep:*)" + ] + } +} diff --git a/EpsonPrintService/HeartbeatBackgroundTask.cs b/EpsonPrintService/HeartbeatBackgroundTask.cs index e3c4294..4de60c3 100644 --- a/EpsonPrintService/HeartbeatBackgroundTask.cs +++ b/EpsonPrintService/HeartbeatBackgroundTask.cs @@ -17,11 +17,12 @@ public class HeartbeatBackgroundTask public HeartbeatBackgroundTask( EpsonPrintServiceConfiguration configuration, - ILogger logger) + ILogger logger, + HttpClient httpClient) { _configuration = configuration; _logger = logger; - _httpClient = new HttpClient(); + _httpClient = httpClient; _interval = TimeSpan.FromSeconds(30); } diff --git a/EpsonPrintService/Program.cs b/EpsonPrintService/Program.cs index 2d4f8fc..2231bec 100644 --- a/EpsonPrintService/Program.cs +++ b/EpsonPrintService/Program.cs @@ -21,6 +21,7 @@ var config = JsonSerializer.Deserialize( File.ReadAllText("config.json")); kernel.Bind().To().InSingletonScope(); +kernel.Bind().ToConstant(new HttpClient { Timeout = TimeSpan.FromSeconds(30) }).InSingletonScope(); kernel.Bind().ToConstant(config); diff --git a/Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs b/Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs index 63f004a..d4fbbd3 100644 --- a/Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs +++ b/Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs @@ -11,17 +11,19 @@ public class EpsonPrintService: IPrintService private readonly IWrapperPrinterFactory _wrapperPrinterFactory; private readonly IPrinterConfigurationSource _printerConfigurationSource; private readonly IReceiptConverterFactory _receiptConverterFactory; + private readonly HttpClient _httpClient; - public EpsonPrintService(ILogger logger, IPrinterFactory printerFactory, IWrapperPrinterFactory wrapperPrinterFactory, IPrinterConfigurationSource printerConfigurationSource, IReceiptConverterFactory receiptConverterFactory) + public EpsonPrintService(ILogger logger, IPrinterFactory printerFactory, IWrapperPrinterFactory wrapperPrinterFactory, IPrinterConfigurationSource printerConfigurationSource, IReceiptConverterFactory receiptConverterFactory, HttpClient httpClient) { _logger = logger; _printerFactory = printerFactory; _wrapperPrinterFactory = wrapperPrinterFactory; _printerConfigurationSource = printerConfigurationSource; _receiptConverterFactory = receiptConverterFactory; + _httpClient = httpClient; } - public async Task PrintAsync(string printerIp, PrintJob job) + public async Task PrintAsync(string printerIp, PrintJob job) { try { @@ -65,10 +67,7 @@ public class EpsonPrintService: IPrintService // Download only if not already cached if (!File.Exists(cachedLogoPath)) { - using var httpClient = new HttpClient(); - httpClient.Timeout = TimeSpan.FromSeconds(10); - - var logoData = await httpClient.GetByteArrayAsync(job.Document.LogoUrl); + var logoData = await _httpClient.GetByteArrayAsync(job.Document.LogoUrl); await File.WriteAllBytesAsync(cachedLogoPath, logoData); _logger.LogInformation("Downloaded logo {LogoFileName} from {LogoUrl}", logoFileName, job.Document.LogoUrl); @@ -84,8 +83,18 @@ public class EpsonPrintService: IPrintService await printerAdapter.SetFontSizeAsync(configuration.FontSize); // Convert receipt content to print commands using the factory - var converter = _receiptConverterFactory.Create(job.Document.ReceiptType, printerId.Value); - var commands = converter.Convert(job.Document.Content); + List commands; + try + { + var converter = _receiptConverterFactory.Create(job.Document.ReceiptType, printerId.Value); + commands = converter.Convert(job.Document.Content); + } + catch (Exception e) + { + _logger.LogWarning(e, "Conversion error for print job on printer {PrinterUid}", printerIp); + return PrintResult.Fail(PrintErrorType.ConversionError); + } + await printerAdapter.PrintAsync(commands); await Task.Delay(200); status = await epsonPrinter.GetPrinterStatusAsync(); @@ -98,10 +107,10 @@ public class EpsonPrintService: IPrintService } catch (Exception e) { - _logger.LogWarning( e, "Failed to print job for printer {PrinterUid}", printerIp); - return false; + _logger.LogWarning(e, "Failed to print job for printer {PrinterUid}", printerIp); + return PrintResult.Fail(PrintErrorType.Other); } - return true; + return PrintResult.Ok(); } } \ No newline at end of file diff --git a/Inspectron.Epson/Queue/IPrintService.cs b/Inspectron.Epson/Queue/IPrintService.cs index 3cc40ce..a4a4125 100644 --- a/Inspectron.Epson/Queue/IPrintService.cs +++ b/Inspectron.Epson/Queue/IPrintService.cs @@ -2,5 +2,5 @@ using System.Threading.Tasks; public interface IPrintService { - Task PrintAsync(string printerIp, PrintJob job); + Task PrintAsync(string printerIp, PrintJob job); } diff --git a/Inspectron.Epson/Queue/PrintErrorType.cs b/Inspectron.Epson/Queue/PrintErrorType.cs new file mode 100644 index 0000000..036e318 --- /dev/null +++ b/Inspectron.Epson/Queue/PrintErrorType.cs @@ -0,0 +1,6 @@ +public enum PrintErrorType +{ + None, + ConversionError, + Other +} diff --git a/Inspectron.Epson/Queue/PrintResult.cs b/Inspectron.Epson/Queue/PrintResult.cs new file mode 100644 index 0000000..fb12e5f --- /dev/null +++ b/Inspectron.Epson/Queue/PrintResult.cs @@ -0,0 +1,14 @@ +public class PrintResult +{ + public bool Success { get; } + public PrintErrorType ErrorType { get; } + + private PrintResult(bool success, PrintErrorType errorType) + { + Success = success; + ErrorType = errorType; + } + + public static PrintResult Ok() => new(true, PrintErrorType.None); + public static PrintResult Fail(PrintErrorType errorType) => new(false, errorType); +} diff --git a/Inspectron.Epson/Queue/PrintServer.cs b/Inspectron.Epson/Queue/PrintServer.cs index e0cb094..6fdc56e 100644 --- a/Inspectron.Epson/Queue/PrintServer.cs +++ b/Inspectron.Epson/Queue/PrintServer.cs @@ -1,36 +1,35 @@ -using Microsoft.Extensions.Logging; +using System.Collections.Concurrent; +using Microsoft.Extensions.Logging; public class PrintServer { private readonly IPrintService _printService; private readonly ILogger _logger; - private readonly Dictionary _printerQueues; + private readonly ConcurrentDictionary _printerQueues; public PrintServer(IPrintService printService, ILogger logger) { _printService = printService; _logger = logger; - _printerQueues = new Dictionary(); + _printerQueues = new ConcurrentDictionary(); } public void RegisterPrinter(string printerIp) { - if (_printerQueues.ContainsKey(printerIp)) - return; - - var queue = new PrinterQueue(printerIp, _printService,_logger); - _printerQueues[printerIp] = queue; - queue.Start(); - Console.WriteLine($"Printer {printerIp} registered"); + var queue = new PrinterQueue(printerIp, _printService, _logger); + if (_printerQueues.TryAdd(printerIp, queue)) + { + queue.Start(); + Console.WriteLine($"Printer {printerIp} registered"); + } } public void UnregisterPrinter(string printerIp) { - if (_printerQueues.TryGetValue(printerIp, out var queue)) + if (_printerQueues.TryRemove(printerIp, out var queue)) { queue.StopAsync().Wait(); - _printerQueues.Remove(printerIp); Console.WriteLine($"Printer {printerIp} unregistered"); } } diff --git a/Inspectron.Epson/Queue/PrinterQueue.cs b/Inspectron.Epson/Queue/PrinterQueue.cs index 1c4ff50..24f8410 100644 --- a/Inspectron.Epson/Queue/PrinterQueue.cs +++ b/Inspectron.Epson/Queue/PrinterQueue.cs @@ -73,15 +73,22 @@ public class PrinterQueue try { // Call the actual print function - bool success = await _printService.PrintAsync(PrinterIp, job); + var result = await _printService.PrintAsync(PrinterIp, job); - if (!success) + if (!result.Success) { - // Re-queue with retry logic - job.RetryCount++; - _logger.LogWarning("Job failed, retrying ({RetryCount}/3)", job.RetryCount); - await Task.Delay(5000, cancellationToken); // Wait before retry - EnqueuePriority(job); + if (result.ErrorType == PrintErrorType.ConversionError) + { + _logger.LogWarning("Job failed due to conversion error, not re-queuing"); + } + else + { + // Re-queue with retry logic + job.RetryCount++; + _logger.LogWarning("Job failed, retrying ({RetryCount}/3)", job.RetryCount); + await Task.Delay(5000, cancellationToken); // Wait before retry + EnqueuePriority(job); + } } else { diff --git a/JUNIOR_ENGINEER_TASKS.md b/JUNIOR_ENGINEER_TASKS.md new file mode 100644 index 0000000..7b9bbd1 --- /dev/null +++ b/JUNIOR_ENGINEER_TASKS.md @@ -0,0 +1,454 @@ +# Junior Engineer Task List - Code Improvements + +This document contains actionable tasks derived from the code analysis in `plan.md`. Complete tasks in priority order. After each fix, run: +```bash +dotnet build Inspectron.Epson.slnx +dotnet test EpsonTest/EpsonTest.csproj +``` + +--- + +## CRITICAL PRIORITY (Fix First) + +### Task 1: Move API Key to Environment Variable +**File:** `EpsonPrintService/config.json:4` + +**Problem:** API key stored in plaintext - security risk. + +**Steps:** +1. Remove `ApiKey` from `config.json` +2. In `EpsonPrintService/Program.cs`, read from environment: +```csharp +var apiKey = Environment.GetEnvironmentVariable("EPSON_API_KEY") + ?? throw new InvalidOperationException("EPSON_API_KEY environment variable not set"); +``` +3. Update `EpsonPrintServiceConfiguration` class to accept apiKey via constructor or property +4. Add to systemd service file `epson.service`: +```ini +[Service] +Environment="EPSON_API_KEY=your-key-here" +``` +5. Document the required environment variable in README + +--- + +### Task 2: Fix HttpClient Anti-Pattern +**Files:** +- `EpsonPrintService/HeartbeatBackgroundTask.cs:24` +- `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:68` + +**Problem:** Creating new HttpClient per request causes socket exhaustion. + +**Steps:** +1. In `HeartbeatBackgroundTask.cs`: + - Change constructor to accept `HttpClient` as parameter + - Remove `_httpClient = new HttpClient();` line + - Keep using `_httpClient` field + +2. In `Program.cs` where HeartbeatBackgroundTask is created: +```csharp +// Create ONE HttpClient instance for the app lifetime +var httpClient = new HttpClient(); +var heartbeatTask = new HeartbeatBackgroundTask(httpClient, ...); +``` + +3. In `EpsonPrintService.cs:68`: + - Add `HttpClient _httpClient` field to class + - Inject via constructor + - Replace `using var httpClient = new HttpClient();` with `_httpClient` + +--- + +### Task 3: Add Graceful Shutdown +**File:** `EpsonPrintService/Program.cs:51-82` + +**Problem:** PrintLoop and PrintServer never call StopAsync() - jobs lost on Ctrl+C. + +**Steps:** +1. Store references to started services: +```csharp +var printLoop = kernel.Get(); +var printServer = kernel.Get(); +``` + +2. In the Console.CancelKeyPress handler (or before Environment.Exit): +```csharp +Console.CancelKeyPress += async (sender, e) => +{ + e.Cancel = true; // Prevent immediate exit + Console.WriteLine("Shutting down gracefully..."); + + await printLoop.StopAsync(); + await printServer.StopAsync(); + + cts.Cancel(); +}; +``` + +3. Ensure `PrintLoop` and `PrintServer` have `StopAsync()` methods that properly clean up + +--- + +### Task 4: Add Null Check for PrinterId +**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:35-44` + +**Problem:** `GetPrinterIdAsync()` can return null but `.Value` accessed without check. + +**Steps:** +1. Find this code block: +```csharp +var printerId = await epsonPrinter.GetPrinterIdAsync(); +var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...); +``` + +2. Add null check: +```csharp +var printerId = await epsonPrinter.GetPrinterIdAsync(); +if (printerId == null || !printerId.HasValue) +{ + _logger.LogError("Failed to get printer ID from {PrinterIp}", printerIp); + return false; +} +var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...); +``` + +--- + +### Task 5: Fix Race Condition in PrintServer +**File:** `Inspectron.Epson/Queue/PrintServer.cs:19-23` + +**Problem:** TOCTOU bug - two threads could create duplicate queues. + +**Steps:** +1. Change dictionary type from `Dictionary` to: +```csharp +private readonly ConcurrentDictionary _printerQueues = new(); +``` + +2. Replace this pattern: +```csharp +if (_printerQueues.ContainsKey(printerIp)) + return; +var queue = new PrinterQueue(...); +_printerQueues[printerIp] = queue; +``` + +With atomic operation: +```csharp +var queue = _printerQueues.GetOrAdd(printerIp, ip => +{ + var newQueue = new PrinterQueue(ip, _printService, _logger); + newQueue.StartAsync(); // Start the queue + Console.WriteLine($"Printer {ip} registered"); + return newQueue; +}); +``` + +3. Add `using System.Collections.Concurrent;` at top of file + +--- + +## HIGH PRIORITY + +### Task 6: Add CancellationToken Support +**File:** `Inspectron.Epson/EpsonPrinter.cs` - Lines 52, 96, 129, 205, 335, 408, etc. + +**Problem:** Async methods can't be cancelled by callers. + +**Steps:** +1. Find all public async methods in EpsonPrinter.cs +2. Add optional parameter to each: +```csharp +// Before: +public async Task ConnectAsync(string host, int port = 9100) + +// After: +public async Task ConnectAsync(string host, int port = 9100, CancellationToken cancellationToken = default) +``` + +3. Pass cancellationToken to internal async calls: +```csharp +await _stream.WriteAsync(data, cancellationToken); +await _stream.ReadAsync(buffer, cancellationToken); +``` + +4. Update interface `IEpsonPrinter` if one exists + +--- + +### Task 7: Add Exception Handling to Fire-and-Forget Tasks +**Files:** +- `Inspectron.Epson/PrintServer/PrintLoop.cs:29` +- `EpsonPrintService/Program.cs:71-72` + +**Problem:** Unobserved exceptions crash the process. + +**Steps:** +1. In `PrintLoop.cs`, replace: +```csharp +_ = Task.Run(Loop); +``` +With: +```csharp +_ = Task.Run(async () => +{ + try + { + await Loop(); + } + catch (Exception ex) + { + _logger.LogError(ex, "PrintLoop crashed unexpectedly"); + } +}); +``` + +2. In `Program.cs`, replace: +```csharp +_ = discoveryTask.StartAsync(cts.Token); +_ = heartbeatTask.StartAsync(cts.Token); +``` +With: +```csharp +var discoveryRunning = Task.Run(async () => +{ + try { await discoveryTask.StartAsync(cts.Token); } + catch (Exception ex) { Console.WriteLine($"Discovery error: {ex.Message}"); } +}); + +var heartbeatRunning = Task.Run(async () => +{ + try { await heartbeatTask.StartAsync(cts.Token); } + catch (Exception ex) { Console.WriteLine($"Heartbeat error: {ex.Message}"); } +}); +``` + +--- + +### Task 8: Implement IDisposable for Resource Cleanup +**Files:** +- `EpsonPrintService/HeartbeatBackgroundTask.cs` +- `Inspectron.Epson/Queue/PrinterQueue.cs:13-14` +- `Inspectron.Epson/EpsonPrinter.cs:757-761` + +**Steps:** + +1. **HeartbeatBackgroundTask.cs** - Add IDisposable: +```csharp +public class HeartbeatBackgroundTask : IDisposable +{ + private bool _disposed; + + public void Dispose() + { + if (_disposed) return; + _httpClient?.Dispose(); + _disposed = true; + } +} +``` + +2. **PrinterQueue.cs** - Add IDisposable: +```csharp +public class PrinterQueue : IDisposable +{ + private bool _disposed; + + public void Dispose() + { + if (_disposed) return; + _semaphore?.Dispose(); + _cts?.Dispose(); + _disposed = true; + } +} +``` + +3. **EpsonPrinter.cs** - Fix DisposeAsync: +```csharp +public async ValueTask DisposeAsync() +{ + Disconnect(); + GC.SuppressFinalize(this); + await Task.CompletedTask; +} +``` + +--- + +### Task 9: Replace .Wait() with await +**File:** `Inspectron.Epson/Queue/PrintServer.cs:32` + +**Problem:** `.Wait()` can cause deadlocks. + +**Steps:** +1. Find method containing: +```csharp +queue.StopAsync().Wait(); +``` + +2. Make the method async: +```csharp +// Before: +public void StopQueue(string printerIp) + +// After: +public async Task StopQueueAsync(string printerIp) +``` + +3. Replace `.Wait()` with `await`: +```csharp +await queue.StopAsync(); +``` + +4. Update all callers to use `await StopQueueAsync()` + +--- + +### Task 10: Add Configuration Validation at Startup +**File:** `EpsonPrintService/Program.cs:20-21` + +**Problem:** No validation after deserializing config. + +**Steps:** +1. After config deserialization, add validation: +```csharp +var config = JsonSerializer.Deserialize(configJson); + +if (config == null) + throw new InvalidOperationException("Failed to parse config.json"); + +if (string.IsNullOrEmpty(config.GroupId)) + throw new InvalidOperationException("GroupId is required in config.json"); + +if (string.IsNullOrEmpty(config.RestaurantId)) + throw new InvalidOperationException("RestaurantId is required in config.json"); + +if (config.PrinterConfigurations == null || config.PrinterConfigurations.Count == 0) + Console.WriteLine("Warning: No printer configurations found"); +``` + +--- + +## MEDIUM PRIORITY + +### Task 11: Replace Console.WriteLine with ILogger +**File:** `Inspectron.Epson/Queue/PrintServer.cs:25, 34, 64` + +Replace all `Console.WriteLine()` calls with proper logging: +```csharp +// Before: +Console.WriteLine($"Printer {printerIp} registered"); + +// After: +_logger.LogInformation("Printer {PrinterIp} registered", printerIp); +``` + +--- + +### Task 12: Fix Logger Factory Singleton +**File:** `EpsonPrintService/Program.cs:33-40` + +Create factory once: +```csharp +// Create once at startup +using var loggerFactory = LoggerFactory.Create(builder => +{ + builder.AddConsole(); + builder.SetMinimumLevel(LogLevel.Debug); +}); + +// Bind as singleton for all injections +kernel.Bind().ToConstant(loggerFactory); +kernel.Bind(typeof(ILogger<>)).ToMethod(ctx => + loggerFactory.CreateLogger(ctx.Request.Service.GetGenericArguments()[0])); +``` + +--- + +### Task 13: Extract Magic Numbers to Constants +**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:87-88` + +```csharp +// Add at top of class: +private const int MaxRetryCount = 3; + +// Replace: +if (job.RetryCount >= 3) +// With: +if (job.RetryCount >= MaxRetryCount) +``` + +--- + +### Task 14: Fix Static Thread Safety +**File:** `Inspectron.Epson/EpsonPrinter.cs:38-43` + +```csharp +private static readonly object _initLock = new(); +private static bool _initialized; + +private static void RegisterCodepages() +{ + if (_initialized) return; + + lock (_initLock) + { + if (_initialized) return; // Double-check pattern + Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); + _initialized = true; + } +} +``` + +--- + +## LOW PRIORITY + +### Task 15: Remove Commented-Out Code +- `EpsonPrintService/Program.cs:27-28` - Delete old factory bindings +- `Inspectron.Epson/PrintServer/Printers/TM-T30III.cs:25` - Delete dead code + +### Task 16: Resolve TODO Comments +**File:** `Inspectron.Epson/Queue/PrintJob.cs:5` +- Either change `IP` property to proper type or remove the TODO comment + +### Task 17: Remove Unused Serilog Packages +**File:** `EpsonPrintService/EpsonPrintService.csproj:14-16` +```bash +dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog +dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog.Extensions.Logging +dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog.Sinks.Console +``` + +### Task 18: Fix Inconsistent Logging Style +Change string interpolation to structured logging: +```csharp +// Before: +_logger.LogWarning($"Printer {printerIp} not found"); + +// After: +_logger.LogWarning("Printer {PrinterIp} not found", printerIp); +``` + +--- + +## Verification Checklist + +After completing all tasks: + +- [ ] `dotnet build Inspectron.Epson.slnx` - No errors +- [ ] `dotnet test EpsonTest/EpsonTest.csproj` - All tests pass +- [ ] Test printer connectivity manually +- [ ] Verify systemd service starts/stops cleanly +- [ ] Verify graceful shutdown with Ctrl+C +- [ ] Check no Console.WriteLine in library code (only logging) + +--- + +## Questions? + +If you're unsure about any task: +1. Read the original issue in `plan.md` for more context +2. Look at existing patterns in the codebase +3. Ask a senior engineer before making architectural changes diff --git a/plan.md b/plan.md new file mode 100644 index 0000000..51f5d8a --- /dev/null +++ b/plan.md @@ -0,0 +1,262 @@ +# Code Improvement Analysis: EpsonPrintService & Inspectron.Epson + +## Executive Summary + +Analysis of both projects revealed **50+ distinct issues** across security, reliability, performance, and maintainability. The most critical issues involve resource management, thread safety, and missing cancellation support. + +--- + +## Critical Issues (Fix Immediately) + +### 1. Security: Plaintext API Key +**File:** `EpsonPrintService/config.json:4` +```json +"ApiKey": "Pd8X/VtXLWgl5Djy4ksjdHC3xboeoh5Jig3Qo4277GQ=" +``` +- API key stored in plaintext JSON file +- **Fix:** Move to environment variables or secure configuration provider + +### 2. HttpClient Anti-Pattern (Socket Exhaustion Risk) +**File:** `EpsonPrintService/HeartbeatBackgroundTask.cs:24` +```csharp +_httpClient = new HttpClient(); // Created per instance +``` +**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:68` +```csharp +using var httpClient = new HttpClient(); // Created per request +``` +- **Fix:** Inject singleton HttpClient or use IHttpClientFactory + +### 3. Missing Graceful Shutdown +**File:** `EpsonPrintService/Program.cs:51-82` +- `PrintLoop` and `PrintServer` never call `StopAsync()` on exit +- Queued print jobs are lost on Ctrl+C +- **Fix:** Store references and call shutdown methods before exiting + +### 4. NullReferenceException Risk +**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:35-44` +```csharp +var printerId = await epsonPrinter.GetPrinterIdAsync(); +var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...); +``` +- `GetPrinterIdAsync()` can return null but `.Value` is called without check +- **Fix:** Add null check before accessing `.Value` + +### 5. Thread Safety: Race Condition in PrintServer +**File:** `Inspectron.Epson/Queue/PrintServer.cs:19-23` +```csharp +if (_printerQueues.ContainsKey(printerIp)) + return; +var queue = new PrinterQueue(...); +_printerQueues[printerIp] = queue; +``` +- TOCTOU bug: two threads could create duplicate queues +- **Fix:** Use `ConcurrentDictionary.GetOrAdd()` or `TryAdd()` + +--- + +## High Priority Issues + +### 6. Missing CancellationToken Support +**File:** `Inspectron.Epson/EpsonPrinter.cs` - Lines 52, 96, 129, 205, 335, 408, etc. +- Core async methods don't accept CancellationToken +- Callers cannot cancel long-running operations +- **Fix:** Add optional `CancellationToken cancellationToken = default` to all async methods + +### 7. Fire-and-Forget Tasks Without Exception Handling +**File:** `Inspectron.Epson/PrintServer/PrintLoop.cs:29` +```csharp +_= Task.Run(Loop); // No exception handling +``` +**File:** `EpsonPrintService/Program.cs:71-72` +```csharp +_ = discoveryTask.StartAsync(cts.Token); +_ = heartbeatTask.StartAsync(cts.Token); +``` +- Unobserved exceptions will terminate the process +- **Fix:** Use exception handler or track tasks for proper completion + +### 8. Resource Leaks (IDisposable Not Implemented) +**File:** `EpsonPrintService/HeartbeatBackgroundTask.cs` - No IDisposable +- HttpClient never disposed + +**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:13-14` +- SemaphoreSlim and CancellationTokenSource never disposed + +**File:** `Inspectron.Epson/EpsonPrinter.cs:757-761` +```csharp +public async ValueTask DisposeAsync() +{ + Disconnect(); + await Task.CompletedTask; // Missing GC.SuppressFinalize +} +``` +- **Fix:** Implement proper disposal pattern with try/finally + +### 9. Synchronous Wait in Async Context +**File:** `Inspectron.Epson/Queue/PrintServer.cs:32` +```csharp +queue.StopAsync().Wait(); // Deadlock risk +``` +- **Fix:** Use `await` or proper async pattern + +### 10. Configuration Not Validated at Startup +**File:** `EpsonPrintService/Program.cs:20-21` +```csharp +var config = JsonSerializer.Deserialize(...); +// No null check or validation +``` +- **Fix:** Validate all required fields immediately after deserialization + +--- + +## Medium Priority Issues + +### 11. Console.WriteLine in Library Code +**File:** `Inspectron.Epson/Queue/PrintServer.cs:25, 34, 64` +```csharp +Console.WriteLine($"Printer {printerIp} registered"); +``` +- Library code should use ILogger only +- **Fix:** Replace with `_logger.LogInformation()` + +### 12. Logger Factory Created Repeatedly +**File:** `EpsonPrintService/Program.cs:33-40` +- Creates new LoggerFactory for every ILogger injection +- **Fix:** Create factory once and bind as singleton + +### 13. Magic Numbers Throughout +**File:** `Inspectron.Epson/EpsonPrinter.cs:263, 273, 282, 327, 463` +- Hard-coded ESC/POS bytes scattered in code +- **Fix:** Move to `EpsonCommands` static class + +**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:87-88` +```csharp +if (job.RetryCount >= 3) // Magic number +``` +- **Fix:** Extract to configurable constant + +### 14. SOLID Violations + +**SRP Violation - EpsonPrinter.cs (763 lines)** +- Handles: connection, status, image processing, text printing, cutting +- **Fix:** Split into smaller classes (ConnectionManager, StatusProvider, ImagePrinter) + +**SRP Violation - EpsonPrintServiceConfiguration** +- Implements both `IPrinterConfigurationSource` AND `IAssignedPrinterRepository` +- **Fix:** Separate into two classes + +### 15. Code Duplication + +**Image Conversion:** +- `ConvertToRasterData()` and `ConvertToColumnFormat()` share 50%+ logic +- **Fix:** Extract common image processing pipeline + +**Image Loading:** +- Same file loading pattern repeated 4 times in EpsonPrinter.cs +- **Fix:** Create private helper method + +### 16. Performance Issue in Image Conversion +**File:** `Inspectron.Epson/EpsonImageConverter.cs:243-247` +```csharp +image.ProcessPixelRows(accessor => { ... }); // Called per pixel! +``` +- O(n²) performance instead of O(n) +- **Fix:** Call ProcessPixelRows once and cache accessor + +### 17. Missing Input Validation +- `ConnectAsync()` doesn't validate IP format +- `PrintTextAsync()` doesn't validate text encoding +- `FeedLinesAsync()` doesn't validate line count > 0 +- **Fix:** Add guard clauses with ArgumentException + +### 18. Static State Thread Safety +**File:** `Inspectron.Epson/EpsonPrinter.cs:38-43` +```csharp +private static bool _initialized; +private static void RegisterCodepages() +{ + if (_initialized) return; // No lock! + Encoding.RegisterProvider(...); + _initialized = true; +} +``` +- **Fix:** Use lock or `Interlocked.CompareExchange()` + +--- + +## Low Priority Issues + +### 19. Commented-Out Code +- `EpsonPrintService/Program.cs:27-28` - Old factory bindings +- `Inspectron.Epson/PrintServer/Printers/TM-T30III.cs:25` - Dead code +- **Fix:** Remove or track in issue tracker + +### 20. TODO Comments Left in Code +**File:** `Inspectron.Epson/Queue/PrintJob.cs:5` +```csharp +// todo: change to ip address +public string IP { get; set; } +``` +- **Fix:** Resolve or create proper issue + +### 21. Unused Serilog Dependencies +**File:** `EpsonPrintService/EpsonPrintService.csproj:14-16` +- Serilog packages referenced but not used +- **Fix:** Remove unused packages + +### 22. Inconsistent Logging +- String interpolation used instead of structured logging placeholders +- Log levels inconsistent (LogWarning for config errors that should fail startup) +- **Fix:** Use `LogWarning("Printer {PrinterId} not found", printerIp)` pattern + +### 23. API Design Inconsistencies +- `PrintTextAsync()` vs `PrintTextAndCutAsync()` naming +- 3 overloads of `PrintImageBitModeAsync()` and `LoadImageAsync()` +- Interface doesn't match all implementation overloads +- **Fix:** Rationalize method naming and signatures + +--- + +## Suggested Refactoring Projects + +### Project 1: Resource Management Overhaul +- Implement IAsyncDisposable properly across all classes +- Fix all HttpClient instantiation +- Add proper cleanup on shutdown +- **Scope:** ~10 files, ~200 lines changed + +### Project 2: Async Pattern Modernization +- Add CancellationToken to all async methods +- Fix fire-and-forget patterns +- Replace synchronous waits +- **Scope:** ~8 files, ~150 lines changed + +### Project 3: Thread Safety Fixes +- Fix race conditions in PrintServer and PrinterQueue +- Add proper locking for static initialization +- **Scope:** ~4 files, ~50 lines changed + +### Project 4: Split EpsonPrinter Class +- Extract ConnectionManager +- Extract StatusProvider +- Extract ImagePrinter +- Keep EpsonPrinter as facade +- **Scope:** ~5 new files, ~800 lines reorganized + +### Project 5: Configuration & DI Cleanup +- Add configuration validation +- Fix Logger factory singleton +- Separate configuration concerns +- Move secrets to environment variables +- **Scope:** ~5 files, ~100 lines changed + +--- + +## Verification + +After implementing any changes: +1. Build the solution: `dotnet build Inspectron.Epson.slnx` +2. Run existing tests: `dotnet test EpsonTest/EpsonTest.csproj` +3. Test printer connectivity manually with EpsonTest project +4. Verify systemd service starts/stops cleanly