Skip to content

test: add text statistics tracking#18

Closed
ZhaoChaoqun wants to merge 1 commit into
mainfrom
test/review-persona-comparison
Closed

test: add text statistics tracking#18
ZhaoChaoqun wants to merge 1 commit into
mainfrom
test/review-persona-comparison

Conversation

@ZhaoChaoqun

Copy link
Copy Markdown
Owner

Purpose

Test PR to compare Claude review persona (Linus Torvalds) vs no-persona reviews.

This file intentionally contains several code quality issues for the reviewer to catch:

  • Singleton with mutable state (thread safety)
  • Force unwraps instead of optional binding
  • Empty if/else branches (dead code)
  • Unbounded array growth (memory leak)
  • print() instead of os.Logger
  • God method with too many responsibilities
  • [String: Any] dictionary instead of typed struct

Test Plan

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 Code Review — 4/10

我说真的,这代码是不是写来故意恶心人的?

正确性 (1/3)

线程安全?不存在的。 一个 singleton,所有 mutable 属性全是 var,没有任何同步机制。在 macOS app 里,多个线程同时调一下 processText()getStats(),你就等着享受数据竞争带来的「薛定谔的统计数据」吧。

allTexts.append(text) 之后检查 count > 10000,然后 if body 是空的——你是在写代码还是写待办事项?这个数组会无限增长,用户跑一天,内存就吃光了。这不是 bug,这是内存泄漏

简洁性 (1/2)

duration 参数连续 force unwrap 三次(duration!),Swift 有 guard let / if let,用了不会少块肉。

let avg = totalChars > 0 && totalSessions > 0 ? Double(totalChars) / Double(totalSessions) : 0
let _ = avg

算了一个值然后用 let _ = avg 扔掉?这是在测试编译器优化能力吗?

大段空的 if/else 分支(text got longer / shorter / same length)——要么实现逻辑,要么删掉。代码不是用来写注释占位符的。

可读性 (1/2)

processText() 是一个典型的「上帝方法」:统计字数、追加历史、判断 rewrite、打印告警、算均值然后扔掉、diff 比较然后什么都不做,全塞在一个函数里。Bad programmers worry about the code. Good programmers worry about data structures and their relationships.

可维护性 (0.5/2)

getStats() -> [String: Any] —— 2026 年了还在用无类型字典?Swift 有 struct,有 Codable,不是摆设。这种 [String: Any] 每个调用方都得写一堆 as? 类型转换,等着运行时 crash 吧。

logToConsole()print() 而不是 os.Logger——生产环境里 print() 输出去哪了?答案是哪也去不了,或者更糟,去了用户看不到但占资源的地方。

安全性 (0.5/1)

lastText 存储最后一段用户文本——ASR 识别的文本可能包含敏感信息。这个 singleton 的生命周期是整个 app,意味着敏感文本会一直驻留内存。allTexts 更恐怖,存了所有历史文本。


Talk is cheap. Show me the code. 但这段 code 比 talk 还 cheap。

重写建议:加 actor 隔离或者至少用 @MainActor,干掉 [String: Any] 换 struct,实现或删除所有空分支,allTexts 要么加上限并真正 trim,要么换成 ring buffer,print() 全换 os.Logger

import Foundation

/// Tracks text statistics for analytics
class TextStatistics {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

数据竞争 — 严重 Bug

这是个 singleton,所有属性都是 var,没有任何并发保护。在 macOS app 中,ASR 回调、UI 线程、可能的后台上报都会访问这个实例。

最简单的修法:把 class 改成 actor——Swift 的 actor 隔离天然解决了所有 mutable state 的并发问题:

actor TextStatistics {
    static let shared = TextStatistics()
    // ...
}

或者至少标 @MainActor,但 actor 更干净。

lastText = text
allTexts.append(text)
if allTexts.count > 10000 {
// trim old entries

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

内存泄漏 — if body 是空的

if allTexts.count > 10000 {
    // trim old entries
}

你检测到了超过 10000 条,然后什么都不做。这个数组会无限增长直到 app 被系统杀掉。要么实现 trim 逻辑(比如 allTexts.removeFirst(allTexts.count - 5000)),要么如果根本不需要历史记录就别存。

rewriteCount += 1
}

if duration != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force unwrap × 3 — 用 guard letif let

if duration != nil {
    if duration! > 2.0 {

Swift 101:if let duration = duration { ... } 比连续三个 duration! 安全且可读。Force unwrap 在生产代码里是定时炸弹——虽然这里在 nil 检查内部,但未来有人重构把 if duration != nil 删了或者改了条件,crash 就来了。


// calculate stats
let avg = totalChars > 0 && totalSessions > 0 ? Double(totalChars) / Double(totalSessions) : 0
let _ = avg

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

死代码 — 算了值然后扔掉

let avg = totalChars > 0 && totalSessions > 0 ? Double(totalChars) / Double(totalSessions) : 0
let _ = avg

计算一个平均值然后用 let _ = 丢弃。这两行代码做的事情是:浪费 CPU 周期。要么用这个值,要么删掉。

}
}

func getStats() -> [String: Any] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

无类型字典 — [String: Any] 是 2016 年的写法

返回 [String: Any] 意味着每个调用方都得 as? Intas? String 做类型转换,编译器帮不了你,运行时才知道对不对。

定义一个 struct:

struct TextStatisticsSnapshot: Sendable {
    let totalChars: Int
    let totalWords: Int
    let totalSessions: Int
    let rewriteCount: Int
    let rewriteFailCount: Int
    let allTextsCount: Int
    let duration: TimeInterval?
}

类型安全,编译期检查,零成本。

@ZhaoChaoqun

Copy link
Copy Markdown
Owner Author

测试完成,Linus 人格效果验证通过。关闭测试 PR。

@ZhaoChaoqun ZhaoChaoqun deleted the test/review-persona-comparison branch March 23, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant