Refactors calendar architecture for month view
Prepares the calendar component for month view implementation by introducing a strategy pattern for view management, splitting configuration settings, and consolidating events into a core set. It also removes dead code and enforces type safety, improving overall code quality and maintainability. Addresses critical issues identified in the code review, laying the groundwork for efficient feature addition.
This commit is contained in:
Janus Knudsen
parent
7d513600d8
commit
3ddc6352f2
17 changed files with 1347 additions and 428 deletions
282
architecture/code-review-2025-01.md
Normal file
282
architecture/code-review-2025-01.md
Normal file
|
|
@ -0,0 +1,282 @@
|
|||
# Critical Code Review - Calendar Plantempus
|
||||
**Date:** January 2025
|
||||
**Reviewer:** Code Analysis Assistant
|
||||
**Scope:** Full TypeScript/JavaScript codebase
|
||||
|
||||
## Executive Summary
|
||||
This code review identifies 14+ critical issues that impact maintainability, performance, and the ability to add new features (especially month view). The codebase shows signs of rapid development without architectural planning, resulting in significant technical debt.
|
||||
|
||||
---
|
||||
|
||||
## 🚨 CRITICAL ISSUES
|
||||
|
||||
### 1. Inconsistent File Structure
|
||||
**Severity:** High
|
||||
**Impact:** Development confusion, build issues
|
||||
|
||||
**Problems:**
|
||||
- Duplicate TypeScript/JavaScript files exist (`src/utils/PositionUtils.js` and `.ts`)
|
||||
- Mixed compiled and source code in `wwwroot/js/`
|
||||
- Legacy files in root directory (`calendar-*.js`)
|
||||
|
||||
**Evidence:**
|
||||
```
|
||||
src/utils/PositionUtils.js (JavaScript)
|
||||
src/utils/PositionUtils.ts (TypeScript)
|
||||
calendar-grid-manager.js (Root legacy file)
|
||||
```
|
||||
|
||||
**Recommendation:** Delete all `.js` files in `src/`, remove legacy root files, keep only TypeScript sources.
|
||||
|
||||
---
|
||||
|
||||
### 2. Event System Overcomplexity
|
||||
**Severity:** Critical
|
||||
**Impact:** Impossible to maintain, performance degradation
|
||||
|
||||
**Problems:**
|
||||
- Two overlapping event systems (`EventTypes.ts` with 102 events + `CalendarState.ts`)
|
||||
- Unclear separation of concerns
|
||||
- Legacy events marked as "removed" but still present
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
// EventTypes.ts - 102 constants!
|
||||
export const EventTypes = {
|
||||
CONFIG_UPDATE: 'calendar:configupdate',
|
||||
CALENDAR_TYPE_CHANGED: 'calendar:calendartypechanged',
|
||||
// ... 100 more events
|
||||
}
|
||||
|
||||
// CalendarState.ts - Another event system
|
||||
export const StateEvents = {
|
||||
CALENDAR_STATE_CHANGED: 'calendar:state:changed',
|
||||
// ... more events
|
||||
}
|
||||
```
|
||||
|
||||
**Recommendation:** Consolidate to ~20 core events with clear ownership.
|
||||
|
||||
---
|
||||
|
||||
### 3. Missing Error Handling
|
||||
**Severity:** High
|
||||
**Impact:** Silent failures, poor user experience
|
||||
|
||||
**Problems:**
|
||||
- No try-catch blocks in critical paths
|
||||
- No error boundaries for component failures
|
||||
- DateCalculator assumes all inputs are valid
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
// DateCalculator.ts - No validation
|
||||
getISOWeekStart(date: Date): Date {
|
||||
const monday = new Date(date); // What if date is invalid?
|
||||
const currentDay = monday.getDay();
|
||||
// ... continues without checks
|
||||
}
|
||||
```
|
||||
|
||||
**Recommendation:** Add comprehensive error handling and validation.
|
||||
|
||||
---
|
||||
|
||||
### 4. Memory Leak Potential
|
||||
**Severity:** Critical
|
||||
**Impact:** Browser performance degradation over time
|
||||
|
||||
**Problems:**
|
||||
- Event listeners never cleaned up
|
||||
- DOM references held indefinitely
|
||||
- Multiple DateCalculator instances created
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
// ViewManager.ts - No cleanup
|
||||
constructor(eventBus: IEventBus) {
|
||||
this.eventBus = eventBus;
|
||||
this.setupEventListeners(); // Never removed!
|
||||
}
|
||||
// No destroy() method exists
|
||||
```
|
||||
|
||||
**Recommendation:** Implement proper cleanup in all managers.
|
||||
|
||||
---
|
||||
|
||||
### 5. Single Responsibility Violations
|
||||
**Severity:** High
|
||||
**Impact:** Unmaintainable code, difficult to test
|
||||
|
||||
**Problems:**
|
||||
- `GridManager`: Handles rendering, events, styling, positioning (311 lines!)
|
||||
- `CalendarConfig`: Config, state management, and factory logic mixed
|
||||
- `NavigationRenderer`: DOM manipulation and event rendering
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
// GridManager doing everything:
|
||||
- subscribeToEvents()
|
||||
- render()
|
||||
- setupGridInteractions()
|
||||
- getClickPosition()
|
||||
- scrollToHour()
|
||||
- minutesToTime()
|
||||
```
|
||||
|
||||
**Recommendation:** Split into focused, single-purpose classes.
|
||||
|
||||
---
|
||||
|
||||
### 6. Dependency Injection Missing
|
||||
**Severity:** Medium
|
||||
**Impact:** Untestable code, tight coupling
|
||||
|
||||
**Problems:**
|
||||
- Hard-coded singleton imports everywhere
|
||||
- `calendarConfig` imported directly in 15+ files
|
||||
- Circular dependencies through EventBus
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
import { calendarConfig } from '../core/CalendarConfig'; // Singleton
|
||||
import { eventBus } from '../core/EventBus'; // Another singleton
|
||||
```
|
||||
|
||||
**Recommendation:** Use constructor injection pattern.
|
||||
|
||||
---
|
||||
|
||||
### 7. Performance Problems
|
||||
**Severity:** Medium
|
||||
**Impact:** Sluggish UI, especially with many events
|
||||
|
||||
**Problems:**
|
||||
- `document.querySelector` called repeatedly
|
||||
- No caching of DOM elements
|
||||
- Full re-renders on every change
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
// Called multiple times per render:
|
||||
const scrollableContent = document.querySelector('swp-scrollable-content');
|
||||
```
|
||||
|
||||
**Recommendation:** Cache DOM queries, implement selective rendering.
|
||||
|
||||
---
|
||||
|
||||
### 8. Type Safety Issues
|
||||
**Severity:** High
|
||||
**Impact:** Runtime errors, hidden bugs
|
||||
|
||||
**Problems:**
|
||||
- `any` types used extensively
|
||||
- Type casting to bypass checks
|
||||
- Missing interfaces for data structures
|
||||
|
||||
**Evidence:**
|
||||
```typescript
|
||||
private allDayEvents: any[] = []; // No type safety
|
||||
(header as any).dataset.today = 'true'; // Bypassing TypeScript
|
||||
```
|
||||
|
||||
**Recommendation:** Define proper TypeScript interfaces for all data.
|
||||
|
||||
---
|
||||
|
||||
## 🔧 TECHNICAL DEBT
|
||||
|
||||
### 9. Redundant Code
|
||||
- Duplicate date logic in DateCalculator and PositionUtils
|
||||
- Headers rendered in multiple places
|
||||
- Similar event handling patterns copy-pasted
|
||||
|
||||
### 10. Testability Issues
|
||||
- No dependency injection makes mocking impossible
|
||||
- Direct DOM manipulation prevents unit testing
|
||||
- Global state makes tests brittle
|
||||
|
||||
### 11. Documentation Problems
|
||||
- Mixed Danish/English comments
|
||||
- Missing JSDoc for public APIs
|
||||
- Outdated comments that don't match code
|
||||
|
||||
---
|
||||
|
||||
## ⚡ ARCHITECTURE ISSUES
|
||||
|
||||
### 12. Massive Interfaces
|
||||
- `CalendarTypes.ts`: Too many interfaces in one file
|
||||
- `EventTypes`: 102 constants is unmanageable
|
||||
- Manager interfaces too broad
|
||||
|
||||
### 13. Coupling Problems
|
||||
- High coupling between managers
|
||||
- Everything communicates via events (performance hit)
|
||||
- All components depend on global config
|
||||
|
||||
### 14. Naming Inconsistency
|
||||
- Mixed language conventions
|
||||
- Unclear event names (`REFRESH_REQUESTED` vs `CALENDAR_REFRESH_REQUESTED`)
|
||||
- `swp-` prefix unexplained
|
||||
|
||||
---
|
||||
|
||||
## 📊 METRICS
|
||||
|
||||
### Code Quality Scores
|
||||
- **Maintainability:** 3/10
|
||||
- **Testability:** 2/10
|
||||
- **Performance:** 5/10
|
||||
- **Type Safety:** 4/10
|
||||
- **Architecture:** 3/10
|
||||
|
||||
### File Statistics
|
||||
- **Total TypeScript files:** 24
|
||||
- **Total JavaScript files:** 8 (should be 0)
|
||||
- **Average file size:** ~200 lines (acceptable)
|
||||
- **Largest file:** GridManager.ts (311 lines)
|
||||
- **Event types defined:** 102 (should be ~20)
|
||||
|
||||
---
|
||||
|
||||
## 🎯 RECOMMENDATIONS
|
||||
|
||||
### Immediate Actions (Week 1)
|
||||
1. **Remove duplicate files** - Clean up `.js` duplicates
|
||||
2. **Add error boundaries** - Prevent cascade failures
|
||||
3. **Fix memory leaks** - Add cleanup methods
|
||||
|
||||
### Short Term (Month 1)
|
||||
4. **Consolidate events** - Reduce to core 20 events
|
||||
5. **Implement DI** - Remove singleton dependencies
|
||||
6. **Split mega-classes** - Apply Single Responsibility
|
||||
|
||||
### Long Term (Quarter 1)
|
||||
7. **Add comprehensive tests** - Aim for 80% coverage
|
||||
8. **Performance optimization** - Virtual scrolling, caching
|
||||
9. **Complete documentation** - JSDoc all public APIs
|
||||
|
||||
---
|
||||
|
||||
## Impact on Month View Implementation
|
||||
|
||||
**Without refactoring:**
|
||||
- 🔴 ~2000 lines of new code
|
||||
- 🔴 3-4 weeks implementation
|
||||
- 🔴 High bug risk
|
||||
|
||||
**With minimal refactoring:**
|
||||
- ✅ ~500 lines of new code
|
||||
- ✅ 1 week implementation
|
||||
- ✅ Reusable components
|
||||
|
||||
---
|
||||
|
||||
## Conclusion
|
||||
|
||||
The codebase requires significant refactoring to support new features efficiently. The identified issues, particularly the lack of strategy pattern and hardcoded week/day assumptions, make adding month view unnecessarily complex.
|
||||
|
||||
**Priority:** Focus on minimal refactoring that enables month view (Strategy pattern, config split, event consolidation) before attempting to add new features.
|
||||
Loading…
Add table
Add a link
Reference in a new issue