diff --git a/CLAUDE.md b/CLAUDE.md index 75e83228..3dd52a15 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,9 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Collaboration -See [COLLABORATING.md](./COLLABORATING.md) for the full rules: issue tracking workflow, commit message conventions, the Research → Plan → Implement → Validate cycle, and code style expectations. +See [COLLABORATING.md](./COLLABORATING.md) for the full rules: issue tracking workflow, commit message conventions, and the Research → Plan → Implement → Validate cycle. + +See [CODESTYLE.md](./CODESTYLE.md) for coding standards: Clean Code, DRY/KISS trade-offs (KISS wins), and SOLID principles applied to this stack. --- diff --git a/CODESTYLE.md b/CODESTYLE.md new file mode 100644 index 00000000..2524f10b --- /dev/null +++ b/CODESTYLE.md @@ -0,0 +1,261 @@ +# Code Style Guide + +This document defines the coding standards for the Familienarchiv project. It applies to both the Java backend and the TypeScript/Svelte frontend. When in doubt, prefer code that a competent developer can read and understand without explanation. + +--- + +## Clean Code (Uncle Bob) + +These are principles, not laws. Apply judgment. + +### Names reveal intent + +A name should tell you *why* something exists, what it does, and how it is used — without needing a comment to explain it. + +```java +// Bad +int d; // elapsed time in days +List list2; + +// Good +int elapsedDays; +List receivedDocuments; +``` + +- No abbreviations unless universally understood (`id`, `url`, `dto`). +- Boolean variables and methods should read as yes/no questions: `isEnabled`, `hasFile`, `canWrite`. +- Avoid redundant context: inside class `Document`, write `getTitle()` not `getDocumentTitle()`. + +### Functions do one thing + +A function that does one thing can rarely be meaningfully subdivided. If you can extract a chunk with a name that isn't just a restatement of what it does, it should probably be its own function. + +```java +// Bad — validates, transforms, and persists +public Document saveDocument(DocumentUpdateDTO dto) { + if (dto.getTitle() == null) throw new DomainException(...); + String cleaned = dto.getTitle().strip(); + Document doc = documentRepository.findById(...).orElseThrow(...); + doc.setTitle(cleaned); + return documentRepository.save(doc); +} + +// Good — one responsibility per method; caller orchestrates +public Document updateDocument(UUID id, DocumentUpdateDTO dto) { + validateTitle(dto.getTitle()); + Document doc = getById(id); + applyUpdate(doc, dto); + return documentRepository.save(doc); +} +``` + +### Small functions, minimal parameters + +- Functions longer than ~20 lines are a signal to look for a natural split. +- Aim for ≤ 3 parameters. More than 3 is a sign the function is doing too much, or parameters should be grouped into an object. +- Never use boolean flag arguments — they announce the function does two things: + +```java +// Bad +renderDocument(doc, true); // what does true mean? + +// Good +renderDocumentWithPreview(doc); +renderDocumentWithoutPreview(doc); +``` + +### Guard clauses over deep nesting + +Return or throw early for preconditions. Keep the happy path at the lowest nesting level. + +```java +// Bad +public Document getDocument(UUID id, AppUser user) { + if (id != null) { + Document doc = repository.findById(id).orElse(null); + if (doc != null) { + if (user.canRead(doc)) { + return doc; + } + } + } + return null; +} + +// Good +public Document getDocument(UUID id, AppUser user) { + if (id == null) throw DomainException.notFound(...); + Document doc = repository.findById(id) + .orElseThrow(() -> DomainException.notFound(...)); + if (!user.canRead(doc)) throw DomainException.forbidden(...); + return doc; +} +``` + +### Comments: only for *why*, never for *what* + +Code explains what it does. Comments explain why a non-obvious decision was made. + +```typescript +// Bad — restates the code +// set auth cookie +cookies.set('auth_token', authHeader, { path: '/' }); + +// Good — explains a non-obvious constraint +// secure: false until the deployment is served over HTTPS +cookies.set('auth_token', authHeader, { path: '/', secure: false }); +``` + +If you feel compelled to write a comment that explains *what* the code does, rewrite the code until it doesn't need one. + +### No dead code + +Remove commented-out code, unused variables, unused imports, and unreachable branches. Version control is the history — dead code in the file is noise. + +### Command-query separation + +A function either *does something* (command) or *answers something* (query) — not both. + +```typescript +// Bad — modifies state and returns a value +function addTagAndReturnCount(tag: string): number { ... } + +// Good — separate concerns +function addTag(tag: string): void { ... } +function getTagCount(): number { ... } +``` + +--- + +## DRY vs KISS — KISS wins + +**DRY** (Don't Repeat Yourself): every piece of knowledge has a single, authoritative representation. + +**KISS** (Keep It Simple, Stupid): prefer the simplest solution that works. + +**When they conflict, KISS wins.** Do not create an abstraction to eliminate duplication unless the abstraction has a clear, stable name and genuinely reduces cognitive load. + +### Practical rules + +**Extract when:** +- The same logic appears in 3+ places *and* it has a meaningful name that isn't just a description of the lines it replaces. +- The extracted unit is independently testable. +- The abstraction makes the call site *more* readable, not less. + +**Don't extract when:** +- Two things look similar but might diverge independently — coupling them through an abstraction would make future changes harder. +- The extracted function would be used exactly once. +- Naming the abstraction requires a long or awkward name. + +```typescript +// Three similar lines — do NOT abstract prematurely +const sentYearRange = yearRange(sentDocuments); +const receivedYearRange = yearRange(receivedDocuments); + +// yearRange() is worth extracting because it has a clear name, +// is used in multiple places, and is independently testable. +// But if it were only used once, keep it inline. +``` + +--- + +## SOLID Principles + +Applied to this stack. + +### S — Single Responsibility + +Each class, service, or component has one reason to change. In practice: + +- **Backend:** Controllers receive HTTP, delegate everything to services. Services contain business logic, never touch another domain's repository directly. +- **Frontend:** Components render UI. Server files (`+page.server.ts`) load and validate data. Don't put business logic in Svelte components. +- **Wrong:** A `DocumentService` that also manages user sessions. +- **Right:** `DocumentService` owns documents; `UserService` owns users; each is ignorant of the other's internal details. + +### O — Open/Closed + +Code should be open for extension and closed for modification. Prefer adding new code over editing existing code to support new behavior. + +```java +// Bad — adding a new export format requires editing this method +public byte[] export(String format) { + if (format.equals("csv")) { ... } + else if (format.equals("pdf")) { ... } // added later, modifies existing method +} + +// Good — each format is a separate implementation +public interface DocumentExporter { + byte[] export(List documents); +} +public class CsvExporter implements DocumentExporter { ... } +public class PdfExporter implements DocumentExporter { ... } +``` + +In practice: when adding a variant of existing behavior, reach for a new class/function before editing an existing one. + +### L — Liskov Substitution + +Subtypes must be usable wherever the parent type is expected, without breaking behavior. Concretely: + +- If you extend a service or implement an interface, the subtype must honor the contracts (error cases, return semantics) of the parent. +- Don't override a method to make it a no-op or throw unconditionally — that breaks callers who rely on the contract. + +### I — Interface Segregation + +Don't force callers to depend on methods they don't use. Keep interfaces and services focused. + +```java +// Bad — DocumentService exposed to ImportService even though import only needs findOrCreate +public class MassImportService { + private final DocumentService documentService; // 40+ methods, only 2 needed +} + +// Good — expose only what's needed via a targeted service method or a narrow interface +public class MassImportService { + private final PersonService personService; // only needs findOrCreateByName + private final TagService tagService; // only needs findOrCreate +} +``` + +### D — Dependency Inversion + +High-level modules should not depend on low-level modules. Both should depend on abstractions. + +- **Backend:** Spring's `@Autowired` / constructor injection handles this. Always inject interfaces or Spring beans, never instantiate services with `new` inside a controller or service. +- **Frontend:** Pass data into components via props rather than fetching it inside the component. Components should receive data; server files should supply it. + +```typescript +// Bad — component fetches its own data (depends on network/fetch implementation) +onMount(async () => { + persons = await fetch('/api/persons').then(r => r.json()); +}); + +// Good — data flows in via props from the server load function +let { data } = $props(); // data.persons supplied by +page.server.ts +``` + +--- + +## Formatting and Style Specifics + +These complement the principles above with project-specific conventions. + +### Both Java and TypeScript + +- One concept per line — don't chain side-effects. +- No magic numbers — extract named constants. +- Fail fast: validate inputs at the boundary (controller / server load), trust internal code. + +### Java (backend) + +- Use `DomainException` static factories for all domain errors — never throw raw `RuntimeException`. +- `@Transactional` only on write methods, not reads. +- Entities use `@Builder` — construct with builder pattern, not setters, in tests. +- Avoid `Optional.get()` without `orElseThrow` — always provide a meaningful exception. + +### TypeScript / Svelte (frontend) + +- `$derived` over `$effect` for computed values — effects are for side-effects only. +- Check `!result.response.ok` for API errors, not `result.error` (see CLAUDE.md). +- Prefer typed API client calls over raw `fetch` — use raw `fetch` only for multipart uploads. +- Svelte component logic in `