同事写的code很乱，需要指出来吗？

同级，互相review。发现同事写的code很不clean。pr小还好，直接指出特定地方就行了。pr大的时候会发现他整个结构都很差/不合理，很难指出具体哪里差。例如codebase里面已经有些好的功能，他加一个类似的，其实只要按已有的结构改改就好了，他非要自己重写了一遍，细节例如命名也不好，总体上完全不搭但是能跑，逻辑上也没有bug。有必要在code review中指出来他写得不好吗？怎么说？直接说 why dont you xxxxx感觉很粗鲁，理论上人家也没有赞成你的必要。但就这样通过了让他merge进去真的很难看，不指出来我点了approve的话被别人发现这么乱都能过，会不会对我信誉有损失？

我觉得需要指出来，否则这种PR被别人看到多了会觉得你们两个都是菜b……

我也有类似困惑. 我基本是:
- 如果是小事情, 写个 "nit: ...". 有时候可以接收的也会说 "xx 方法会更好, 但是如果不想改也可以"
- "你这个可以运行, 但是 xx 更好. 如果不想在这里改, 可以写个 todo 以后修". (虽然可能未必会来修, 至少以后自己/别人看到能知道. 而且如果正好要改这部分代码, 可能下一个人就把这个 TODO 给改了)
- 最好能指出具体的客观原因, 带有明确的出处, 而不是 "我觉得 xx 更好". 我觉得最有用的就是公司的 style guide, 或者使用的相关 framework 的 doc.
. 1point3acres.com - 非常偶尔的也会 "这个可以工作, 我 approve, 你写个 TODO, 我可以帮你去改掉 xx".
- 我还正在学习的 - "情况允许的时候, 接受别人做的事情不是自己觉得最正确的方式" (主要是针对 junior 的 code review 的态度)

整体还很看同事, 以及你们的关系 - 有的人就很 defensive, 有的就非常不 care code quality, 有的很在意你重写 TA 的 code. 提几次后看看人家反应.

Could you/ may be it’s better to xxx

我觉得你说的这两个点都很make sense啊。。就直接指出来比如哪个function不需要重新写是多余的，变量名你觉得哪个不太好 不用把所有的都列出来举几个例子ta明白就行。。 ta改好了再重新commit你approve就行了 因为是细节问题不影响performance 也不用所有的都改的尽善尽美。。

实话实说，同事无论如何都不愿意改就是他自己的事了

左青龙右白虎胸前纹个米老鼠 发表于 2021-12-3 15:47
睁一只眼闭一只眼能运行就过了，都是给资本家打工，又不是自己开公司，老中最容易犯的错误就是拿公司不当外 ...

当然需要指出来啦。。
不然以后别人很难读，也很难维护啊。。

说句扎心的话，楼主

如果我是你的同事的话，在了解你的水平不错的情况下，看到你approve这样的代码，会觉得你很easy，很软

实话实说，feedback is a present