From 11048ae73ed8879a4d96196efbdd3315515b6bb7 Mon Sep 17 00:00:00 2001 From: Aaron Weiker Date: Thu, 30 Apr 2026 13:26:20 +0000 Subject: [PATCH] docs: add when-not to interfaces + error-handling + concurrency --- patterns/concurrency.md | 200 +++++++++++++++++++++++++++++++++++++ patterns/documentation.md | 61 +++++++++++ patterns/error-handling.md | 119 ++++++++++++++++++++++ 3 files changed, 380 insertions(+) diff --git a/patterns/concurrency.md b/patterns/concurrency.md index 24ed05e..d9913c4 100644 --- a/patterns/concurrency.md +++ b/patterns/concurrency.md @@ -71,6 +71,52 @@ func (s *Stats) RecordHit() { } ``` + +### When NOT to Use + +**Don't use this when:** +- Goroutines need to coordinate or communicate (not just protect state) — use channels +- The critical section involves blocking I/O (holding a mutex during network calls starves other goroutines) +- You can restructure to have a single goroutine own the state (no sharing = no lock needed) + +**Over-application example:** +```go +// Using a mutex to coordinate work between goroutines +type TaskQueue struct { + mu sync.Mutex + tasks []Task + ready bool +} + +func (q *TaskQueue) WaitForReady() { + for { + q.mu.Lock() + if q.ready { + q.mu.Unlock() + return + } + q.mu.Unlock() + time.Sleep(10 * time.Millisecond) // spin-waiting with a lock — terrible + } +} +``` + +**Better alternative:** +```go +// Use a channel for coordination/signaling +type TaskQueue struct { + tasks chan Task + ready chan struct{} +} + +func (q *TaskQueue) WaitForReady() { + <-q.ready // blocks efficiently, no spinning +} +``` + +**Why:** Mutexes protect data; channels coordinate goroutines. If you're polling a mutex-protected flag, you've reinvented a bad channel. The Go proverb applies: "Don't communicate by sharing memory; share memory by communicating." + + ### Idiomatic Usage ```go @@ -168,6 +214,63 @@ func GetDB() *sql.DB { } ``` + +### When NOT to Use + +**Don't use this when:** +- Initialization can fail and you need to retry — Once runs the func at most once, even on failure +- You need to reset/reinitialize later (Once has no Reset method) +- The initialization is cheap — just do it in `init()` or at declaration time + +**Over-application example:** +```go +var ( + conn *grpc.ClientConn + once sync.Once +) + +func GetConn() (*grpc.ClientConn, error) { + var err error + once.Do(func() { + conn, err = grpc.Dial("server:443") + }) + if err != nil { + return nil, err // PROBLEM: next call to GetConn returns nil conn, nil err + // because once.Do won't run again + } + return conn, nil +} +``` + +**Better alternative:** +```go +// Use sync.OnceValues (Go 1.21+) which caches both value and error, +// or handle retry logic explicitly +var getConn = sync.OnceValues(func() (*grpc.ClientConn, error) { + return grpc.Dial("server:443") +}) + +// Or for retry scenarios, use a mutex with a nil check +var ( + mu sync.Mutex + conn *grpc.ClientConn +) + +func GetConn() (*grpc.ClientConn, error) { + mu.Lock() + defer mu.Unlock() + if conn != nil { + return conn, nil + } + var err error + conn, err = grpc.Dial("server:443") // retries on next call if this fails + return conn, err +} +``` + +**Why:** `sync.Once` guarantees exactly-once execution regardless of success or failure. If the initialization can fail transiently (network timeout, service unavailable), Once will permanently cache the failure. Use `sync.OnceValues` for caching results, or a mutex with a nil-check pattern when retry is needed. + + ### Idiomatic Usage ```go @@ -409,6 +512,60 @@ func (s *Server) worker() { func (s *Server) Stop() { close(s.done) } // broadcasts to ALL workers ``` + +### When NOT to Use + +**Don't use this when:** +- You need to send actual data between goroutines — use a typed channel +- You only have one goroutine to signal — a simple `return` or function call suffices +- You're using it as a poor substitute for `context.Context` (which already provides Done()) + +**Over-application example:** +```go +// Rolling your own done channel when context already provides this +type Worker struct { + done chan struct{} + ctx context.Context + cancel context.CancelFunc +} + +func (w *Worker) Start() { + go func() { + select { + case <-w.done: // redundant — ctx.Done() does the same thing + return + case <-w.ctx.Done(): + return + } + }() +} +``` + +**Better alternative:** +```go +// Just use the context — it already IS a done signal +type Worker struct { + ctx context.Context + cancel context.CancelFunc +} + +func (w *Worker) Start() { + go func() { + select { + case <-w.ctx.Done(): + return + case work := <-w.workCh: + process(work) + } + }() +} + +func (w *Worker) Stop() { w.cancel() } +``` + +**Why:** If you already have a `context.Context`, its `Done()` channel is your cancellation signal. Adding a separate `chan struct{}` duplicates functionality and creates two shutdown paths that must be kept in sync. Use raw done channels only when you don't have a context (e.g., standalone libraries that predate context). + + ### Anti-pattern ```go @@ -648,6 +805,49 @@ func processAll(ctx context.Context, items []string) []Result { } ``` + +### When NOT to Use + +**Don't use this when:** +- The "pipeline" has only one stage — you're just adding goroutine overhead to sequential code +- All items are already in memory and processing is CPU-bound with no I/O — `for` loop is simpler and faster +- You don't actually need backpressure (data fits in memory, producer is finite) + +**Over-application example:** +```go +// Channel pipeline for a simple in-memory transformation +func doubleAll(nums []int) []int { + ch := make(chan int) + go func() { + defer close(ch) + for _, n := range nums { + ch <- n // channel overhead for no benefit + } + }() + + var result []int + for n := range ch { + result = append(result, n*2) + } + return result +} +``` + +**Better alternative:** +```go +// Just use a loop — no concurrency needed +func doubleAll(nums []int) []int { + result := make([]int, len(nums)) + for i, n := range nums { + result[i] = n * 2 + } + return result +} +``` + +**Why:** Channel pipelines shine when stages involve I/O (network, disk) and can overlap waiting. For pure computation on in-memory data, the channel send/receive overhead (~50-100ns per item) adds up and the goroutine scheduling has no useful work to overlap with. A plain loop is faster, simpler, and easier to debug. + + ### Anti-pattern ```go diff --git a/patterns/documentation.md b/patterns/documentation.md index d728a58..a192fc8 100644 --- a/patterns/documentation.md +++ b/patterns/documentation.md @@ -197,6 +197,37 @@ func ExampleProcess() { // ← go test verifies this compiles and produces the expected output ``` +**When NOT to Use** + +**Don't write Example tests when:** +- The function signature is self-explanatory (`func Max(a, b int) int` doesn't need an example) +- The example would just restate the doc comment with no additional insight +- You're testing internal/unexported functions (examples must use the public API) +- The output is non-deterministic (timestamps, random values, goroutine ordering) + +**Over-application example:** +```go +// Pointless — the signature tells you everything +func ExampleAbs() { + fmt.Println(math.Abs(-5)) + // Output: + // 5 +} +``` + +**Better alternative:** +```go +// Skip the example for trivial functions. Write examples for non-obvious behavior: +func ExampleAbs_nan() { + fmt.Println(math.Abs(math.NaN())) + // Output: + // NaN +} +// ← This teaches something surprising that the signature doesn't convey +``` + +**Why:** Examples exist to teach usage patterns that aren't obvious from the type signature and doc comment. Trivial examples add maintenance burden without teaching anything. + **Anti-pattern:** Examples that don't compile; examples without Output comments (not verified); examples in README that drift from reality. @@ -319,6 +350,36 @@ func ParseDuration(s string) (time.Duration, error) { } ``` +**When NOT to Use** + +**Don't deprecate when:** +- The function still works fine and there's no better replacement yet +- You're deprecating to "clean up" the API without a migration path (users will just ignore it) +- The replacement has a different behavior or isn't a drop-in substitute (document the difference instead) +- You're on v0.x and can just remove it (pre-1.0 allows breaking changes) + +**Over-application example:** +```go +// Deprecated: Use NewClientV2 instead. +func NewClient(addr string) *Client { ... } + +// But NewClientV2 has a completely different API: +func NewClientV2(cfg Config) (*ClientV2, error) { ... } +// Users can't just find-and-replace — this isn't a deprecation, it's a migration +``` + +**Better alternative:** +```go +// NewClient creates a client with default configuration. +// For advanced configuration (TLS, timeouts, connection pooling), +// use [NewClientWithConfig] instead. +func NewClient(addr string) *Client { + return NewClientWithConfig(Config{Addr: addr}) +} +``` + +**Why:** Deprecation means "there's a better way to do the same thing." If the replacement requires a fundamentally different approach, provide a migration guide — don't just slap `Deprecated:` on it and leave users stranded. + **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 615cb58..72b62a6 100644 --- a/patterns/error-handling.md +++ b/patterns/error-handling.md @@ -74,6 +74,48 @@ func fetchUser(id int) (*User, error) { } ``` + +### When NOT to Use + +**Don't use this when:** +- The error condition is internal and no caller should branch on it — use `fmt.Errorf` instead +- You have too many sentinels (>5-6 per package) — consider a custom error type with a code field +- The error carries varying context (file path, user ID) — sentinels are for fixed conditions + +**Over-application example:** +```go +// A sentinel for every possible failure — explosion of package-level vars +var ( + ErrUserNotFound = errors.New("users: not found") + ErrUserInactive = errors.New("users: inactive") + ErrUserSuspended = errors.New("users: suspended") + ErrUserRateLimited = errors.New("users: rate limited") + ErrUserInvalidEmail = errors.New("users: invalid email") + ErrUserInvalidName = errors.New("users: invalid name") + ErrUserInvalidAge = errors.New("users: invalid age") + // 20 more... +) +``` + +**Better alternative:** +```go +// Use a typed error with a code when you have many distinct conditions +type UserError struct { + Code string // "not_found", "inactive", "suspended" + Field string // which field failed validation + Message string +} + +func (e *UserError) Error() string { return "users: " + e.Message } + +// Callers use errors.As to inspect +var uerr *UserError +if errors.As(err, &uerr) && uerr.Code == "not_found" { ... } +``` + +**Why:** Sentinels are for a small number of well-known states that callers frequently branch on. If you're creating dozens, you've outgrown the pattern — a structured error type with an enum/code field scales better and avoids polluting the package namespace. + + ### Anti-pattern ```go @@ -217,6 +259,43 @@ func loadConfig(path string) (*Config, error) { - **%w**: When the wrapped error is part of your API contract. Callers can depend on it. - **%v**: When you want to include the error text but NOT let callers depend on the underlying type. Use for implementation details. + +### When NOT to Use + +**Don't use this when:** +- You're wrapping at every single layer — the error message becomes `"a: b: c: d: e: permission denied"` +- The added context is obvious from the function name (the caller already knows what function they called) +- You're wrapping errors from a dependency you don't want in your API contract (use `%v` instead) + +**Over-application example:** +```go +func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) { + user, err := s.getUser(r) + if err != nil { + // "handleRequest: getUser: fetchFromDB: queryRow: scan: sql: no rows" + // The handler name adds nothing — the caller IS the handler + http.Error(w, fmt.Errorf("handleRequest: %w", err).Error(), 500) + return + } +} +``` + +**Better alternative:** +```go +func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) { + user, err := s.getUser(r) + if err != nil { + // Log the full chain internally, return a clean message to the client + slog.Error("fetching user", "err", err, "path", r.URL.Path) + http.Error(w, "internal error", 500) + return + } +} +``` + +**Why:** Wrapping should add *meaningful* context that isn't already obvious. At HTTP handler boundaries, you typically log the error (with full chain) and return a generic response. Wrapping at every layer creates unreadable error strings and leaks internals. + + ### Anti-pattern ```go @@ -419,6 +498,46 @@ func cleanup(db *sql.DB, cache *redis.Client, file *os.File) error { } ``` + +### When NOT to Use + +**Don't use this when:** +- Errors are causally related (error B was caused by error A) — use wrapping (`%w`) instead +- You're collecting errors across retries of the *same* operation — return the last error or wrap them causally +- The caller needs to distinguish which resource failed — Join loses that information + +**Over-application example:** +```go +func fetchWithRetry(url string, attempts int) error { + var errs []error + for i := 0; i < attempts; i++ { + err := fetch(url) + if err == nil { + return nil + } + errs = append(errs, err) // collecting retry errors — misleading + } + return errors.Join(errs...) // caller sees 3 errors but they're the SAME operation +} +``` + +**Better alternative:** +```go +func fetchWithRetry(url string, attempts int) error { + var lastErr error + for i := 0; i < attempts; i++ { + lastErr = fetch(url) + if lastErr == nil { + return nil + } + } + return fmt.Errorf("fetch %s: %d attempts failed, last: %w", url, attempts, lastErr) +} +``` + +**Why:** `errors.Join` is for *independent* failures that all matter equally. For retries, the caller cares about the final failure and the retry count, not a list of (often identical) errors. For causal chains, wrapping preserves the relationship between cause and effect. + + ### Anti-pattern ```go