Closed F-park closed 8 months ago
相对来说,refactor是对一个项目帮助最小的事情之一。更关键的是,refactor有的时候只是迎合了“个人喜好”。在对项目本身帮助不大的情况下,大量修改了代码,导致很多可能出现的问题。
我并不是说refactor是一个无用功,只是对于一个项目而言,如果你要进行这种规模的refactor,那你必须具备两样东西,标尺和验证。第一是什么样的refactor是好的,第二是我如何知道refactor并没有破坏已经存在的功能。而这个量级的refactor,光靠code review是一个压力很大的事情。
举个简单的例子,我不认为
if (!match) {
return 0;
}
return f(match);
是一个比
if (match) {
return f(match);
}
return 0;
更加优质的表达。提前退出并不总能提升可读性。
这也是为什么很多开源项目(包括CPython)是不接受单纯的cosmetic change的。如果你想要做这种类型的refactor,你需要有一个linter,然后去follow一个固定的rule,从而保证代码的一致性。而这个一致性是客观的,不是你的主观认知。
同样的,在这种大规模refactor的PR里,最担心的就是非等价变换,也就是一个看似等价的修改,其实破坏了代码本身的功能。而这种情况,单纯的code review是不足够的,需要自动化测试。现在biliscope是没有任何测试的……所以我不会在现在这个阶段去考虑这么多行的refactor,风险太大。
如果你真的想去所谓“polish”这个项目(我非常欢迎),你应该优先尝试的是如何增加一些CI/CD的东西。怎么利用github actions去做lint,怎么增加一些基本的测试。尤其是这种插件性质的软件,怎么去mock或者用什么框架才能更好地保障程序的稳定性,让你在refactor的时候有信心说我这个refactor是正确并且有意义的。
所以我的建议是,如果你确实想深度参与这个项目,然后觉得希望做一些有助于项目本身提升的工作,从lint和test开始或许是一个非常不错的选择。
那我先给 biliscope
写 test
吧。
lint
感觉不是很着急, 先用 pre-commit
检查一些 pr 的基础格式问题就好了。
常规改动
let
都替换成const
var
都替换成let
?.
和??
代替||
和&&
特别说明
note.js
中noteNode.getElementsByTagName
改为noteNode.querySelector
ui.js
中relationClass
重构为getRelationHTML
(关联改动)UserProfileCard.prototype.updateCursor
增加参数默认值,this.cursorX/this.cursorY
变为cursorX/cursorY
sitescript.js
中Array.slice(1)
和Array.forEach
代替 for 循环