249 lines
6.4 KiB
Markdown
249 lines
6.4 KiB
Markdown
# 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.
|