refactoring
This commit is contained in:
EugeneTes
262
plan.md
Normal file
262
plan.md
Normal file
@@ -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<EpsonPrintServiceConfiguration>(...);
|
||||
// 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
|
||||
Reference in New Issue
Block a user