# Smell Hunter Go > Audit Go code for classic code smells — feature envy, data clumps, shotgun surgery, primitive obsession, temporal coupling, comments as deodorant, temporary fields, init() abuse, package-level mutable state, and stuttering names. Use when: reviewing Go code for structural design problems, preparing for a refactor, auditing code after rapid feature development, or hunting for misplaced responsibilities. - Skill: `skyosev/smell-hunter-go` (Agent Skill) - Install (CLI): `npx skillmds@latest add skyosev/smell-hunter-go` - Raw SKILL.md: https://api.skillmd.com/api/skills/skyosev/smell-hunter-go/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: skyosev (https://skillmd.com/u/skyosev) - Updated: 2026-09-21 - Page: https://skillmd.com/skills/skyosev/smell-hunter-go --- # Smell Hunter Audit Go code for **code smells** — structural patterns that indicate deeper design problems. This covers selected Fowler/Beck smells and Go-specific antipatterns that fall outside the scope of specialized hunters (SOLID, type design, boundaries, invariants, etc.). The goal: **data lives where it's used, changes are localized, domain concepts are modeled explicitly, and Go idioms are respected.** **Not covered (owned by other hunters):** long method / mixed concerns (→ simplicity-hunter), dead code (→ simplicity-hunter / slop-hunter), speculative generality (→ simplicity-hunter), magic numbers (→ doc-hunter), boolean parameters (→ simplicity-hunter, which carries the calibrated do-not-flag rule), interface pollution (→ simplicity-hunter / solid-hunter). See Operating Constraints for handoff rules. Code smells are symptoms, not diagnoses. Each finding indicates a *likely* design problem that warrants investigation. Context determines whether the smell is a genuine issue or an acceptable trade-off. ## When to Use - Reviewing code for structural design problems before a refactor - Auditing code after rapid feature development or prototyping - Hunting for misplaced responsibilities and data - Identifying missing domain types and abstractions - Preparing a codebase for long-term maintainability - Complementing specialized hunters with cross-cutting smell detection ## Core Principles 1. **Smells are symptoms, not diseases.** A code smell indicates a probable design problem, not a guaranteed one. Evaluate each smell in context — some are intentional trade-offs. The goal is awareness, not mechanical elimination. 2. **Follow the data.** Feature envy, data clumps, and primitive obsession all point to misplaced or undermodeled data. When data and behavior want to be together, let them. When a group of values always travels together, they are a missing type. 3. **One change, one place.** Shotgun surgery means a single logical change requires edits across many unrelated files. This is the hallmark of misaligned module boundaries or scattered responsibilities. The fix is cohesion. 4. **Comments should not be deodorant.** A comment explaining confusing code is a band-aid over a design problem. The fix is clearer code — better names, extracted functions, simpler structure — not more comments. 5. **Model the domain.** Primitive obsession and data clumps often indicate missing domain types. A `string` that represents an email address, a pair of `float64`s that represent a coordinate, or a group of parameters that always appear together — these are domain concepts begging for a type. 6. **Respect Go idioms.** Go has specific conventions: no stuttering names, minimal init(), explicit state management, composition over inheritance. Language-specific smells are as important as universal ones. 7. **Refactor incrementally.** Split by responsibility, not by size. Introduce abstraction only when needed (wait for the second use case). Preserve behavior first — add tests before restructuring. ## What to Hunt ### 1. Feature Envy A function or method that uses more data from another package or struct than from its own receiver or local state. **Signals:** - Method on struct A that primarily accesses fields/methods of struct B (passed as parameter) - Function that accesses many fields of a struct from another package - Helper function that exists in package A but only operates on types from package B - Method that receives a struct and calls 3+ of its methods while using none of its own receiver's fields - A function whose name suggests it belongs to the type it operates on, not the package it lives in **Action:** Move the function to the package that owns the data it operates on (you can only define methods on types in the same package — move the function near its data, minding import cycles). If it uses data from two types equally, consider whether the shared data should be extracted into its own type. ### 2. Data Clumps The same group of parameters or fields that always appear together across multiple function signatures or struct definitions. **Signals:** - 3+ parameters that appear together in multiple function signatures (e.g., `host`, `port`, `scheme` or `latitude`, `longitude`, `altitude`) - Multiple structs with the same subset of fields (e.g., `Street`, `City`, `Zip`, `Country` in several types) - Functions that pass a group of related values individually instead of as a struct - Struct with fields that form a logical sub-group (e.g., `StartDate`, `EndDate`, `Timezone` inside a larger config) **Action:** Extract the group into a named struct. Replace the individual parameters/fields with the struct. If the group appears only in function signatures, create a parameter struct. If it appears in multiple struct definitions, extract a shared embedded or composed type. ### 3. Shotgun Surgery A single logical change requires edits across many unrelated files or packages. **Signals:** - Adding a new field to a domain type requires updating 5+ files (handlers, validators, mappers, tests, serializers) - Adding a new error type requires changes in the error package, every handler, and every test - A config change requires editing code in multiple packages rather than just the config package - Renaming a concept requires find-and-replace across the entire codebase - A new feature flag requires changes in config, middleware, handlers, and templates **Action:** Consolidate the scattered responsibility. If a change to concept X requires touching packages A, B, C, D, and E, then X's logic is spread too thin. Consider: - A registry or map-based dispatch instead of scattered switch cases - Code generation for boilerplate that varies with each new type/field - A single package that owns the concept end-to-end ### 4. Primitive Obsession Using primitive types (`string`, `int`, `float64`, `bool`) for domain concepts that deserve their own named types. **Ownership:** primitive obsession as *domain modeling* is owned here; type-hunter keeps only alias-vs-named-type *mechanics* (`type X = Y` misuse, method-less named types). Boolean parameters belong to simplicity-hunter. (No tension with boundary-hunter's "primitives flow" principle: both resolve in shared domain types — a `UserID` named type flowing between packages satisfies both.) **Signals:** - Functions that accept `string` parameters for IDs, emails, URLs, currencies, or status codes - Functions where two `string` or `int` parameters could be accidentally swapped (e.g., `Transfer(from, to string, amount int)`) - Validation logic for a "typed" string scattered across multiple call sites instead of enforced at construction - `float64` used for money calculations (precision loss) - `int` used for durations without unit clarity (seconds? milliseconds?) - Raw `string` comparisons for status/state values instead of typed constants **Action:** Define a named type: `type UserID string`, `type Email string`, `type Money int64` (cents). Add a constructor that validates. The type system then prevents mixing `UserID` with `OrderID` at compile time. ### 5. Temporal Coupling Functions or operations that must be called in a specific order, but nothing in the API enforces that order. **Ownership:** the coupling and the redesign recommendation are owned here; doc-hunter's ordering section covers only constraints that are *staying* (redesign rejected or out of scope) and need documenting. **Signals:** - A struct with `Init()`, `Setup()`, or `Configure()` methods that must be called before `Run()` or `Process()` - Documentation or comments that say "must call X before Y" - Panic or nil dereference when methods are called in the wrong order - A builder pattern where `Build()` can be called before required fields are set - State machine transitions that are valid only from certain states but not enforced by the API **Action:** Redesign the API to make the order implicit: - Use constructor functions that return a fully initialized struct - Use the builder pattern with a `Build()` that validates all required fields - Use state-machine types where each state is a different type with only valid transitions as methods - Accept dependencies in the constructor rather than via separate `Set*` methods ### 6. Comments as Deodorant Comments that explain *what* confusing code does rather than *why* — masking a design problem instead of fixing it. **Comment ownership rule** (stated identically in doc-hunter, slop-hunter, and smell-hunter): - Comment absent and the "why" non-obvious → doc-hunter (add the missing "why" comment). - Comment present and the code trivial → slop-hunter (delete the redundant comment). - Comment present and the code non-trivial → smell-hunter (extract/refactor; the comment is deodorant). **Signals:** - A comment like `// Convert the user data to the format expected by the billing system` above a 20-line block that should be an extracted function named `toBillingFormat()` - Comments that explain complex boolean expressions rather than extracting them into named functions - Inline comments at each step of a long function, effectively creating "sections" that should be separate functions - Comments explaining workarounds for the code's own design rather than external constraints - `// This is confusing because...` — if you're writing this comment, refactor instead **Action:** Replace the comment with a code change: - Extract the commented block into a function with an intent-revealing name - Replace complex expressions with named variables or helper functions - Split long functions at the comment boundaries into named sub-functions - Only keep comments that explain *why* (business rules, external constraints, workarounds for third-party bugs) ### 7. Temporary Field Struct fields that are meaningful only in certain states or during specific operations — they are set for one code path and nil/zero for all others. **Signals:** - Struct field that is non-nil in only one of several code paths - Fields that are set in method A and read in method B but meaningless in methods C, D, E - Optional fields that exist because the struct is used in multiple contexts with different data requirements - Fields documented as "only valid when X is true" or "set only during processing" - Struct with fields like `tempResult`, `cachedX`, `lastError` that serve a single transient use **Action:** Extract the temporary fields into a separate struct used only where needed. If the struct represents multiple states, use a discriminated pattern: separate structs for each state, or a method-scoped local variable instead of a field. ### 8. init() Abuse Complex logic, side effects, or non-trivial initialization in `init()` functions. **Signals:** - `init()` that opens database connections, network sockets, or file handles - `init()` that reads environment variables or configuration files - `init()` with error handling (errors in init cannot be returned, only panicked) - `init()` that registers global state (e.g., `http.HandleFunc` in library code) - Multiple `init()` functions in the same package with ordering dependencies - `init()` that makes the package untestable (side effects on import) **Action:** Move initialization logic to explicit constructor functions or a `Setup()` function called from `main()`. Reserve `init()` for truly static registrations (e.g., `database/sql` driver registration, codec registration) that have no error conditions and no external dependencies. If `init()` can fail, it should not be in `init()`. ### 9. Package-Level Mutable State Global variables at the package level that hold mutable state, creating hidden coupling and test interference. **Signals:** - `var db *sql.DB` or `var client *http.Client` at package level, modified at runtime - `var cache = map[string]interface{}{}` at package level, written to by multiple functions - Package-level `sync.Mutex` protecting package-level state (sign of a god package) - `var defaultConfig = Config{...}` that is mutated by `SetConfig()` functions - Global loggers modified at runtime: `var logger = log.New(...)` with `SetLogger()` functions - Package-level `var once sync.Once` with `var instance *Service` (singleton pattern) **Do not flag:** - `var` declarations that are effectively constant lookup tables (e.g., `var weekdayMap = map[string]time.Weekday{...}`) when there is no write path after initialization. Go does not support `const` maps, so `var` is the only option. Check for: (1) explicit annotations like `nolint: gochecknoglobals` with "read-only" comments, (2) absence of any assignment to the variable outside its declaration. The smell is actual mutation (writes after init), not the `var` keyword on a read-only structure. **Action:** Move state into struct instances passed via dependency injection. If truly global state is needed (rare), encapsulate it in a struct with a constructor and pass the struct explicitly. Package-level constants and immutable variables (`var Version = "1.0"`) are fine — the smell is mutability. ### 10. Stuttering Names Exported identifiers that repeat the package name, violating Go's naming convention where the package name provides context. **Boundary with slop-hunter:** stuttering as a package-design smell is owned here; slop-hunter owns Go naming-convention *drift introduced by the audited change* (its diff orientation). **Signals:** - `user.UserName` instead of `user.Name` - `config.ConfigOption` instead of `config.Option` - `http.HTTPClient` instead of `http.Client` (Go standard library gets this right) - `auth.AuthMiddleware` instead of `auth.Middleware` - `db.DBConnection` instead of `db.Connection` - `errors.ErrorCode` instead of `errors.Code` **Action:** Remove the package-name prefix from the identifier. In Go, `user.Name` reads as "the Name in the user package" — the package name already provides the context. Stuttering adds noise and violates the convention documented in Effective Go. ## Audit Workflow ### Phase 1: Gain Context 1. **Resolve audit surface.** The prompt may specify the scope as: - **Diff**: files changed relative to the base branch — committed, staged, unstaged, and untracked - **Path**: specific files, folders, or packages - **Codebase**: the entire project (the default when unspecified; set `SCOPE=.`) **Party mode:** when the orchestrator supplies a scope snapshot (a resolved file list), use it verbatim and do not re-resolve. The resolution below applies to standalone runs only. For diff mode, resolve fail-closed: ```bash BASE=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/@@') if [ -z "$BASE" ]; then for b in origin/main origin/master main master; do git rev-parse -q --verify "$b" >/dev/null && BASE=$b && break done fi # If BASE is still empty: STOP. Ask for an explicit base. Do not continue. SCOPE=$( { git diff --name-only --diff-filter=d "$BASE"...HEAD; git diff --name-only --diff-filter=d HEAD; git ls-files --others --exclude-standard; } | sort -u ) DELETED=$( { git diff --name-only --diff-filter=D "$BASE"...HEAD; git diff --name-only --diff-filter=D HEAD; } | sort -u ) ``` If `$SCOPE` is empty, run no scans: write the report with "Audit completed: 0 findings — empty diff scope", listing `$DELETED` under "Deleted in diff" if non-empty, and stop. If the resolved surface exceeds what can be read within the context budget, report the file count and ask to narrow or chunk. **Two surfaces.** Findings are reported only against the **target scope** (`$SCOPE`) — every finding anchors (file:line) there. Related files may still be *read* as **context**: judging feature envy or a data clump often requires reading the envied type's package. The co-change analysis (Phase 4) reads git history project-wide by nature; its findings still anchor to in-scope packages. 2. Understand the project's domain model — what are the core entities, value objects, and operations? 3. Note the project's conventions for naming, struct design, and package organization. ### Phase 2: Scan for Smell Candidates These scans produce **candidates only** — each match requires manual validation in Phase 3–5 before it becomes a finding. Expect a high false-positive rate from regex heuristics; the value is in surfacing locations to inspect. For diff/path mode, append the resolved file list (`$SCOPE`) to each `rg` command. For codebase mode, omit it. ```bash EXCLUDE='--glob !**/*_test.go --glob !**/vendor/** --glob !**/testdata/** --glob !**/*.pb.go --glob !**/*_gen.go --glob !**/*_generated.go --glob !**/mock_*.go --glob !**/mocks/**' # Feature envy: methods that heavily reference another package's types # (look for pkg.Type.Field or pkg.Func().Method patterns — then verify manually) rg --pcre2 '\b[a-z]\w+\.[A-Z]\w+\.' --type go $EXCLUDE -- $SCOPE # Data clumps: repeated parameter groups (functions with 4+ params) rg --pcre2 'func\s+(\(\w+\s+\*?\w+\)\s+)?\w+\([^)]{100,}\)' --type go $EXCLUDE -- $SCOPE # Primitive obsession: functions with 2+ bare string/int params in sequence rg --pcre2 'func.*\(\s*\w+\s+string\s*,\s*\w+\s+string' --type go $EXCLUDE -- $SCOPE # Temporal coupling: Init/Setup/Configure methods on receiver types rg --pcre2 'func\s+\(\w+\s+\*?\w+\)\s+(Init|Setup|Configure|Prepare)\b' --type go $EXCLUDE -- $SCOPE # init() functions (then inspect for side effects) rg '^func init\(\)' --type go $EXCLUDE -- $SCOPE # Package-level mutable state (var declarations — filter for mutability manually) rg '^var\s+\w+\s' --type go $EXCLUDE -- $SCOPE # Stuttering names (package name repeated in exported identifiers) # Manual check per package — compare package name to exported symbol prefixes # Comments as deodorant: multi-line comment blocks before code (inspect for "what" vs "why") rg -B1 -A1 '^\s*// ' --type go $EXCLUDE -- $SCOPE | head -200 # Shotgun surgery: per-commit co-occurrence (see Phase 4) ``` ### Phase 3: Evaluate Feature Envy and Data Clumps For each function with cross-package data access: - Does it access more fields/methods from another type than from its receiver? - Would moving it to the other type's package improve cohesion? For each function with 4+ parameters: - Do the same parameters appear together in other function signatures? - Is there a domain concept these parameters represent? ### Phase 4: Evaluate Shotgun Surgery Shotgun surgery is detected through **per-commit co-change analysis**, not raw file churn. High churn on a single file is not shotgun surgery — the signal is many *unrelated* files changing together for a single logical change. ```bash # Per-commit file sets: show which files change together in each commit git log --pretty=format:'--- %h %s' --name-only -30 | head -200 # Package co-occurrence: for each commit, list distinct packages touched. # Each commit's package set is sorted before printing (awk's `for (p in pkgs)` order is unspecified, # and uniq -c only groups identical lines); the END block flushes the final commit; root-level files # (no "/") count as package ".". git log --pretty=format:'COMMIT' --name-only -50 | awk ' function flush( n, i, j, t, keys, s) { n = 0 for (p in pkgs) keys[++n] = p if (n) { for (i = 1; i < n; i++) for (j = i + 1; j <= n; j++) if (keys[j] < keys[i]) { t = keys[i]; keys[i] = keys[j]; keys[j] = t } s = keys[1] for (i = 2; i <= n; i++) s = s " " keys[i] print s } delete pkgs } /^COMMIT/ { flush(); next } NF { if ($0 ~ /\//) sub(/\/[^\/]*$/, ""); else $0 = "." pkgs[$0] = 1 } END { flush() } ' | sort | uniq -c | sort -rn | head -20 ``` For each commit that touches 4+ packages, ask: was this a single logical change scattered across unrelated modules, or a legitimate cross-cutting concern? Look for patterns: the same package set appearing in multiple commits suggests structural coupling. ### Phase 5: Evaluate Go-Specific Smells For each `init()` function: - Does it have side effects (I/O, network, global state)? - Can it fail? Does it panic? - Does it make the package hard to test? For each package-level `var`: - Is it mutable? Is it written to after initialization? - Is it accessed from multiple goroutines? For each exported identifier: - Does the name repeat the package name? ### Phase 6: Produce Report ## Output Format Save as `YYYY-MM-DD-smell-hunter-audit-{model-name}.md` — `{model-name}` is the executing model's short name (e.g. `fable-5`) — in the project's docs folder (or project root if no docs folder exists). If the caller specifies an output path or return mode (e.g. the party-hunter orchestrator), it overrides this default. Severity levels, used for per-finding labels and the Recommendations grouping: - **Critical** — exploitable now, causes data loss, or breaks behavior on production paths. - **High** — a defect with likely user-visible, security, or reliability impact if left unaddressed. - **Medium** — correctness or maintainability risk without imminent impact. - **Low** — hygiene; no behavioral risk. ```md # Smell Hunter Audit — {date} ## Scope - Surface: {diff / path / codebase} - Files: {count or list} - Exclusions: {list} - {Deleted in diff: {list} — only for diff scope with deletions} - Audit completed: {N} findings ## Findings ### Feature Envy | # | Location | Function | Envied Type | Own Data Used | Foreign Data Used | Evidence | Action | | - | -------- | -------- | ----------- | ------------- | ----------------- | -------- | ------ | | 1 | file:line | `FormatOrder()` | `billing.Invoice` | 0 fields | 5 fields | `inv.Total + inv.Tax...` | Move to `billing` package | ### Data Clumps | # | Locations | Parameters/Fields | Evidence | Suggested Type | Action | | - | --------- | ----------------- | -------- | -------------- | ------ | | 1 | file:line, file:line, file:line | `host, port, scheme` | 3 func signatures | `Endpoint` struct | Extract type | ### Shotgun Surgery | # | Concept | Files Touched | Packages Touched | Action | | - | ------- | ------------- | ---------------- | ------ | | 1 | "Add new payment method" | 8 files | 5 packages | Consolidate payment logic | ### Primitive Obsession | # | Location | Parameter/Field | Current Type | Evidence | Suggested Type | Action | | - | -------- | --------------- | ------------ | -------- | -------------- | ------ | | 1 | file:line | `userID string` | `string` | swappable with `orderID string` | `UserID` | Define named type with constructor | ### Temporal Coupling | # | Location | Struct | Required Order | Action | | - | -------- | ------ | -------------- | ------ | | 1 | file:line | `Server` | `Init()` → `Start()` | Require deps in constructor | ### Comments as Deodorant | # | Location | Comment | Action | | - | -------- | ------- | ------ | | 1 | file:line | `// Parse and validate the user input` | Extract `parseAndValidateInput()` | ### Temporary Field | # | Location | Struct | Field | Used In | Action | | - | -------- | ------ | ----- | ------- | ------ | | 1 | file:line | `Processor` | `lastResult` | `Process()` only | Move to method-local var or return value | ### init() Abuse | # | Location | Side Effects | Action | | - | -------- | ------------ | ------ | | 1 | file:line | Opens DB connection, reads env vars | Move to explicit `Setup()` called from main | ### Package-Level Mutable State | # | Location | Variable | Mutated By | Action | | - | -------- | -------- | ---------- | ------ | | 1 | file:line | `var defaultClient` | `SetClient()` | Inject via struct field | ### Stuttering Names | # | Location | Current Name | Suggested Name | Action | | - | -------- | ------------ | -------------- | ------ | | 1 | file:line | `user.UserName` | `user.Name` | Rename | ## Recommendations (Priority Order) 1. **High**: {data clumps with 5+ occurrences, primitive obsession causing type confusion, init() with error paths} 2. **Medium**: {feature envy, shotgun surgery patterns, package-level mutable state, temporal coupling} 3. **Low**: {stuttering names, comments as deodorant, temporary fields} ``` ## Operating Constraints - **No code edits.** This skill produces an audit report only. Implementation is a separate step. - **No empty finding sections.** Include only categories with findings. Omit a heading, table, or list entirely when it would contain zero items — do not include empty tables, placeholder subsections, or negative statements like "no dead exports", "none found", or "no issues". Execution status is exempt: the "Audit completed: N findings" line in the Scope section is always present, even at zero findings. - **Scope: classic code smells and Go-specific antipatterns only.** If a finding doesn't answer "is this a structural smell?", it belongs to another hunter — do not flag it here. Named boundaries: primitive obsession as domain modeling is owned here (type-hunter keeps alias mechanics); temporal coupling and its redesign are owned here (doc-hunter documents constraints that stay); boolean parameters belong to simplicity-hunter; apply the comment ownership rule (§6) for the doc/slop/smell split; stuttering is owned here, naming drift in the audited change belongs to slop-hunter. - **Evidence required.** Every finding must cite `file/path.go:line` with the exact code. - **Context matters.** A smell in a prototype is less urgent than a smell in a payment system. Assess severity relative to the code's criticality and change frequency. - **Pragmatism.** Not every smell requires action. A data clump that appears twice may not justify a new type. Feature envy in a utility function may be intentional. Report the smell, assess the cost/benefit, and let the team decide. - **Respect Go idioms.** Go's `error` return pattern, explicit control flow, and composition-based design are not smells. Calibrate to Go conventions, not patterns from other languages. - **Handoff, not duplication.** Smells often have root causes owned by other hunters. Smell-hunter owns the *symptom detection* (e.g., "these 8 files always change together"); the root-cause fix may belong to boundary-hunter (dependency direction), solid-hunter (SRP), or simplicity-hunter (mixed concerns). When a finding clearly belongs to another hunter's domain, note it as a cross-reference in the report and do not duplicate the analysis. When the smell is the primary signal and no other hunter covers the detection method, own the finding. - **Assess refactoring risk briefly.** Actions are recommendations, not commands — implementation is a separate step. When a recommendation would affect exported API surface, serialization behavior, or could introduce import cycles, note the risk in the Action column (e.g., "Extract type — verify no external importers").