Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

重构 go-fsrs #3

Merged
merged 7 commits into from
Nov 30, 2022
Merged

Conversation

L-M-Sherlock
Copy link
Member

目前是仿照 Anki 的调度器重构了 go-fsrs,文档稍后会先补充到 wiki

@L-M-Sherlock
Copy link
Member Author

@L-M-Sherlock L-M-Sherlock added the enhancement New feature or request label Nov 26, 2022
models.go Show resolved Hide resolved
Fix/Short-term schedule
Feat/Custom Parameters
models.go Show resolved Hide resolved
@L-M-Sherlock
Copy link
Member Author

@ImSingee 方便来 review 一下代码吗?

@ImSingee
Copy link
Member

@L-M-Sherlock 好的,我今天看下

@L-M-Sherlock
Copy link
Member Author

既然 ReviewLog 已经有 CardID 了,我在想是不是 ReviewLog 没必要嵌入 Card?

@88250
Copy link

88250 commented Nov 29, 2022

既然 ReviewLog 已经有 CardID 了,我在想是不是 ReviewLog 没必要嵌入 Card?

对于应用层来说不嵌入的话更好,这样分开两张表单独持久化会更容易一些。但是不嵌入的话 ReviewLog 的管理就得提供接口或者至少提供一个列表变量来维护。

@88250
Copy link

88250 commented Nov 29, 2022

或者可以考虑在 Repeat() 函数的返回值上直接返回 4 种 ReviewLog(大概这样 map[Rating]Scheduling(Card, ReviewLog),Scheduling 是新增的结构,里面两个字段 Card 和 ReviewLog),这样将后续 ReviewLog 维护完全交给应用。

Copy link

@88250 88250 left a comment

Choose a reason for hiding this comment

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

我这里基本没有疑问了,由衷感谢叶大。

Review time.Time
Reps uint64
Lapses uint64
func NewCard() Card {
Copy link

Choose a reason for hiding this comment

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

这里可以返回指针类型 *Card 吗?

Copy link
Member

Choose a reason for hiding this comment

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

我感觉这里返回 Card 挺好的,整个库的设计倾向于一个 immutable,即一个 Card 创建后就不再可修改,如果这里返回 *Card 可能会在外部更多的倾向于嵌入/使用 *Card ,这在一定程度上属于违反了设计理念

如果是担心性能问题可以忽略,一方面 Go 本身对于这种情况的优化已经相当不错了绝大多数下会避免拷贝(绝大多数情况下 NewCard 的调用都会被内联优化掉),另一方面就算拷贝整体的数据量也不大

Copy link

Choose a reason for hiding this comment

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

@ImSingee Repeat 函数那里入参是指针哦,我还是倾向于 NewCard 也返回指针,这样可能统一一点。

Copy link
Member Author

Choose a reason for hiding this comment

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

但 Repeat 应该是不修改 Card 的,我可能要把入参改成 Card 类型。

Copy link

Choose a reason for hiding this comment

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

@L-M-Sherlock 这样的话和 SchedulingCards 里的字段 Card (非指针)就统一了,如果我没理解错的话这就是之前 @ImSingee 提到的 go-fsrs 不变式设计理念,这样的话我这里对 NewCard 返回 Card 没有疑问了,谢谢。

models.go Show resolved Hide resolved
models.go Show resolved Hide resolved
@ImSingee
Copy link
Member

我还是觉得 Card 和 ReviewLog 不应该加 Id

  1. go-fsrs 更多的是一个算法库,ID 更多的是应用层面的内容
  2. 虽然 id 使用了时间戳,但是实质上并不能保证唯一性(特别是分布式场景下),并且 id 这个名称可能会给外部以错误的暗示
  3. Card 的使用更多的应当是被嵌入其他结构体中使用,外部通常已经会嵌入一些元信息(例如卡片内容、创建时间等),再加上一个 ID 对外部的成本并不高
  4. 即使考虑到扁平存储,通常使用外部 ID 也是更好的实践,例如数据库中的主键等

我认为 go-fsrs 的设计应当天然考虑其是算法库的属性,而尽可能的避免掉和应用层打交道,其管理卡片的复习参数、且只管理卡片的复习参数

@88250
Copy link

88250 commented Nov 29, 2022

没有 ID 的话确实更符合算法库的本意,目前 int64 的类型和生成方式也不可以自定义 ID 生成器,确实也有些局限性。我这里还是考虑外部嵌入或者自行关联吧,不好意思之前可能误导 @L-M-Sherlock 了,抱歉抱歉。

@L-M-Sherlock
Copy link
Member Author

L-M-Sherlock commented Nov 29, 2022

嗯,时间戳跟系统时间有关,存在碰撞的可能性。不过我已经回家了,电脑不在身边,要改也是明天改了。

Copy link

@88250 88250 left a comment

Choose a reason for hiding this comment

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

谢谢,我这里没有疑问了,期待合并。

@L-M-Sherlock L-M-Sherlock merged commit 2e5d411 into open-spaced-repetition:main Nov 30, 2022
@L-M-Sherlock
Copy link
Member Author

我这边先合并了,后续补充测试和文档。

@L-M-Sherlock L-M-Sherlock deleted the refactor branch November 30, 2022 01:43
@L-M-Sherlock L-M-Sherlock linked an issue Nov 30, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

重构 go-fsrs
3 participants