# Code-Review (Self-Review mit GitLab Discussions)

**WICHTIG:** Code-Review erst durchführen wenn die MR-Pipeline GRÜN ist.
Reihenfolge: Push → MR erstellen → Pipeline abwarten → Pipeline grün → Code-Review → Findings fixen → Merge.
Nicht reviewen solange Pipeline noch läuft oder failed ist – erst Pipeline fixen!

## Ablauf nach MR-Erstellung (ERST wenn MR-Pipeline grün!)

1. **Code lokal reviewen** (nicht nur Diff, sondern gesamten Code)
2. **Findings als GitLab Discussions anlegen**
3. **Findings selbst fixen**
4. **Threads resolven**
5. **Erst mergen wenn alle Threads resolved**

## Review-Kriterien

### Clean Code
- Single Responsibility Principle
- Sprechende Namen (Variablen, Methoden, Klassen)
- Kleine Methoden (max 20 Zeilen)
- Kein toter Code, keine auskommentierten Blöcke
- DRY (Don't Repeat Yourself)

### Separation of Concerns
- Controller nur für HTTP-Handling (kein Business-Logic)
- Service-Layer für Business-Logik
- Repository nur für Datenzugriff
- DTOs für API-Kommunikation (nicht Entity direkt exponieren)

### Best Practices
- Input-Validierung (@Valid, Bean Validation)
- Fehlerbehandlung (Exception Handler, sinnvolle HTTP Status Codes)
- Logging an kritischen Stellen
- Keine Secrets/Credentials im Code
- Immutable wo möglich

### Tests
- Alle öffentlichen Methoden getestet
- Edge Cases abgedeckt (null, leer, ungültig)
- Given-When-Then Struktur
- Mocks nur wo nötig (keine Over-Mocking)

### Security
- Keine SQL Injection (Parameterized Queries / JPA)
- Keine XSS (Output Encoding)
- Keine Secrets im Code
- Dependencies ohne bekannte CVEs

## GitLab Discussion anlegen

```bash
# Discussion an einer bestimmten Datei/Zeile
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
  -f "body=**Clean Code:** Diese Methode hat zu viele Verantwortlichkeiten. Bitte in separate Methoden aufteilen." \
  -f "position[base_sha]=$(git merge-base main HEAD)" \
  -f "position[start_sha]=$(git merge-base main HEAD)" \
  -f "position[head_sha]=$(git rev-parse HEAD)" \
  -f "position[position_type]=text" \
  -f "position[new_path]=src/main/java/com/example/controller/AddressController.java" \
  -f "position[new_line]=42"

# Allgemeine Discussion (nicht an Datei gebunden)
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
  -f "body=**Architektur:** Service-Layer fehlt. Controller greift direkt auf Repository zu. Bitte AddressService einführen."
```

## Findings fixen und Thread resolven

```bash
# Fix implementieren
# ... Code ändern ...
git add -A && git commit -m "fix(review): Service-Layer einführen (#issue)"
git push

# Thread resolven
glab api -X PUT "projects/:id/merge_requests/{mr_iid}/discussions/{discussion_id}" \
  -f "resolved=true"
```

## Merge-Bedingung

```bash
# Prüfen ob alle Threads resolved sind
UNRESOLVED=$(glab api "projects/:id/merge_requests/{mr_iid}/discussions" | python3 -c "
import sys, json
discussions = json.load(sys.stdin)
unresolved = [d for d in discussions if any(n.get('resolvable') and not n.get('resolved') for n in d.get('notes',[]))]
print(len(unresolved))
")

if [ "$UNRESOLVED" = "0" ]; then
  echo "Alle Threads resolved - merge möglich"
  glab mr merge --when-pipeline-succeeds
else
  echo "Noch $UNRESOLVED offene Threads - erst fixen"
fi
```

## Wichtig

- Review den GESAMTEN Code, nicht nur den Diff
- Mindestens 3 Review-Punkte prüfen (auch wenn alles gut aussieht → "LGTM" als Discussion)
- Findings mit Kategorie-Prefix: **Clean Code:**, **Security:**, **Architektur:**, **Test:**
- Jedes Finding muss actionable sein (konkreter Verbesserungsvorschlag)
- Erst mergen wenn 0 unresolved Threads