Skip to content

refactor(cli/checkers): package-level configPath DIP violation and guard pattern DRY #507

@hrygo

Description

@hrygo

Background

internal/cli/checkers 实现 hotplex doctor 诊断检查器注册表,包含 12+ 个 Checker 实现(配置、依赖、消息、安全、运行时)。模块约 700 行,7 个源文件。此次分析为首次 SOLID/DRY/coupling 审计。

Scope: SOLID, DRY, coupling — cycle 146
Key files: config.go, dependencies.go, runtime.go, messaging.go, stt.go, tts.go, security.go


Finding Summary

Category Critical High Medium Low
SOLID (DIP) 0 0 1 0
DRY 0 0 1 0
合计 0 0 2 0

Findings

configpath-package-level-dip-violation

Severity: Medium | Confidence: High | ROI: Medium
Location: config.go:17, config.go:20-22, config.go:31,108,147,211, messaging.go:112, stt.go:90, tts.go:107, security.go:70

Problem: 所有配置感知检查器依赖可变包级变量 var configPath string 而非构造函数注入。违反 DIP — 高层诊断逻辑依赖具体全局状态。测试文件明确注释无法使用 t.Parallel

Current Pattern:

// config.go:17 — shared mutable state
var configPath string

func SetConfigPath(p string) { configPath = p }

// config.go:31 — every checker copies this guard
func (c configLoadedChecker) Check() cli.Diagnostic {
    if configPath == "" {
        return cli.Diagnostic{Status: cli.StatusFail, Message: "Config path not set"}
    }
    cfg, err := config.Load(configPath)
    if err != nil {
        return cli.Diagnostic{Status: cli.StatusFail, Message: err.Error()}
    }
    // ... actual check logic
}

Proposed Fix:

type configLoadedChecker struct {
    configPath string
}

func NewConfigLoadedChecker(path string) *configLoadedChecker {
    return &configLoadedChecker{configPath: path}
}

func (c *configLoadedChecker) Check() cli.Diagnostic {
    cfg, diag, ok := loadConfig(c.configPath)
    if !ok { return diag }
    // ... actual check logic
}

Estimated Impact: 消除共享可变状态,启用 t.Parallel(),减少新检查器样板代码


configpath-guard-load-dry-repetition

Severity: Medium | Confidence: High | ROI: Medium
Location: config.go:31,108,147,211, messaging.go:112,121, tts.go:107,110, stt.go:90,93, security.go:70,121

Problem: if configPath == "" 守卫 + config.Load(configPath) + 错误处理模式在 6 个文件中重复 16 次。每个新的配置感知检查器必须复制相同的 8 行样板代码。

Current Pattern:

// Repeated in ~16 locations across 6 files:
if configPath == "" {
    return cli.Diagnostic{Status: cli.StatusFail, Message: "Config path not set"}
}
cfg, err := config.Load(configPath)
if err != nil {
    return cli.Diagnostic{Status: cli.StatusFail, Message: "Cannot load config: " + err.Error()}
}

Proposed Fix:

func loadConfig(path string) (*config.Config, cli.Diagnostic, bool) {
    if path == "" {
        return nil, cli.Diagnostic{Status: cli.StatusFail, Message: "Config path not set"}, false
    }
    cfg, err := config.Load(path)
    if err != nil {
        return nil, cli.Diagnostic{Status: cli.StatusFail, Message: "Cannot load config: " + err.Error()}, false
    }
    return cfg, cli.Diagnostic{}, true
}

Estimated Impact: ~120 行减少,消除新检查器组装错误风险

Acceptance Criteria:

  • 提取 loadConfig(path string) (*config.Config, cli.Diagnostic, bool) 辅助函数
  • 所有配置感知检查器通过结构体字段接收 configPath(或通过 loadConfig 辅助函数)
  • 测试可以使用 t.Parallel() 运行,无共享状态冲突
  • make test 通过,所有 checkers_test.go 零回归
  • hotplex doctor CLI 行为不变

Implementation Priority

Finding Priority Effort Risk Impact
configpath-package-level-dip-violation P2 Medium Low 消除共享可变状态,启用并行测试
configpath-guard-load-dry-repetition P2 Small Low ~120 行减少,降低新检查器错误率

Recommended starting point: 同时修复 — F1 的构造函数注入自然消除 F2 的重复


Out of Scope

  • sqlitePathChecker/dataDirWritableChecker 可写目录检查重复:Low 严重性,仅 2 实例
  • configRequiredCheckervar missing []string 死代码:逻辑 bug,非架构问题
  • checkPythonPackages/checkMossPythonPackages 相似模式:不同导入列表,提取收益低

Verification

  • make test 通过,无回归
  • make lint 不产生新警告
  • hotplex doctor 输出与修改前一致
  • TestConfig* 测试可并行运行

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Medium: tech debt, refactoring, improvementsarchitectureDomain: design patterns, coupling, separation of concernsarea/cliScope: cobra commands, service management, updaterefactorRefactor: DRY, SOLID, code quality improvements

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions