# Go Code Quality Standards

Reference guide for reviewing Go code quality in PRs.

## Common Issues to Watch For

### Error Handling

**Bad:**
```go
result, _ := doSomething()
```

**Good:**
```go
result, err := doSomething()
if err != nil {
    return fmt.Errorf("failed to do something: %w", err)
}
```

### Context Propagation

**Bad:**
```go
func handleRequest(c *gin.Context) {
    result := callExternalAPI() // No timeout
}
```

**Good:**
```go
func handleRequest(c *gin.Context) {
    ctx := c.Request.Context()
    result, err := callExternalAPIWithContext(ctx)
}
```

### Resource Cleanup

**Bad:**
```go
file, _ := os.Open("data.txt")
// Missing Close()
```

**Good:**
```go
file, err := os.Open("data.txt")
if err != nil {
    return err
}
defer file.Close()
```

### Hardcoded Values

**Bad:**
```go
timeout := 30 * time.Second
apiURL := "https://api.example.com"
```

**Good:**
```go
const defaultTimeout = 30 * time.Second

apiURL := os.Getenv("API_URL")
if apiURL == "" {
    return errors.New("API_URL environment variable required")
}
```

## Swag Documentation Standards

### Complete Handler Documentation

```go
// GetFile godoc
//
// @Summary        Retrieve file by ID
// @Description    Returns detailed information about a specific file including metadata, permissions, and sharing status
// @Tags           files
// @Accept         json
// @Produce        json
// @Param          id   path      string  true  "File ID"
// @Param          X-Tenant-ID  header  string  true  "Tenant identifier"
// @Success        200  {object}  model.File
// @Failure        400  {string}  string  "Invalid file ID"
// @Failure        404  {string}  string  "File not found"
// @Failure        500  {string}  string  "Internal server error"
// @Router         /v1/files/{id} [get]
func GetFile(c *gin.Context) {
    // implementation
}
```

### Required Swag Elements

1. **@Summary** - One line description (< 120 chars)
2. **@Description** - Detailed explanation of behavior
3. **@Tags** - Logical grouping (files, folders, sessions, etc.)
4. **@Accept** - Content-Type accepted (if body expected)
5. **@Produce** - Content-Type returned
6. **@Param** - Document EVERY parameter (path, query, header, body)
7. **@Success** - All successful response codes with types
8. **@Failure** - All error response codes with descriptions
9. **@Router** - Exact route path and HTTP method

### Common Swag Mistakes

- Missing `@Param` for headers (X-Tenant-ID, Authorization)
- Incomplete `@Failure` documentation (only documenting 400/500)
- Generic descriptions ("Get file" vs "Retrieve file with metadata and permissions")
- Wrong model references (string vs object)
- Missing `@Accept` on POST/PUT handlers

## Project-Specific Patterns

### SQL Style

Use lowercase SQL keywords:
```go
query := "select id, name from users where active = ?"
```

Not:
```go
query := "SELECT id, name FROM users WHERE active = ?"
```

### Layer Separation

- **Handlers** (`/internal/server/`): HTTP routing, request/response mapping
- **Service** (`/internal/service/`): Business logic, validation, orchestration
- **API Clients** (`/internal/api/`): External service communication

Don't put business logic in handlers or HTTP code in services.

### Database Operations

Prefer stored procedures for complex queries:
- Type-safe parameters
- Optimized execution
- Centralized logic

### Test Organization

- Unit tests: `/internal/service/*_test.go`
- Integration tests: `/api-tests/*_test.go`
- Helpers: `/api-tests/helpers/*.go`

## Security Checklist

- [ ] No secrets in code (use env vars)
- [ ] All user inputs validated
- [ ] Authentication checks on protected endpoints
- [ ] Authorization checks for resource access
- [ ] SQL queries use parameterization (no string concatenation)
- [ ] Sensitive data not logged
- [ ] CORS configured appropriately
- [ ] Rate limiting considered for public endpoints

## Performance Considerations

- [ ] Database queries are efficient (no N+1 queries)
- [ ] Appropriate indexes exist for queries
- [ ] Large lists are paginated
- [ ] File uploads handle streaming (not loading entire file in memory)
- [ ] Long operations use SSE for progress updates
- [ ] Context timeouts prevent hanging requests
- [ ] Connection pooling configured properly

## Code Organization Red Flags

- God functions (>100 lines)
- Deep nesting (>4 levels)
- Duplicate code (consider extracting helper)
- Magic numbers without constants
- Inconsistent naming conventions
- Missing package documentation
- Circular dependencies between packages

## Testing Red Flags

- Tests that don't assert anything
- Tests that sleep/use arbitrary timeouts
- Tests that depend on external services (without mocking)
- Tests that share state (global variables)
- Tests with no cleanup (leaked resources)
- Missing table-driven tests for multiple cases
- No error case testing

## Documentation Standards

### Anchor Comments for AI Review

Use these ONLY during development, remove before merge:
- `// AIDEV-NOTE:` - Important business logic explanation
- `// AIDEV-TODO:` - Incomplete work or needed improvement
- `// AIDEV-QUESTION:` - Uncertainty or need for discussion

These should be reviewed and removed before PR approval.

### Regular Comments

Only add comments when:
- Explaining non-obvious business rules
- Documenting complex algorithms
- Clarifying why (not what) code does something
- Warning about edge cases or gotchas

Don't comment obvious code:
```go
// Bad: Obvious
// increment counter
counter++

// Good: Explains why
// Skip deleted items to maintain accurate active count
if !item.IsDeleted {
    counter++
}
```

## Go-Specific Best Practices

- Use `gofmt` formatting (enforced by `make tidy`)
- Follow [Effective Go](https://golang.org/doc/effective_go.html) guidelines
- Use short variable names in limited scopes (i, err, ok)
- Use descriptive names for package-level and exported items
- Prefer composition over inheritance
- Use interfaces for abstraction, not just for mocking
- Keep interfaces small and focused
- Return errors, don't panic (except in init() for fatal issues)
- Use `context.Context` for cancellation and deadlines
- Avoid global mutable state

## Makefile Integration

Always verify the Makefile has these targets and run them:
- `make test` or `make test-unit` - Unit tests must pass
- `make test-api` - Integration tests must pass
- `make lint` - Linting must pass with no errors
- `make tidy` - Code must be properly formatted
- `make docs` - If Swag is used, docs must generate successfully

Failing any of these is grounds for requesting changes.