familienarchiv/CODESTYLE.md

# 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<Document> list2;

// Good
int elapsedDays;
List<Document> 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<Document> 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 `<script>`, layout/styles in template — no business logic in markup.

---

## Svelte 5 — Specific Rules

These rules are enforced by ESLint (`eslint-plugin-svelte`). Knowing *why* they exist prevents the need to fix violations after the fact.

### Always key `{#each}` blocks

Without a key, Svelte tracks list items by array position. When items are added, removed, or reordered, Svelte patches DOM nodes in-place from the top — it never moves the correct node. Component-local state (counters, animation state, focus) becomes permanently attached to the wrong item. This is a silent data integrity bug, not a crash.

```svelte
<!-- Bad — position-based tracking; reordering silently corrupts local state -->
{#each documents as doc}
    <DocumentCard {doc} />
{/each}

<!-- Good — identity-based; each node follows its data through reorders -->
{#each documents as doc (doc.id)}
    <DocumentCard {doc} />
{/each}
```

Use `(item.id)` when items have a stable ID. Use the loop index `(i)` only for static lists that will never be reordered. Use `(item)` for primitive lists.

### Use `$derived` for computed values, never `$state` + `$effect`

`$effect` is for *side effects* (DOM calls, network, logging). Using it to assign a computed value introduces a timing problem: `$derived` updates synchronously before the render, while `$effect` runs *after* the render — meaning the component briefly displays a stale value. It also triggers a second reactive pass, doubling the work.

```svelte
<!-- Bad — stale value during render; extra reactive cycle; unclear intent -->
<script>
    let fullName = $state('');
    $effect(() => {
        fullName = `${person.firstName} ${person.lastName}`;
    });
</script>

<!-- Good — synchronous, single-pass, intent is obvious -->
<script>
    const fullName = $derived(`${person.firstName} ${person.lastName}`);
</script>
```

Use `$derived.by(() => { ... })` when the computation needs multiple statements.

### Use Svelte reactive collections, not plain JS ones

Svelte 5's reactivity tracks object *references*, not mutations. When you call `.set()` on a plain `Map` or `.set()` on a plain `URLSearchParams`, the reference doesn't change — Svelte never notices, and the UI goes silently stale.

`SvelteMap`, `SvelteSet`, and `SvelteURLSearchParams` from `svelte/reactivity` wrap the native classes and hook into Svelte's dependency tracker. Every mutation notifies the reactive graph; every read registers a dependency.

```svelte
<!-- Bad — mutations are invisible to Svelte; derived values never update -->
<script>
    const freq = new Map<string, number>();
    freq.set('key', 1); // Svelte does not see this
</script>

<!-- Good — mutations are tracked; all dependents re-run correctly -->
<script>
    import { SvelteMap } from 'svelte/reactivity';
    const freq = new SvelteMap<string, number>();
    freq.set('key', 1); // Svelte tracks this
</script>
```

The same applies to `URLSearchParams` in reactive contexts — use `SvelteURLSearchParams`.