diff --git a/kubernetes/patterns.md b/kubernetes/patterns.md index 7af1269..3943507 100644 --- a/kubernetes/patterns.md +++ b/kubernetes/patterns.md @@ -71,6 +71,41 @@ func (dc *DeploymentController) handleErr(ctx context.Context, err error, key st } ``` +### When to Use + +**Triggers:** +- You're building a system that must maintain desired state over time (not just react to events once) +- External state can change outside your control (user edits, crashes, network partitions) +- You need automatic recovery from partial failures without human intervention + +**Example — before:** +```go +// Event-driven: reacts once and hopes nothing changes +func handlePodCreated(pod Pod) { + assignToNode(pod) + // What if the node dies 5 seconds later? Nobody re-assigns. +} +``` + +**Example — after:** +```go +// Controller pattern: continuously reconciles desired vs actual +func (c *Scheduler) Reconcile(ctx context.Context, key string) error { + pod, err := c.podLister.Get(key) + if err != nil { return err } + + if pod.Spec.NodeName == "" { + node := c.selectBestNode(pod) + return c.assignPodToNode(ctx, pod, node) + } + // Already assigned — verify node is still healthy + if !c.nodeIsReady(pod.Spec.NodeName) { + return c.reassignPod(ctx, pod) + } + return nil // desired state matches actual state +} +``` + ### Key Properties 1. **Level-triggered, not edge-triggered** — the sync loop reads current state, not diffs 2. **Idempotent** — running sync twice produces the same result @@ -162,6 +197,51 @@ A concurrent-safe work queue with three critical properties: ### Why In a controller, multiple events may fire for the same object in rapid succession. Without deduplication, you'd process stale intermediate states. The dirty/processing set design ensures you always process the latest state while never losing notifications. +### When to Use + +**Triggers:** +- Multiple event sources (informer callbacks) trigger work on the same object rapidly +- You need to deduplicate: 5 events for the same pod should result in 1 sync, not 5 +- Failed processing should retry with exponential backoff, not flood the system + +**Example — before:** +```go +// Raw channel: no deduplication, no backoff +events := make(chan string, 100) + +// Producer fires rapid updates: +events <- "pod-abc" // event 1 +events <- "pod-abc" // event 2 (duplicate!) +events <- "pod-abc" // event 3 (duplicate!) + +// Consumer processes all 3 — wasteful +for key := range events { + reconcile(key) // called 3 times for the same stale state +} +``` + +**Example — after:** +```go +queue := workqueue.NewTypedRateLimitingQueue[string]( + workqueue.DefaultTypedControllerRateLimiter[string](), +) + +// Producer fires rapid updates — queue deduplicates: +queue.Add("pod-abc") // queued +queue.Add("pod-abc") // already dirty — no-op +queue.Add("pod-abc") // already dirty — no-op + +// Consumer processes once with latest state: +key, _ := queue.Get() +err := reconcile(key) // called once +if err != nil { + queue.AddRateLimited(key) // retry with backoff +} else { + queue.Forget(key) // clear backoff counter +} +queue.Done(key) +``` + ### The Dirty/Processing Dance ```go @@ -321,6 +401,43 @@ func (m *BaseControllerRefManager) ClaimObject(ctx context.Context, obj metav1.O ### What it does Provides distributed mutex semantics using a Kubernetes resource (Lease) as the lock. Only one instance of a controller runs actively; others are hot standbys. +### When to Use + +**Triggers:** +- You're running multiple replicas of a controller for high availability +- Only ONE instance should actively reconcile at a time (to avoid conflicts) +- You need automatic failover: if the leader dies, another replica takes over within seconds + +**Example — before:** +```go +// All replicas reconcile simultaneously → write conflicts, duplicate work +func main() { + ctrl := NewController() + ctrl.Run(ctx) // every replica does this — chaos +} +``` + +**Example — after:** +```go +func main() { + ctrl := NewController() + leaderelection.RunOrDie(ctx, leaderelection.LeaderElectionConfig{ + Lock: resourceLock, + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, + Callbacks: leaderelection.LeaderCallbacks{ + OnStartedLeading: func(ctx context.Context) { + ctrl.Run(ctx) // only the leader reconciles + }, + OnStoppedLeading: func() { + log.Fatal("lost leadership") // restart to re-enter election + }, + }, + }) +} +``` + ### Why Controller-manager runs multiple replicas for HA. Only one should reconcile to avoid conflicts. diff --git a/kubernetes/production-go.md b/kubernetes/production-go.md index ef35ed1..a418339 100644 --- a/kubernetes/production-go.md +++ b/kubernetes/production-go.md @@ -52,6 +52,37 @@ Generated informers (note the header comment): // Code generated by informer-gen. DO NOT EDIT. ``` +### When to Use + +**Triggers:** +- You have 10+ types that need identical boilerplate methods (DeepCopy, Validate, Marshal) +- Hand-writing the code is error-prone (forgetting to copy a new field causes silent bugs) +- The generated output is mechanical and reviewable, not creative + +**Example — before:** +```go +// Hand-written deep copy for every type — 50 types × 30 lines each = 1500 lines of bugs +func (in *Deployment) DeepCopy() *Deployment { + out := new(Deployment) + out.Name = in.Name + out.Labels = make(map[string]string) + for k, v := range in.Labels { out.Labels[k] = v } + // Did you remember Annotations? Finalizers? Every nested struct? +} +``` + +**Example — after:** +```go +// +k8s:deepcopy-gen=true +type Deployment struct { + Name string + Labels map[string]string + Annotations map[string]string +} +// Generated: zz_generated.deepcopy.go handles ALL fields correctly, always. +// Adding a new field? Re-run generator. Zero chance of forgetting. +``` + ### Key Insight **Stdlib has no code generation culture.** stdlib keeps things small enough that hand-writing works. Kubernetes proves that once you cross ~20 types with shared behavior, code gen is the only sane path. @@ -329,6 +360,47 @@ In a production system with hundreds of goroutines, an unrecovered panic in one - Allows cleanup handlers (shutdown gracefully) - In tests, can be configured to not actually crash +### When to Use + +**Triggers:** +- You're running multiple independent subsystems in one process (multiple controllers, background workers) +- A panic in one subsystem shouldn't kill the entire process +- You need structured logging of panic stack traces before potential recovery + +**Example — before:** +```go +// One bad nil pointer in workerB kills workerA, workerC, and the whole server +func main() { + go workerA(ctx) + go workerB(ctx) // panics → entire process dies + go workerC(ctx) + select {} +} +``` + +**Example — after:** +```go +func safeGo(ctx context.Context, name string, f func(ctx context.Context)) { + go func() { + defer func() { + if r := recover(); r != nil { + log.Printf("panic in %s: %v +%s", name, r, debug.Stack()) + // Log, alert, increment metric — but don't kill siblings + } + }() + f(ctx) + }() +} + +func main() { + safeGo(ctx, "worker-a", workerA) + safeGo(ctx, "worker-b", workerB) // panics → logged, other workers continue + safeGo(ctx, "worker-c", workerC) + select {} +} +``` + ### Key Insight Stdlib's approach is "let it crash." Kubernetes' approach is "catch it, log it, let the controller retry on the next sync." This is only safe because the controller pattern is idempotent. diff --git a/patterns/api-conventions.md b/patterns/api-conventions.md index c131fed..bf75d21 100644 --- a/patterns/api-conventions.md +++ b/patterns/api-conventions.md @@ -13,6 +13,32 @@ is non-nil. Named `MustXxx` or `Must` (when wrapping a generic `(T, error)` pair `var` initializers can't handle errors, `Must` converts programmer errors (bad regex literals, bad templates) into immediate panics that surface during init. +**When to Use** + +**Triggers:** +- You're initializing a package-level variable with a value that is known at compile time (regex, template, URL) +- Failure means a programmer bug, not a runtime condition (the regex literal is wrong, not user input) +- `var` initialization can't handle the `(T, error)` return + +**Example — before:** +```go +var emailRegex *regexp.Regexp + +func init() { + var err error + emailRegex, err = regexp.Compile(`^[a-z]+@[a-z]+\.[a-z]+$`) + if err != nil { + panic(err) // manual panic, verbose + } +} +``` + +**Example — after:** +```go +var emailRegex = regexp.MustCompile(`^[a-z]+@[a-z]+\.[a-z]+$`) +// One line. Panics on typo (caught immediately in tests). Clean. +``` + **Anti-pattern:** Using Must in runtime code where the input is dynamic/user-provided; panicking on recoverable errors; naming it something other than Must (e.g., `PanicOnError`). @@ -211,6 +237,29 @@ func (b *Builder) String() string { **Why:** 90% of file opens are reads or creates. Layered APIs serve the common case without hiding power. The naming makes intent clear. +**When to Use** + +**Triggers:** +- 90% of callers need the simple case (open for read, create and truncate) +- You have a powerful function with many flags/options but most combinations are rare +- You find yourself writing the same flag combination repeatedly in calling code + +**Example — before:** +```go +// User must know about flags for every file open +f, err := os.OpenFile("data.json", os.O_RDONLY, 0) +// Every. Single. Time. +``` + +**Example — after:** +```go +// Simple case: +f, err := os.Open("data.json") // just reads — no flags to remember + +// Power case (when you actually need it): +f, err := os.OpenFile("data.json", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) +``` + **Anti-pattern:** Only exposing the full-power version; making users learn flag constants for simple reads; duplicating implementation across convenience functions. @@ -311,6 +360,48 @@ timeout mechanism. Graceful shutdown is critical for production services; immediate close is needed for tests and emergency stops. +**When to Use** + +**Triggers:** +- Your type manages long-lived connections or in-flight requests +- You need both "stop now" (tests, emergencies) and "drain gracefully" (deploys, SIGTERM) +- A `Close()` that waits forever would make tests hang + +**Example — before:** +```go +type Server struct { listener net.Listener } + +func (s *Server) Stop() { + s.listener.Close() // all in-flight requests get connection reset — data loss +} +``` + +**Example — after:** +```go +type Server struct { + listener net.Listener + active sync.WaitGroup +} + +// Immediate: drop everything +func (s *Server) Close() error { + return s.listener.Close() +} + +// Graceful: stop accepting, wait for in-flight with timeout +func (s *Server) Shutdown(ctx context.Context) error { + s.listener.Close() // stop accepting new connections + done := make(chan struct{}) + go func() { s.active.Wait(); close(done) }() + select { + case <-done: + return nil // all requests finished + case <-ctx.Done(): + return ctx.Err() // timed out — caller decides what to do + } +} +``` + **Anti-pattern:** Only providing one shutdown mode; not accepting a context for timeout control; leaking goroutines on shutdown. diff --git a/patterns/concurrency.md b/patterns/concurrency.md index c57733f..24ed05e 100644 --- a/patterns/concurrency.md +++ b/patterns/concurrency.md @@ -38,6 +38,39 @@ func (m *Mutex) Lock() { - **Not associated with a goroutine** — one goroutine can Lock, another can Unlock - **Locker interface** — abstracts over Mutex and RWMutex +### When to Use + +**Triggers:** +- Multiple goroutines read AND write the same data structure +- You need to protect a small critical section (a few field accesses) +- A channel-based solution would add complexity without benefit (no coordination needed, just protection) + +**Example — before:** +```go +type Stats struct { + hits int + misses int +} + +func (s *Stats) RecordHit() { s.hits++ } // DATA RACE when called from multiple goroutines +func (s *Stats) RecordMiss() { s.misses++ } // DATA RACE +``` + +**Example — after:** +```go +type Stats struct { + mu sync.Mutex + hits int + misses int +} + +func (s *Stats) RecordHit() { + s.mu.Lock() + defer s.mu.Unlock() + s.hits++ +} +``` + ### Idiomatic Usage ```go @@ -101,6 +134,40 @@ The implementation reveals a subtle guarantee: **when Do returns, f has finished The `done` field is first in the struct for hot-path performance on amd64/386 (noted in comment at line 24-27). +### When to Use + +**Triggers:** +- You have expensive initialization that should happen exactly once (DB connection, config parse, compiled regex) +- Multiple goroutines may trigger the initialization concurrently +- You're using `var` + `if instance == nil` checks that aren't goroutine-safe + +**Example — before:** +```go +var db *sql.DB + +func GetDB() *sql.DB { + if db == nil { // RACE: two goroutines can both see nil + db, _ = sql.Open("postgres", connStr) + } + return db +} +``` + +**Example — after:** +```go +var ( + db *sql.DB + once sync.Once +) + +func GetDB() *sql.DB { + once.Do(func() { + db, _ = sql.Open("postgres", connStr) + }) + return db +} +``` + ### Idiomatic Usage ```go @@ -297,6 +364,51 @@ case result := <-work: } ``` +### When to Use + +**Triggers:** +- You need to broadcast "stop" to multiple goroutines simultaneously +- A goroutine needs to select between work and cancellation +- You're implementing graceful shutdown for a long-running service + +**Example — before:** +```go +type Server struct { + stopped bool // RACE: no synchronization +} + +func (s *Server) worker() { + for { + if s.stopped { return } // busy-polls, racy + doWork() + } +} +``` + +**Example — after:** +```go +type Server struct { + done chan struct{} +} + +func NewServer() *Server { + return &Server{done: make(chan struct{})} +} + +func (s *Server) worker() { + for { + select { + case <-s.done: + return + case work := <-s.workCh: + process(work) + } + } +} + +func (s *Server) Stop() { close(s.done) } // broadcasts to ALL workers +``` + ### Anti-pattern ```go @@ -494,6 +606,48 @@ func generate(ctx context.Context) <-chan int { } ``` +### When to Use + +**Triggers:** +- You have a producer-consumer flow where the consumer's speed should limit the producer (backpressure) +- Data flows through multiple transformation stages +- You want to decouple stages that can run concurrently + +**Example — before:** +```go +func processAll(items []string) []Result { + var results []Result + for _, item := range items { + fetched := fetch(item) // sequential: fetch then transform + results = append(results, transform(fetched)) + } + return results +} +``` + +**Example — after:** +```go +func processAll(ctx context.Context, items []string) []Result { + fetched := make(chan Fetched) + go func() { + defer close(fetched) + for _, item := range items { + select { + case fetched <- fetch(item): // backpressure: blocks if transform is slow + case <-ctx.Done(): + return + } + } + }() + + var results []Result + for f := range fetched { + results = append(results, transform(f)) + } + return results +} +``` + ### Anti-pattern ```go diff --git a/patterns/documentation.md b/patterns/documentation.md index 5cda94a..d728a58 100644 --- a/patterns/documentation.md +++ b/patterns/documentation.md @@ -168,6 +168,35 @@ an `// Output:` comment that `go test` verifies. in `go doc` and pkg.go.dev alongside the relevant symbol. They teach by showing real, working code. +**When to Use** + +**Triggers:** +- You have a public function/type whose usage isn't obvious from the signature alone +- Your README examples have drifted from the actual API (broken examples in docs) +- You want examples that appear on pkg.go.dev AND are verified by `go test` + +**Example — before:** +```go +// README.md (may be stale): +// ```go +// result := mylib.Process("input") +// fmt.Println(result.Data) +// ``` +// ← compiles? who knows. API changed last month. +``` + +**Example — after:** +```go +// example_test.go +func ExampleProcess() { + result := mylib.Process("input") + fmt.Println(result.Data) + // Output: + // processed: input +} +// ← go test verifies this compiles and produces the expected output +``` + **Anti-pattern:** Examples that don't compile; examples without Output comments (not verified); examples in README that drift from reality. @@ -266,6 +295,30 @@ deprecated and explains what to use instead. **Why:** Recognized by tooling (go vet, staticcheck, IDEs). Provides a migration path without breaking backward compatibility. +**When to Use** + +**Triggers:** +- You have a better replacement for an existing function but can't remove the old one (semver) +- Users are still calling a function that has known issues or a superior alternative +- You want IDEs to show a strikethrough and linters to warn on usage + +**Example — before:** +```go +// Just delete it? Breaks everyone's code. +// Leave it silently? Users never learn about the better way. +``` + +**Example — after:** +```go +// ParseDuration parses a duration string. +// +// Deprecated: Use [time.ParseDuration] instead, which handles +// all standard duration formats. +func ParseDuration(s string) (time.Duration, error) { + return time.ParseDuration(s) // delegate to the replacement +} +``` + **Anti-pattern:** Removing deprecated APIs (breaks semver); deprecating without suggesting an alternative; using non-standard deprecation markers. diff --git a/patterns/error-handling.md b/patterns/error-handling.md index 9fe9719..615cb58 100644 --- a/patterns/error-handling.md +++ b/patterns/error-handling.md @@ -36,6 +36,44 @@ if err == io.EOF { **Critical rule from io.EOF's doc comment**: Read must return EOF itself, **not an error wrapping EOF**, because callers test for it with `==`. This is the distinction between sentinel errors (identity-checked) and wrapped errors (tree-checked). +### When to Use + +**Triggers:** +- You have a specific, well-known failure condition callers need to check by identity +- Multiple packages compare against the same error value (`io.EOF`, `sql.ErrNoRows`) +- The error represents a **state** ("end of stream", "not found"), not a bug + +**Example — before:** +```go +func fetchUser(id int) (*User, error) { + row := db.QueryRow("SELECT ...") + var u User + err := row.Scan(&u.Name) + if err != nil { + return nil, fmt.Errorf("user not found") // caller can't distinguish "not found" from "db down" + } + return &u, nil +} +``` + +**Example — after:** +```go +var ErrUserNotFound = errors.New("users: not found") + +func fetchUser(id int) (*User, error) { + row := db.QueryRow("SELECT ...") + var u User + err := row.Scan(&u.Name) + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrUserNotFound // sentinel: callers can test with errors.Is + } + if err != nil { + return nil, fmt.Errorf("fetchUser: %w", err) + } + return &u, nil +} +``` + ### Anti-pattern ```go @@ -137,6 +175,43 @@ return fmt.Errorf("open config: %w", err) return fmt.Errorf("open config: %v", err) ``` +### When to Use + +**Triggers:** +- You're adding context to an error before returning it up the call stack +- The caller's error message would be meaningless without knowing *what* operation failed +- You have a chain of function calls and want a readable error trail: `"open config: read file: permission denied"` + +**Example — before:** +```go +func loadConfig(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err // caller sees "open /etc/app.conf: permission denied" — no context about WHO called ReadFile + } + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, err // caller can't tell if this was a read error or a parse error + } + return &cfg, nil +} +``` + +**Example — after:** +```go +func loadConfig(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("load config: %w", err) // wraps: callers can errors.Is(err, os.ErrNotExist) + } + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("load config: parse %s: %v", path, err) // %v: hides internal JSON error type + } + return &cfg, nil +} +``` + ### When to use %w vs %v - **%w**: When the wrapped error is part of your API contract. Callers can depend on it. @@ -313,6 +388,37 @@ errs = append(errs, closeCache()) return errors.Join(errs...) // nil if all nil ``` +### When to Use + +**Triggers:** +- You're closing/cleaning up multiple resources and each can fail independently +- A validation function checks multiple fields and you want ALL errors, not just the first +- You're running parallel operations and collecting errors from each + +**Example — before:** +```go +func cleanup(db *sql.DB, cache *redis.Client, file *os.File) error { + if err := db.Close(); err != nil { + return err // stops here — cache and file leak! + } + if err := cache.Close(); err != nil { + return err // file still leaks + } + return file.Close() +} +``` + +**Example — after:** +```go +func cleanup(db *sql.DB, cache *redis.Client, file *os.File) error { + return errors.Join( + db.Close(), + cache.Close(), + file.Close(), + ) // nil if all nil; contains all failures otherwise +} +``` + ### Anti-pattern ```go diff --git a/patterns/interfaces.md b/patterns/interfaces.md index 328c203..498c52e 100644 --- a/patterns/interfaces.md +++ b/patterns/interfaces.md @@ -31,6 +31,35 @@ type Closer interface { Small interfaces are easy to implement and easy to compose. Any type can satisfy `io.Reader` by implementing a single method. This maximizes the number of types that can participate in the ecosystem — files, network connections, buffers, compressors, encryptors all satisfy `Reader`. +### When to Use + +**Triggers:** +- You're defining a function that only needs ONE capability from its argument (reading, writing, closing) +- You want maximum reusability — many different types should be able to satisfy your requirement +- You're tempted to create a big interface but realize most consumers only use 1-2 methods + +**Example — before:** +```go +// Accepts only *os.File — can't use with buffers, HTTP bodies, test mocks +func countLines(f *os.File) (int, error) { + scanner := bufio.NewScanner(f) + count := 0 + for scanner.Scan() { count++ } + return count, scanner.Err() +} +``` + +**Example — after:** +```go +// Accepts io.Reader — works with files, HTTP bodies, strings.NewReader, gzip.Reader, etc. +func countLines(r io.Reader) (int, error) { + scanner := bufio.NewScanner(r) + count := 0 + for scanner.Scan() { count++ } + return count, scanner.Err() +} +``` + ### Anti-pattern ```go @@ -127,6 +156,37 @@ func TeeReader(r Reader, w Writer) Reader { The return type of `LimitReader` is `Reader` (interface), but the underlying value is `*LimitedReader` (struct). Functions like `io.Copy` can type-assert to `*LimitedReader` to optimize buffer sizes (line 425). +### When to Use + +**Triggers:** +- You're writing a function/constructor that operates on a capability (reading, hashing, connecting) +- Your return type has useful fields or methods beyond the interface contract +- You want callers to pass anything that satisfies the contract, but return something concrete they can inspect + +**Example — before:** +```go +// Too restrictive input, too vague output +func NewLogger(f *os.File) io.Writer { + return &logger{out: f, level: "info"} // hides SetLevel, Flush methods +} +``` + +**Example — after:** +```go +type Logger struct { + out io.Writer + level string +} + +func (l *Logger) SetLevel(lvl string) { l.level = lvl } +func (l *Logger) Flush() error { /* ... */ } + +// Accept interface (any io.Writer), return struct (full access) +func NewLogger(w io.Writer) *Logger { + return &Logger{out: w, level: "info"} +} +``` + ### Anti-pattern ```go @@ -218,6 +278,45 @@ func (f HandlerFunc) ServeHTTP(w ResponseWriter, r *Request) { This bridges functions and interfaces. Any function with the right signature becomes a `Handler` via `HandlerFunc(myFunc)`. You get the simplicity of functions with the composability of interfaces. +### When to Use + +**Triggers:** +- You have an interface with a single method and users frequently implement it with a bare function +- You want to accept both struct-based and function-based implementations of the same behavior +- Requiring a struct definition for simple cases feels like boilerplate + +**Example — before:** +```go +type Processor interface { + Process(data []byte) error +} + +// User must create a whole struct just to use a function +type upperProcessor struct{} +func (u upperProcessor) Process(data []byte) error { + fmt.Println(strings.ToUpper(string(data))) + return nil +} +``` + +**Example — after:** +```go +type Processor interface { + Process(data []byte) error +} + +// Adapter: any function with the right signature becomes a Processor +type ProcessorFunc func([]byte) error + +func (f ProcessorFunc) Process(data []byte) error { return f(data) } + +// Now users can write: +pipeline.Use(ProcessorFunc(func(data []byte) error { + fmt.Println(strings.ToUpper(string(data))) + return nil +})) +``` + ### Anti-pattern ```go @@ -259,6 +358,42 @@ if flusher, ok := w.(Flusher); ok { Not every `ResponseWriter` supports flushing or hijacking (HTTP/2 doesn't support Hijacker). Instead of bloating the main interface, optional capabilities are separate interfaces checked at runtime. This keeps the core interface small while allowing progressive enhancement. +### When to Use + +**Triggers:** +- Some implementations support a capability but others don't (flushing, hijacking, seeking) +- You want to keep the core interface small but allow optimizations when available +- You're writing middleware that should enhance behavior when possible, not require it + +**Example — before:** +```go +// Forces ALL stores to implement caching, even simple ones +type Store interface { + Get(key string) ([]byte, error) + Set(key string, val []byte) error + InvalidateCache() error // not all stores have a cache! +} +``` + +**Example — after:** +```go +type Store interface { + Get(key string) ([]byte, error) + Set(key string, val []byte) error +} + +// Optional capability — check at runtime +type Cacheable interface { + InvalidateCache() error +} + +func refreshAll(s Store) { + if c, ok := s.(Cacheable); ok { + c.InvalidateCache() // only if supported + } +} +``` + ### Anti-pattern ```go diff --git a/patterns/package-design.md b/patterns/package-design.md index c359964..ee0661e 100644 --- a/patterns/package-design.md +++ b/patterns/package-design.md @@ -117,6 +117,32 @@ Packages under `internal/` can only be imported by code rooted at the parent of - `net/http/internal/ascii` → importable by `net/http` and children - NOT importable by `net/url` or any other package +### When to Use + +**Triggers:** +- You have helper code shared between sub-packages but NOT part of your public API +- You're tempted to export a function "just for testing" — put it in `internal/` instead +- Your package has grown and you want to split it without committing to new public APIs + +**Example — before:** +```go +// pkg/mylib/helpers.go — exported just so pkg/mylib/sub can use it +package mylib + +func ParseInternalFormat(s string) Thing { ... } // now anyone can depend on this! +``` + +**Example — after:** +```go +// pkg/mylib/internal/parse/parse.go +package parse + +func InternalFormat(s string) Thing { ... } // only importable by pkg/mylib and children + +// pkg/mylib/sub/handler.go +import "pkg/mylib/internal/parse" // ✓ allowed +``` + ### Anti-pattern ```go @@ -192,6 +218,34 @@ The stdlib uses `init()` for: 3. Keep them short 4. Prefer explicit initialization in `main()` when possible +### When to Use + +**Triggers:** +- You're writing a driver or plugin that needs to register itself with a central registry on import +- The registration is side-effect-only (no return value, can't fail) +- You want `import _ "mydb/driver"` to make the driver available without explicit setup + +**Example — before:** +```go +// main.go — user must manually register every driver +func main() { + postgres.Register() // easy to forget + mysql.Register() // order matters? + sqlite.Register() +} +``` + +**Example — after:** +```go +// postgres/driver.go +func init() { + sql.Register("postgres", &Driver{}) // auto-registers on import +} + +// main.go — import for side-effect +import _ "github.com/lib/pq" // driver registers itself +``` + ### Anti-pattern ```go @@ -404,6 +458,34 @@ type contextKey struct { - **Type-safe accessors** avoid repeated type assertions - **Pointer-based keys** guarantee uniqueness +### When to Use + +**Triggers:** +- You need to pass request-scoped metadata through a call chain (user ID, trace ID, auth token) +- The data crosses package boundaries and isn't appropriate as a function parameter +- You want type safety — only your package should read/write its context values + +**Example — before:** +```go +// String keys — any package can collide or access your values +ctx = context.WithValue(ctx, "userID", 42) +uid := ctx.Value("userID").(int) // panics if wrong type or missing +``` + +**Example — after:** +```go +type ctxKey struct{} + +func WithUserID(ctx context.Context, id int) context.Context { + return context.WithValue(ctx, ctxKey{}, id) +} + +func UserID(ctx context.Context) (int, bool) { + id, ok := ctx.Value(ctxKey{}).(int) + return id, ok +} +``` + ### Anti-pattern ```go diff --git a/patterns/structs.md b/patterns/structs.md index 187bcd8..47d098e 100644 --- a/patterns/structs.md +++ b/patterns/structs.md @@ -13,6 +13,39 @@ explicit initialization. Nil fields fall back to sensible defaults at method cal self-documenting about its defaults. Users can write `var c http.Client` and start making requests. +**When to Use** + +**Triggers:** +- You're designing a type where the "empty" or "default" state is meaningful and safe +- Users should be able to write `var x MyType` and immediately call methods +- Your struct's nil/zero fields can fall back to sensible defaults at call time + +**Example — before:** +```go +type Cache struct { + store map[string][]byte + ttl time.Duration +} + +// Panics on zero value — store is nil! +func (c *Cache) Set(k string, v []byte) { c.store[k] = v } +``` + +**Example — after:** +```go +type Cache struct { + store map[string][]byte + ttl time.Duration // zero means no expiry +} + +func (c *Cache) Set(k string, v []byte) { + if c.store == nil { + c.store = make(map[string][]byte) // lazy init on first use + } + c.store[k] = v +} +``` + **Anti-pattern:** Requiring a constructor for basic use; panicking on zero-value use; requiring all fields be set before the type is functional. @@ -116,6 +149,39 @@ value alone. clearly communicates what's required. The constructor can set internal invariants (buffer sizes, split functions) that users shouldn't need to know about. +**When to Use** + +**Triggers:** +- Your type has mandatory dependencies that can't be expressed as zero values (an `io.Reader`, a DB connection) +- Internal invariants must be set up (buffer allocation, goroutine start) +- The type isn't useful without initialization (unlike `sync.Mutex` or `bytes.Buffer`) + +**Example — before:** +```go +type Parser struct { + lexer *Lexer + buf []Token + maxDepth int +} + +// User must know about all internal state: +p := &Parser{lexer: NewLexer(input), buf: make([]Token, 0, 64), maxDepth: 100} +``` + +**Example — after:** +```go +func NewParser(input io.Reader) *Parser { + return &Parser{ + lexer: NewLexer(input), + buf: make([]Token, 0, 64), + maxDepth: 100, + } +} + +// User writes: +p := NewParser(file) +``` + **Anti-pattern:** Forcing users to manually set unexported fields; having a constructor that takes 10 optional parameters (use config struct instead); requiring New when zero value would suffice. @@ -205,6 +271,35 @@ configuration knobs. Nil/zero values always mean "use the default". construct partially; serializable; the zero value works. This is Go's primary configuration pattern (preferred over functional options in the stdlib). +**When to Use** + +**Triggers:** +- Your constructor has 4+ optional parameters that would make a function signature unwieldy +- You want users to see all options in one place with godoc documentation +- Zero/nil values should mean "use the default" — no required fields beyond what the constructor demands + +**Example — before:** +```go +// 7 parameters — impossible to remember the order +func NewServer(addr string, handler http.Handler, readTimeout, writeTimeout time.Duration, + maxConns int, logger *log.Logger, tlsConfig *tls.Config) *Server { ... } +``` + +**Example — after:** +```go +type ServerConfig struct { + Addr string // ":8080" if empty + Handler http.Handler // http.DefaultServeMux if nil + ReadTimeout time.Duration // zero means no timeout + WriteTimeout time.Duration // zero means no timeout + MaxConns int // 1000 if zero + Logger *log.Logger // log.Default() if nil + TLSConfig *tls.Config // plain HTTP if nil +} + +func NewServer(cfg ServerConfig) *Server { ... } +``` + **Anti-pattern:** Undocumented fields; requiring all fields set; using sentinel values other than zero/nil for defaults; providing setters when direct assignment works. diff --git a/patterns/style.md b/patterns/style.md index 145f20b..de81cdf 100644 --- a/patterns/style.md +++ b/patterns/style.md @@ -110,6 +110,33 @@ build time. **Why:** Catches interface drift at compile time without creating an instance. The blank identifier discards the value — this is purely a static assertion. +**When to Use** + +**Triggers:** +- You've defined a type that MUST satisfy an interface (implements `io.Reader`, `http.Handler`, etc.) +- You want a compile-time guarantee that catches drift when you add/remove methods +- You're writing a package with multiple types implementing the same interface + +**Example — before:** +```go +type MyWriter struct{ buf bytes.Buffer } + +func (w *MyWriter) Write(p []byte) (int, error) { return w.buf.Write(p) } + +// Months later, someone renames Write to WriteBuf... +// No compile error — only discovered at runtime when passed as io.Writer +``` + +**Example — after:** +```go +// Compile-time check: if MyWriter stops implementing io.Writer, this fails to build +var _ io.Writer = (*MyWriter)(nil) + +type MyWriter struct{ buf bytes.Buffer } + +func (w *MyWriter) Write(p []byte) (int, error) { return w.buf.Write(p) } +``` + **Anti-pattern:** Relying on tests to catch interface conformance; skipping the check and discovering the mismatch at runtime; using reflection. @@ -312,6 +339,39 @@ are exhaustively listed together. **Why:** Type safety (can't accidentally pass an `os.Flag` where a `crypto.Hash` is expected). `iota` eliminates magic numbers. Grouping makes the full set visible. +**When to Use** + +**Triggers:** +- You have a set of related values that represent distinct states or options (status codes, modes, categories) +- Raw integers would be meaningless to readers (`SetMode(3)` vs `SetMode(ModeAsync)`) +- You want the type system to prevent passing a "color" where a "direction" is expected + +**Example — before:** +```go +func SetLogLevel(level int) { ... } + +// Caller: +SetLogLevel(3) // what does 3 mean?! +SetLogLevel(-1) // valid? who knows +``` + +**Example — after:** +```go +type LogLevel int + +const ( + LevelDebug LogLevel = iota + LevelInfo + LevelWarn + LevelError +) + +func SetLogLevel(level LogLevel) { ... } + +// Caller: +SetLogLevel(LevelWarn) // self-documenting +``` + **Anti-pattern:** Untyped numeric constants; separate `const` declarations for related values; using raw integers in function signatures. @@ -382,6 +442,34 @@ accidental mixing with raw int64 values. nanoseconds where seconds are expected. Methods provide conversion and formatting. Constants like `time.Second` make intent clear. +**When to Use** + +**Triggers:** +- A primitive type (`int`, `string`, `float64`) has a specific **semantic meaning** in your domain +- You want to attach methods (formatting, validation, arithmetic) to the value +- You've seen bugs from accidentally mixing units (`int` could be seconds, milliseconds, or nanoseconds) + +**Example — before:** +```go +func SetTimeout(ms int) { ... } // is this milliseconds? seconds? +func SetRetries(n int) { ... } // can't accidentally swap these... or CAN you? + +SetTimeout(5) // 5 what? +SetRetries(500) // oops, swapped arguments — compiles fine! +``` + +**Example — after:** +```go +type Timeout time.Duration +type RetryCount int + +func SetTimeout(t Timeout) { ... } +func SetRetries(n RetryCount) { ... } + +SetTimeout(Timeout(5 * time.Second)) // explicit units +SetRetries(RetryCount(3)) // can't swap — different types +``` + **Anti-pattern:** Using raw `int64` for durations; accepting `int` parameters for time intervals; mixing units (milliseconds in one place, seconds in another). diff --git a/patterns/testing-advanced.md b/patterns/testing-advanced.md index ecaba8c..25d7368 100644 --- a/patterns/testing-advanced.md +++ b/patterns/testing-advanced.md @@ -16,6 +16,55 @@ The canonical Go test style. Every Go stdlib test file uses this pattern. **Why:** Eliminates repetition, makes adding cases trivial, keeps the assertion logic in one place. Every test case gets the same verification path — no "special" cases hidden in different code paths. +**When to Use** + +**Triggers:** +- You're testing a function with many input/output combinations +- You're copy-pasting test functions that differ by one or two values +- Adding a new test case requires duplicating 10+ lines of setup/assertion code + +**Example — before:** +```go +func TestParseSize(t *testing.T) { + result1, err1 := ParseSize("10MB") + if err1 != nil || result1 != 10_000_000 { t.Error("10MB failed") } + + result2, err2 := ParseSize("1GB") + if err2 != nil || result2 != 1_000_000_000 { t.Error("1GB failed") } + + result3, err3 := ParseSize("invalid") + if err3 == nil { t.Error("invalid should fail") } + // ... 20 more copy-pasted blocks +} +``` + +**Example — after:** +```go +func TestParseSize(t *testing.T) { + tests := []struct { + input string + want int64 + wantErr bool + }{ + {"10MB", 10_000_000, false}, + {"1GB", 1_000_000_000, false}, + {"invalid", 0, true}, + {"0B", 0, false}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got, err := ParseSize(tt.input) + if (err != nil) != tt.wantErr { + t.Fatalf("ParseSize(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) + } + if got != tt.want { + t.Errorf("ParseSize(%q) = %d, want %d", tt.input, got, tt.want) + } + }) + } +} +``` + **Anti-pattern:** Writing individual assertions for each case, or copy-pasting test functions that differ by one input. **Code example (stdlib):** @@ -266,6 +315,48 @@ ServeFile(w, r, "testdata/file") 3. Golden files serve as documentation of expected behavior. 4. Reviewers can see exactly what output changed in diffs. +**When to Use** + +**Triggers:** +- Your function produces complex multi-line output (formatted code, HTML, JSON, error messages) +- Expected output would be 20+ lines if inlined in the test — unreadable +- Output changes intentionally sometimes and you need a quick way to approve the new version + +**Example — before:** +```go +func TestRenderTemplate(t *testing.T) { + got := renderHTML(data) + want := ` + +
You have 3 messages.
+ +` // 8 lines inline — and this is a SIMPLE template + if got != want { t.Errorf("mismatch") } +} +``` + +**Example — after:** +```go +var update = flag.Bool("update", false, "update golden files") + +func TestRenderTemplate(t *testing.T) { + got := renderHTML(data) + golden := filepath.Join("testdata", t.Name()+".golden") + if *update { + os.WriteFile(golden, []byte(got), 0644) + return + } + want, _ := os.ReadFile(golden) + if got != string(want) { + t.Errorf("output mismatch; run with -update to accept new output") + } +} +// Golden file lives at testdata/TestRenderTemplate.golden +``` + **Anti-pattern:** Comparing against inline expected strings that span 50+ lines, or manually constructing expected output. **Code example (stdlib):** @@ -319,6 +410,38 @@ func TestRewrite(t *testing.T) { **Why:** Fast, no network, no port allocation, no goroutines. Perfect for unit testing individual handlers in isolation. +**When to Use** + +**Triggers:** +- You're testing HTTP handler logic (status codes, headers, response body) in isolation +- You don't need real TCP connections, TLS, or routing +- Your test should run in <1ms, not wait for port binding + +**Example — before:** +```go +func TestHealthHandler(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(healthHandler)) + defer srv.Close() + resp, _ := http.Get(srv.URL + "/health") // real TCP connection — slow + if resp.StatusCode != 200 { t.Fatal("not healthy") } +} +``` + +**Example — after:** +```go +func TestHealthHandler(t *testing.T) { + req := httptest.NewRequest("GET", "/health", nil) + rec := httptest.NewRecorder() + healthHandler(rec, req) // direct call — no network + if rec.Code != 200 { + t.Fatalf("got status %d, want 200", rec.Code) + } + if rec.Body.String() != "ok" { + t.Errorf("body = %q, want %q", rec.Body.String(), "ok") + } +} +``` + **Anti-pattern:** Spinning up a full server to test handler logic that doesn't need networking. **Code example (stdlib):** diff --git a/smells/anti-patterns.md b/smells/anti-patterns.md index 7e120b7..1efcd4d 100644 --- a/smells/anti-patterns.md +++ b/smells/anti-patterns.md @@ -18,6 +18,30 @@ deploymentCopy := deployment.DeepCopy() deploymentCopy.Spec.Replicas = ptr.To[int32](3) ``` + +### When to Apply This Rule + +**Triggers:** +- You're reading from any shared data structure (cache, registry, concurrent map) +- Your function modifies a struct that was returned by a Lister, cache, or lookup +- You see code like `obj.Field = newValue` where `obj` came from a shared source + +**Example — detecting the smell:** +```go +// This line is the red flag: modifying something obtained from a shared cache +deployment, _ := deploymentLister.Get(name) +deployment.Spec.Replicas = ptr.To[int32](5) // SMELL: mutating cached object +client.Update(ctx, deployment) +``` + +**Example — fixed:** +```go +deployment, _ := deploymentLister.Get(name) +copy := deployment.DeepCopy() // isolate your mutation +copy.Spec.Replicas = ptr.To[int32](5) +client.Update(ctx, copy) +``` + **Evidence:** The `runtime.Object` interface *mandates* `DeepCopyObject()`. Every API type has generated deep copy methods. The entire architecture assumes immutable reads. --- @@ -50,6 +74,29 @@ func (q *Typed[T]) Get() (item T, shutdown bool) { **The pattern K8s enforces:** Level-triggered reconciliation. The `syncHandler` reads *current state from the cache*, computes *desired state from the spec*, and makes the world match: + +### When to Apply This Rule + +**Triggers:** +- Your handler says "X happened, so do Y" instead of "the world should look like Z, make it so" +- You're incrementing/decrementing counters based on events instead of counting actual state +- A missed event (network blip, restart) would cause permanent drift + +**Example — detecting the smell:** +```go +func onPodDeleted(pod Pod) { + deployment.Status.Replicas-- // edge-triggered: if we miss a delete, count is wrong forever +} +``` + +**Example — fixed:** +```go +func reconcile(deployment Deployment) { + actualPods := listPodsForDeployment(deployment) + deployment.Status.Replicas = int32(len(actualPods)) // level-triggered: always correct +} +``` + ```go // The sync function always reads current state, never relies on "what happened" func (dc *DeploymentController) syncDeployment(ctx context.Context, key string) error {