496303471d
---ci--- project: atelier phase: 7 milestone: v0.1 status: complete phase_role: final milestone_complete: true requirements: covered: [ATELIER-01, ATELIER-02, ATELIER-03, ATELIER-04, ATELIER-05, ATELIER-06, ATELIER-07, ATELIER-08, ATELIER-09, ATELIER-10, ATELIER-11, ATELIER-12, ATELIER-13, ATELIER-14, ATELIER-15, ATELIER-16, ATELIER-17, ATELIER-18, ATELIER-19, ATELIER-20, ATELIER-21, ATELIER-22, ATELIER-23, ATELIER-24, ATELIER-25, ATELIER-26, ATELIER-27, ATELIER-28, ATELIER-29, ATELIER-30, ATELIER-31, ATELIER-32, ATELIER-33, ATELIER-34, ATELIER-35] partial: [] ship: milestone: v0.1 type: NFR tag: v0.0.7 merge: milestone/v0.1-atelier -> main release: https://git.cloudinit.dev/cloudinit-bot/atelier/releases/tag/v0.0.7 ---/ci--- Milestone v0.1 — Initial Framework (NFR, complete). 8 core principles (C1-C8), 11 domains, 110 domain principles, 27 derived docs, 4 good + 3 bad examples, 4 language docs, full matrix, 3 review docs. All 35 requirements covered. 7 patches (v0.0.0 pre-execution through v0.0.7 final). v0.0.7 IS the v0.1.0 milestone release.
107 lines
4.8 KiB
Markdown
107 lines
4.8 KiB
Markdown
# Bad Example: God Object
|
|
|
|
> A component that violates Atelier principles. Each violation is cited.
|
|
|
|
## The Code
|
|
|
|
```typescript
|
|
// UserManager.ts — 1,200 lines
|
|
class UserManager {
|
|
private users: User[] = [];
|
|
private cache: Map<string, User> = new Map();
|
|
private db: Database;
|
|
private emailService: EmailService;
|
|
private logger: Logger;
|
|
private auditLog: AuditLog;
|
|
|
|
constructor(db: Database, email: EmailService, logger: Logger, audit: AuditLog) {
|
|
this.db = db;
|
|
this.emailService = email;
|
|
this.logger = logger;
|
|
this.auditLog = audit;
|
|
}
|
|
|
|
// CRUD
|
|
async createUser(data: UserData): Promise<User> { /* 80 lines */ }
|
|
async getUser(id: string): Promise<User> { /* 40 lines */ }
|
|
async updateUser(id: string, data: Partial<UserData>): Promise<User> { /* 60 lines */ }
|
|
async deleteUser(id: string): Promise<void> { /* 50 lines */ }
|
|
async listUsers(page: number): Promise<User[]> { /* 40 lines */ }
|
|
|
|
// Email
|
|
async sendWelcomeEmail(user: User): Promise<void> { /* 50 lines */ }
|
|
async sendPasswordReset(user: User): Promise<void> { /* 50 lines */ }
|
|
async sendDeletionNotice(user: User): Promise<void> { /* 40 lines */ }
|
|
|
|
// Auth
|
|
async authenticate(email: string, password: string): Promise<boolean> { /* 70 lines */ }
|
|
async authorize(userId: string, action: string): Promise<boolean> { /* 60 lines */ }
|
|
async hashPassword(password: string): Promise<string> { /* 20 lines */ }
|
|
|
|
// Cache
|
|
private cacheGet(id: string): User | null { /* 20 lines */ }
|
|
private cacheSet(user: User): void { /* 20 lines */ }
|
|
private cacheInvalidate(id: string): void { /* 20 lines */ }
|
|
|
|
// Audit
|
|
private logAudit(action: string, userId: string): void { /* 30 lines */ }
|
|
|
|
// Validation
|
|
private validateEmail(email: string): boolean { /* 20 lines */ }
|
|
private validatePassword(password: string): boolean { /* 20 lines */ }
|
|
|
|
// Serialization
|
|
toJSON(user: User): Record<string, unknown> { /* 30 lines */ }
|
|
fromJSON(data: Record<string, unknown>): User { /* 30 lines */ }
|
|
|
|
// ... 200 more lines of helper methods
|
|
}
|
|
```
|
|
|
|
## Violations
|
|
|
|
### C3 Simplicity (Core)
|
|
- A 1,200-line class doing 8 different things (CRUD, email, auth, cache, audit, validation, serialization).
|
|
- The class cannot be understood in one read. Complexity is the liability.
|
|
- **Fix:** Split into `UserRepository` (CRUD), `UserEmailService` (email), `UserAuthService` (auth), `UserCache` (cache), `UserAuditLogger` (audit), `UserValidator` (validation), `UserSerializer` (serialization).
|
|
|
|
### C6 Composability (Core)
|
|
- The class takes 4 dependencies and does 8 jobs. It is not composable; it is monolithic.
|
|
- You cannot reuse the email logic without the DB, the cache, the audit log.
|
|
- **Fix:** Each responsibility is its own class. Compose them: `UserEmailService` takes only `EmailService`.
|
|
|
|
### components.md §1 Single Responsibility (UI/UX, applies to code)
|
|
- The class name is `UserManager`. "Manager" is a smell — it manages what? Everything.
|
|
- If the name is "Manager," it has no single responsibility.
|
|
- **Fix:** Name by responsibility: `UserRepository`, `UserAuthService`. Names that cannot be "And"-ed.
|
|
|
|
### C4 Locality (Core)
|
|
- Cache logic is in the same class as email logic. A change to cache touches the email methods' neighbor.
|
|
- Related logic (cache get/set/invalidate) is grouped, but unrelated logic (email) is adjacent.
|
|
- **Fix:** `UserCache` is its own class. Cache changes are local to cache.
|
|
|
|
### C2 Clarity (Core)
|
|
- A reader cannot answer "what does `UserManager` do?" in one sentence.
|
|
- The class has 20+ methods. The reader must scan all of them to find the one they need.
|
|
- **Fix:** Smaller classes with clear names. The name is the documentation.
|
|
|
|
### Security P2 Least Privilege (Security)
|
|
- The class has `db`, `emailService`, `logger`, `auditLog` — all available to all methods.
|
|
- `sendWelcomeEmail` has access to `db.delete`. Least privilege is violated.
|
|
- **Fix:** Each service has only the dependencies it needs. `UserEmailService` has `EmailService`, not `Database`.
|
|
|
|
### Testing P2 Independence (Testing)
|
|
- To test `sendWelcomeEmail`, you must construct `UserManager` with a real/mock DB, email, logger, audit.
|
|
- The test setup is 4 mocks for one method. Independence is violated.
|
|
- **Fix:** Test `UserEmailService` with one mock (`EmailService`).
|
|
|
|
## What This Example Reveals
|
|
|
|
The "God Object" is the cardinal sin of OOP. It violates C3 (Simplicity), C6 (Composability), C4 (Locality), and C2 (Clarity) simultaneously. Every other principle suffers downstream:
|
|
|
|
- Testing is hard (T2 Independence).
|
|
- Security is loose (S2 Least Privilege).
|
|
- Evolution is brittle (a change to email risks cache).
|
|
- Review is exhausting (a 1,200-line diff).
|
|
|
|
The fix is always the same: **decompose by responsibility**. The class name is the test: if it is "Manager," "Handler," or "Helper," it has no single responsibility. |