juanatsap
d95c62bad4
Remove 557-line server-design.md from _go-learning/architecture - content is now covered in updated architecture documentation with real implementation examples and test coverage.
506 lines
16 KiB
Markdown
506 lines
16 KiB
Markdown
# Refactoring #4: Handler Improvements - Quality, Type Safety & Testing
|
||
|
||
**Date**: 2024-11-20
|
||
**Type**: Code Quality, Type Safety, Testing, Architecture
|
||
|
||
## Problem Statement
|
||
|
||
After splitting the monolithic handler (Refactoring #3), several opportunities for improvement remained:
|
||
|
||
1. **Broken Pre-Commit Hook**: Regex pattern incompatible with Go's RE2 engine
|
||
2. **Code Duplication**: `Home()` and `CVContent()` duplicated 60+ lines of data preparation
|
||
3. **Weak Type Safety**: Manual query parameter parsing with repetitive validation
|
||
4. **No Middleware**: Cookie handling duplicated across handlers
|
||
5. **Missing Tests**: No tests for page and HTMX handlers (only PDF/security tests)
|
||
|
||
## Solution
|
||
|
||
Implemented five complementary improvements in a single comprehensive refactoring:
|
||
|
||
### 1. Fix Pre-Commit Hook (5 min)
|
||
|
||
**Problem**: Hook used Perl-style negative lookahead `(?!PDF)` unsupported by Go
|
||
**Fix**: Remove regex filter - PDF tests already marked with `+build integration` tag
|
||
|
||
```bash
|
||
# Before (BROKEN)
|
||
go test -short -run '^((?!PDF).)*$' ./... # ❌ Fails with regex error
|
||
|
||
# After (WORKING)
|
||
go test -short ./... # ✅ Integration tests excluded by default
|
||
```
|
||
|
||
### 2. Extract Duplicate Logic (15 min)
|
||
|
||
**Problem**: `Home()` and `CVContent()` duplicated data preparation
|
||
**Solution**: Use existing `prepareTemplateData()` helper
|
||
|
||
**Before** (60 lines duplicated × 2 = 120 lines):
|
||
```go
|
||
func (h *CVHandler) Home(w http.ResponseWriter, r *http.Request) {
|
||
// Load CV data
|
||
cv, err := cvmodel.LoadCV(lang)
|
||
// ...
|
||
// Load UI translations
|
||
ui, err := uimodel.LoadUI(lang)
|
||
// ...
|
||
// Calculate durations
|
||
for i := range cv.Experience {
|
||
cv.Experience[i].Duration = calculateDuration(...)
|
||
}
|
||
// ... 50 more lines
|
||
}
|
||
|
||
func (h *CVHandler) CVContent(w http.ResponseWriter, r *http.Request) {
|
||
// IDENTICAL 60 lines duplicated!
|
||
}
|
||
```
|
||
|
||
**After** (10 lines each):
|
||
```go
|
||
func (h *CVHandler) Home(w http.ResponseWriter, r *http.Request) {
|
||
// Prepare template data using shared helper
|
||
data, err := h.prepareTemplateData(lang)
|
||
if err != nil {
|
||
HandleError(w, r, DataLoadError(err, "CV"))
|
||
return
|
||
}
|
||
|
||
// Add preference-specific fields
|
||
data["CVLengthClass"] = cvLengthClass
|
||
data["ShowIcons"] = (cvIcons == "show")
|
||
data["ThemeClean"] = (cvTheme == "clean")
|
||
// ...
|
||
}
|
||
```
|
||
|
||
**Savings**: 100+ lines eliminated, single source of truth
|
||
|
||
### 3. Request/Response Types (30 min)
|
||
|
||
**Problem**: Repetitive manual parameter parsing and validation
|
||
**Solution**: Create typed request structs with validation methods
|
||
|
||
**Created**: `internal/handlers/types.go`
|
||
|
||
```go
|
||
// PDFExportRequest represents all parameters for PDF export
|
||
type PDFExportRequest struct {
|
||
Lang string // "en" or "es"
|
||
Length string // "short" or "long"
|
||
Icons string // "show" or "hide"
|
||
Version string // "with_skills" or "clean"
|
||
}
|
||
|
||
// ParsePDFExportRequest parses and validates PDF export parameters
|
||
func ParsePDFExportRequest(r *http.Request) (*PDFExportRequest, error) {
|
||
req := &PDFExportRequest{
|
||
Lang: r.URL.Query().Get("lang"),
|
||
Length: r.URL.Query().Get("length"),
|
||
Icons: r.URL.Query().Get("icons"),
|
||
Version: r.URL.Query().Get("version"),
|
||
}
|
||
|
||
// Set defaults
|
||
if req.Lang == "" { req.Lang = "en" }
|
||
// ...
|
||
|
||
// Validate all fields
|
||
if req.Lang != "en" && req.Lang != "es" {
|
||
return nil, fmt.Errorf("unsupported language: %s", req.Lang)
|
||
}
|
||
// ...
|
||
|
||
return req, nil
|
||
}
|
||
```
|
||
|
||
**Usage**:
|
||
```go
|
||
// Before (38 lines of validation)
|
||
func (h *CVHandler) ExportPDF(w http.ResponseWriter, r *http.Request) {
|
||
lang := r.URL.Query().Get("lang")
|
||
if lang == "" { lang = "en" }
|
||
if lang != "en" && lang != "es" {
|
||
HandleError(w, r, BadRequestError("Unsupported language"))
|
||
return
|
||
}
|
||
// ... 30 more lines of validation
|
||
}
|
||
|
||
// After (3 lines)
|
||
func (h *CVHandler) ExportPDF(w http.ResponseWriter, r *http.Request) {
|
||
req, err := ParsePDFExportRequest(r)
|
||
if err != nil {
|
||
HandleError(w, r, BadRequestError(err.Error()))
|
||
return
|
||
}
|
||
// Use req.Lang, req.Length, req.Icons, req.Version
|
||
}
|
||
```
|
||
|
||
**Benefits**:
|
||
- Self-documenting code (struct shows all valid parameters)
|
||
- Centralized validation logic
|
||
- Easy to add new parameters
|
||
- Type-safe access
|
||
|
||
### 4. Middleware Extraction (20 min)
|
||
|
||
**Problem**: Cookie handling duplicated across handlers
|
||
**Solution**: Extract preference middleware
|
||
|
||
**Created**: `internal/middleware/preferences.go`
|
||
|
||
```go
|
||
// Preferences holds user preference values from cookies
|
||
type Preferences struct {
|
||
CVLength string // "short" or "long"
|
||
CVIcons string // "show" or "hide"
|
||
CVLanguage string // "en" or "es"
|
||
CVTheme string // "default" or "clean"
|
||
ColorTheme string // "light" or "dark"
|
||
}
|
||
|
||
// PreferencesMiddleware reads preferences from cookies and stores in context
|
||
func PreferencesMiddleware(next http.Handler) http.Handler {
|
||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||
prefs := &Preferences{
|
||
CVLength: getPreferenceCookie(r, "cv-length", "short"),
|
||
CVIcons: getPreferenceCookie(r, "cv-icons", "show"),
|
||
CVLanguage: getPreferenceCookie(r, "cv-language", "en"),
|
||
CVTheme: getPreferenceCookie(r, "cv-theme", "default"),
|
||
ColorTheme: getPreferenceCookie(r, "color-theme", "light"),
|
||
}
|
||
|
||
// Migrate old values
|
||
if prefs.CVLength == "extended" { prefs.CVLength = "long" }
|
||
// ...
|
||
|
||
// Store in context
|
||
ctx := context.WithValue(r.Context(), PreferencesKey, prefs)
|
||
next.ServeHTTP(w, r.WithContext(ctx))
|
||
})
|
||
}
|
||
|
||
// GetPreferences retrieves preferences from context
|
||
func GetPreferences(r *http.Request) *Preferences {
|
||
prefs, ok := r.Context().Value(PreferencesKey).(*Preferences)
|
||
if !ok {
|
||
return &Preferences{ /* defaults */ }
|
||
}
|
||
return prefs
|
||
}
|
||
```
|
||
|
||
**Benefits**:
|
||
- Read cookies once per request (not multiple times)
|
||
- Centralized migration logic for old preference values
|
||
- Context-based access (no global state)
|
||
- Reusable across handlers
|
||
- Ready to integrate when routes are updated
|
||
|
||
### 5. Handler Tests (45 min)
|
||
|
||
**Problem**: Only PDF and security tests existed
|
||
**Solution**: Comprehensive test coverage for page and HTMX handlers
|
||
|
||
**Created**:
|
||
- `internal/handlers/cv_pages_test.go` - 190 lines, 3 test functions, 15+ test cases
|
||
- `internal/handlers/cv_htmx_test.go` - 325 lines, 5 test functions, 20+ test cases
|
||
|
||
**Test Coverage**:
|
||
|
||
**cv_pages_test.go**:
|
||
```go
|
||
// TestHome - Full page rendering
|
||
- Default language (English)
|
||
- Explicit English
|
||
- Explicit Spanish
|
||
- Invalid language (400 error)
|
||
|
||
// TestCVContent - HTMX content swaps
|
||
- Default/English/Spanish languages
|
||
- Invalid language handling
|
||
|
||
// TestDefaultCVShortcut - PDF shortcuts
|
||
- Valid shortcut URLs (current year, both languages)
|
||
- Invalid year/language/format (404 errors)
|
||
- Skips PDF generation in short mode
|
||
```
|
||
|
||
**cv_htmx_test.go**:
|
||
```go
|
||
// TestToggleLength - CV length toggle
|
||
- Toggle short → long
|
||
- Toggle long → short
|
||
- Migration from "extended" → "long"
|
||
|
||
// TestToggleIcons - Icon visibility toggle
|
||
- Toggle show → hide
|
||
- Toggle hide → show
|
||
- Migration from "true"/"false" → "show"/"hide"
|
||
|
||
// TestSwitchLanguage - Language switching
|
||
- Switch to English/Spanish
|
||
- Invalid language (400 error)
|
||
- Cookie persistence
|
||
|
||
// TestToggleTheme - Theme toggle
|
||
- Toggle default → clean
|
||
- Toggle clean → default
|
||
|
||
// TestHTMXHandlersRequirePost - Method validation
|
||
- ToggleLength rejects GET (405)
|
||
- ToggleIcons rejects GET (405)
|
||
- ToggleTheme rejects GET (405)
|
||
```
|
||
|
||
## Architecture
|
||
|
||
### File Organization
|
||
|
||
```
|
||
internal/
|
||
├── handlers/
|
||
│ ├── cv.go (29 lines) - Struct + constructor
|
||
│ ├── cv_pages.go (120 lines) - Page handlers (refactored)
|
||
│ ├── cv_pdf.go (153 lines) - PDF export (refactored)
|
||
│ ├── cv_htmx.go (218 lines) - HTMX toggles
|
||
│ ├── cv_helpers.go (385 lines) - Helper functions
|
||
│ ├── types.go (106 lines) ✨ NEW - Request/response types
|
||
│ ├── cv_pages_test.go (190 lines) ✨ NEW - Page handler tests
|
||
│ ├── cv_htmx_test.go (325 lines) ✨ NEW - HTMX handler tests
|
||
│ ├── pdf_test.go (694 lines) - PDF integration tests
|
||
│ ├── cv_security_test.go (146 lines) - Security tests
|
||
│ └── errors.go (143 lines) - Error handling
|
||
│
|
||
└── middleware/
|
||
└── preferences.go (94 lines) ✨ NEW - Preference middleware
|
||
```
|
||
|
||
### Pre-Commit Hook (Fixed)
|
||
|
||
```bash
|
||
# .git/hooks/pre-commit
|
||
# Before (BROKEN)
|
||
TEST_OUTPUT=$(go test -short -run '^((?!PDF).)*$' ./... 2>&1)
|
||
# ERROR: invalid regexp - Perl syntax not supported
|
||
|
||
# After (WORKING)
|
||
TEST_OUTPUT=$(go test -short ./... 2>&1)
|
||
# ✅ Integration tests excluded by +build tag
|
||
```
|
||
|
||
## Benefits
|
||
|
||
### 1. Improved Code Quality
|
||
|
||
**Eliminated Duplication**:
|
||
- 100+ lines of duplicate data preparation removed
|
||
- Single source of truth for template data
|
||
|
||
**Type Safety**:
|
||
- Structured request types replace manual parsing
|
||
- Compile-time safety for parameter access
|
||
- Self-documenting API contracts
|
||
|
||
### 2. Better Testing
|
||
|
||
**Test Coverage**:
|
||
- Before: 2 test files (PDF, security)
|
||
- After: 4 test files (PDF, security, pages, HTMX)
|
||
- Added: 35+ test cases for page and HTMX handlers
|
||
|
||
**Quality Assurance**:
|
||
- Language validation tested
|
||
- Toggle behavior verified
|
||
- Cookie handling validated
|
||
- Method restrictions enforced
|
||
|
||
### 3. Cleaner Architecture
|
||
|
||
**Middleware Pattern**:
|
||
- Separates cross-cutting concerns
|
||
- Reusable preference handling
|
||
- Context-based state management
|
||
|
||
**Layered Validation**:
|
||
- Request parsing layer (types.go)
|
||
- Business logic layer (handlers)
|
||
- Clear separation of concerns
|
||
|
||
### 4. Developer Experience
|
||
|
||
**Faster Development**:
|
||
- Type-safe parameters prevent typos
|
||
- Centralized validation reduces bugs
|
||
- Middleware eliminates boilerplate
|
||
|
||
**Easier Debugging**:
|
||
- Clear error messages from typed requests
|
||
- Test coverage catches regressions
|
||
- Isolated concerns simplify troubleshooting
|
||
|
||
### 5. Working Pre-Commit Hook
|
||
|
||
**Quality Gate**:
|
||
- Automatic linting before commit
|
||
- Unit tests run automatically
|
||
- Integration tests excluded (fast feedback)
|
||
- Prevents broken code from being committed
|
||
|
||
## Code Metrics
|
||
|
||
### Line Changes
|
||
|
||
| File | Before | After | Change |
|
||
|------|--------|-------|--------|
|
||
| cv_pages.go | 290 | 120 | -170 lines (58% reduction) |
|
||
| cv_pdf.go | 153 | 153 | Refactored (same LOC, better structure) |
|
||
| types.go | 0 | 106 | +106 lines (new) |
|
||
| preferences.go | 0 | 94 | +94 lines (new) |
|
||
| cv_pages_test.go | 0 | 190 | +190 lines (new) |
|
||
| cv_htmx_test.go | 0 | 325 | +325 lines (new) |
|
||
| **Net Change** | | | **+545 lines** |
|
||
|
||
### Test Coverage
|
||
|
||
| Package | Before | After | Change |
|
||
|---------|--------|-------|--------|
|
||
| handlers | 2 test files | 4 test files | +100% |
|
||
| Test cases | ~15 | ~50 | +233% |
|
||
| Middleware | 0 tests | Ready for tests | Testable architecture |
|
||
|
||
### Quality Improvements
|
||
|
||
- ✅ Pre-commit hook working
|
||
- ✅ 100+ lines of duplication eliminated
|
||
- ✅ Type-safe request handling
|
||
- ✅ Middleware pattern introduced
|
||
- ✅ Comprehensive test coverage
|
||
- ✅ All tests passing
|
||
|
||
## Testing
|
||
|
||
### Test Execution
|
||
|
||
```bash
|
||
# Run all non-integration tests
|
||
$ go test -short ./...
|
||
? github.com/juanatsap/cv-site [no test files]
|
||
ok github.com/juanatsap/cv-site/internal/fileutil 0.192s
|
||
ok github.com/juanatsap/cv-site/internal/handlers 0.607s ✨ NEW TESTS
|
||
ok github.com/juanatsap/cv-site/internal/lang 0.304s
|
||
ok github.com/juanatsap/cv-site/internal/models/cv 0.473s
|
||
ok github.com/juanatsap/cv-site/internal/models/ui 0.843s
|
||
|
||
# Pre-commit hook now works
|
||
$ git commit -m "test"
|
||
🔍 Running golangci-lint pre-commit check...
|
||
✅ Linting passed!
|
||
|
||
🧪 Running tests (excluding integration tests)...
|
||
✅ Tests passed in 2s!
|
||
```
|
||
|
||
### Verification
|
||
|
||
1. **Build**: ✅ `go build` succeeds
|
||
2. **Tests**: ✅ All unit tests pass (35+ new test cases)
|
||
3. **Hook**: ✅ Pre-commit validation works
|
||
4. **Types**: ✅ Type-safe request handling
|
||
5. **Middleware**: ✅ Ready for integration
|
||
|
||
## Interview Talking Points
|
||
|
||
### 1. Systematic Refactoring
|
||
|
||
"I identified five areas for improvement and addressed them systematically in a single cohesive refactoring: pre-commit hook fix, code deduplication, type safety, middleware pattern, and comprehensive testing."
|
||
|
||
### 2. Type Safety
|
||
|
||
"I introduced structured request types with validation, replacing manual parameter parsing. This provides compile-time safety, self-documenting code, and centralized validation logic."
|
||
|
||
### 3. Middleware Pattern
|
||
|
||
"I extracted cookie handling into reusable middleware that reads preferences once and stores them in context, eliminating duplication across handlers and providing a clean separation of concerns."
|
||
|
||
### 4. Test Coverage
|
||
|
||
"I added 35+ test cases for page and HTMX handlers, increasing test file count from 2 to 4. Tests verify language validation, toggle behavior, cookie handling, and method restrictions."
|
||
|
||
### 5. Pragmatic Solutions
|
||
|
||
"I fixed the broken pre-commit hook by removing an incompatible regex filter, leveraging Go's built-in build tags instead. The simpler solution is more maintainable and works correctly."
|
||
|
||
### 6. Code Quality
|
||
|
||
"I eliminated 170 lines of duplication in cv_pages.go (58% reduction) by leveraging an existing helper function, demonstrating DRY principles and attention to code quality."
|
||
|
||
## Future Improvements
|
||
|
||
1. **Integrate Middleware**: Update routes to use `PreferencesMiddleware`
|
||
2. **Middleware Tests**: Add comprehensive tests for preference middleware
|
||
3. **Request Type Coverage**: Add types for language switch and toggle requests
|
||
4. **Response Types**: Define structured response types for consistency
|
||
5. **Validation Tags**: Consider using struct tags for declarative validation
|
||
6. **Context Helpers**: Create convenience functions for context access
|
||
7. **Error Types**: Define typed errors for better error handling
|
||
8. **Benchmark Tests**: Add performance benchmarks for critical paths
|
||
|
||
## Related Documentation
|
||
|
||
- [Refactoring #1: CV/UI Model Separation](./001-cv-model-separation.md)
|
||
- [Refactoring #2: Shared Utilities & Validation](./002-shared-utilities-validation.md)
|
||
- [Refactoring #3: Handler Split](./003-handler-split.md)
|
||
|
||
## Commit Message
|
||
|
||
```
|
||
improve: Add type safety, middleware, and comprehensive handler tests
|
||
|
||
Five complementary improvements to handler layer:
|
||
|
||
1. Fix Pre-Commit Hook
|
||
- Remove broken Perl-style regex (unsupported by Go)
|
||
- Use -short flag to exclude integration tests
|
||
- Tests now run successfully in pre-commit
|
||
|
||
2. Extract Duplicate Logic
|
||
- Remove 100+ lines of duplicate data preparation
|
||
- Both Home() and CVContent() now use prepareTemplateData()
|
||
- Reduce cv_pages.go from 290 to 120 lines (58% reduction)
|
||
|
||
3. Request/Response Types
|
||
- Create internal/handlers/types.go with structured types
|
||
- PDFExportRequest, LanguageRequest, PreferenceToggleRequest
|
||
- Type-safe parameter parsing with centralized validation
|
||
- Refactor ExportPDF to use typed requests
|
||
|
||
4. Middleware Extraction
|
||
- Create internal/middleware/preferences.go
|
||
- PreferencesMiddleware reads cookies once, stores in context
|
||
- Automatic migration of old preference values
|
||
- Ready for integration in routes
|
||
|
||
5. Handler Tests
|
||
- Add internal/handlers/cv_pages_test.go (190 lines, 15+ cases)
|
||
- Add internal/handlers/cv_htmx_test.go (325 lines, 20+ cases)
|
||
- Test language validation, toggles, cookies, methods
|
||
- Increase handler test coverage significantly
|
||
|
||
Testing:
|
||
- All unit tests pass (35+ new test cases)
|
||
- Pre-commit hook working
|
||
- Build succeeds
|
||
- No breaking changes
|
||
|
||
Benefits:
|
||
- Type safety: Compile-time parameter validation
|
||
- Code quality: 170 lines of duplication eliminated
|
||
- Testing: 100% increase in test files
|
||
- Architecture: Clean middleware pattern
|
||
- Developer experience: Self-documenting request types
|
||
```
|