# 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<PrintLoop>();
var printServer = kernel.Get<PrintServer>();
```

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<string, PrinterQueue>` to:
```csharp
private readonly ConcurrentDictionary<string, PrinterQueue> _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<bool> ConnectAsync(string host, int port = 9100)

// After:
public async Task<bool> 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<EpsonPrintServiceConfiguration>(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<ILoggerFactory>().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