V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
fyxtc
V2EX  ›  程序员

新公司,代码审查的时候 leader 修改了一些我个人觉得真的没必要的地方

  •  1
     
  •   fyxtc · 2018-06-29 10:56:44 +08:00 · 15194 次点击
    这是一个创建于 2121 天前的主题,其中的信息可能已经有所发展或是发生改变。

    for k, v in pairs(icons) do (一些调用) end

    for 里几行代码,然后把 v 改成了 item。。。。改成 icon 我也服气一点啊。。。(这里的 kv 真的是习惯了,如果是 python 的话我就会写 for icon in icons:)

    一个初始化图片 uri 的方法名:initSkinImage => initSkin.......

    随机显示一些按钮的文本方法名:randSelectLabel => randomOptions

    一些变量名: GROUP_INDEX = "group_index" => GROUP_INDEX = "gi" USER_STATUS_DATA = ” user_status_data" => USER_STATUS_DATA = "u_sd"

    一个调用我是拆开的 a.do1() a.do2() => a.do1().do2()

    更新用户状态方法名 updateUserRes => updateUserResponse

    最不能忍的是这个。 image.setVisible(true) => image.show()

    show 的内部实现就是调用 setVisible,两个方法都是框架自带的 照这种改法,现有代码里的所有 setVisible 调用处都该改了

    其他有些地方是命名是修改得好的,比如 title => titles, enterStartView => enterReadyView

    但是像上面那些的真的有点不太能接受。。。唉

    第 1 条附言  ·  2018-06-29 12:00:45 +08:00
    补充一句,好多人说风格问题,可是我真的除了那个 res 的缩写之外(这个是我的错,就算缩写也应该是 resp ),其他的修改完全看不到有任何属于能被风格所定义的修改范畴啊! 又不是什么驼峰大括号 abc 这样的。。。希望你们能先好好看看修改的部分可以吗。。。
    第 2 条附言  ·  2018-06-29 14:32:29 +08:00
    谢谢大家的回复,可能是一周内我这个小模块需求就添加了三次(注意是添加),本身自己就算是挺用心的在重构了,所以这些小问题的修改确实影响到了一点自己的情绪。大家说的话我也都看了,我也会选择适合自己的解决方法,谢谢大家
    第 3 条附言  ·  2018-06-29 18:39:29 +08:00
    最后一条附言
    首先反驳一下那些说 show 比 setVisible 好的,考虑一下要通过某个数据状态来更新显示显示状态的情况
    setVisible(isFin)还是 if isFin then show() else hide() end
    就知道为什么我习惯写 setVisble 了。没有说 show 不好,只不过 setVisble 也没有不好,所有改动没必要

    最后:leader 很好,很 nice,对事不对人,我个人观点依然觉得那几处(除了 res )之外依旧是无关紧要,我也认为 review 是很好的,但是我更偏向看到一些代码性的修改:比如修改了某个算法,提高了性能,或者修复了某些隐藏 bug 之类的。而不是在已有代码就存在我被修改的调用(刚搜了下 setVisible: 204 matches across 56 files)和一些冗余的使用 temp 而不是用 a,b = b, a 交换的情况下。这样显得修改建议并没有那么有说服力,反而有点画蛇添足?

    最后的最后,大家周末愉快~ 明天约妹子看电影了,紧脏。。。。
    131 条回复    2018-07-05 22:33:45 +08:00
    1  2  
    jorneyr
        101
    jorneyr  
       2018-06-29 19:16:53 +08:00
    随机显示一些按钮的文本方法名:randSelectLabel => randomOptions

    一个是单数,一个是复数,最后随机显示的是单数还是复数呢?如果不确定是单数还是复数大家就不知道那个好,但至少有一个是有问题的。
    huijiewei
        102
    huijiewei  
       2018-06-29 20:28:10 +08:00 via iPhone
    下周不去了

    咱不惯着这种 leader

    就这样子
    cnTangLang
        103
    cnTangLang  
       2018-06-29 21:22:09 +08:00
    对你来说可能没必要
    对他未必
    不要拿自己的所知和思维来衡量别人的所知和思维,这是职场基本规则
    yangqi
        104
    yangqi  
       2018-06-29 21:28:04 +08:00
    很正常,没有对错,但是 leader 要负责整个组的代码一致性,不然每个人都自己的风格,之后维护起来很麻烦的。所以单个看每个修改都可以说没必要,但是从全局来看就是有必要。
    BruceLi
        105
    BruceLi  
       2018-06-29 22:20:02 +08:00
    和 leader 深入讨论:1 )如果你对但是 leader 不接受,那么以后日子也不会太舒服,趁早想办法吧;如果 leader 接受了,对你有加分。2 )如果 leader 对,你学到了改就是了。总之多沟通,不要憋着,自己内心不爽,对大家都没好处。
    a7a2
        106
    a7a2  
       2018-06-29 22:24:21 +08:00
    无看完 ,第一个 for 的,你是对的。k,v 足够简洁干练一看就知道 key,value,绝对比 item 实际。
    coolcfan
        107
    coolcfan  
       2018-06-29 23:03:24 +08:00
    你得这么想,如果过很久之后换个人来读你的代码,他能不能读懂。

    我看惯了我司的代码风格,读别家代码的时候看到满屏的缩写就头疼。
    M003
        108
    M003  
       2018-06-30 01:06:33 +08:00   ❤️ 1
    有强制规定还是比较好的,毕竟大家都统一,接手也方便,

    你们试着接手 变量有 $aa,$a

    我找谁说理去?
    armysheng
        109
    armysheng  
       2018-06-30 01:36:01 +08:00
    leader 的水平确实比你高很多,素养问题啊。Option 就是 Option,不是什么 selectedLabel
    fuermosi777
        110
    fuermosi777  
       2018-06-30 02:04:49 +08:00
    你写代码要考虑的是后来人看你代码的感受。
    ToT
        111
    ToT  
       2018-06-30 02:59:46 +08:00
    改了以后,就照着这个命名方式来写。老板希望团队统一命名 和 习惯,没有必要钻这个牛角尖。
    POPOEVER
        112
    POPOEVER  
       2018-06-30 03:41:42 +08:00
    新公司没先给你看 Best Practice 或者开发规范么?
    gtanyin
        113
    gtanyin  
       2018-06-30 08:04:31 +08:00
    改就改呗,哪有这么多*事?还来 v 站发个帖。。。。。。。。
    lrh3321
        114
    lrh3321  
       2018-06-30 08:27:23 +08:00 via Android
    统一代码风格
    DavidNineRoc
        115
    DavidNineRoc  
       2018-06-30 08:39:29 +08:00
    说实话响应我习惯用 res 了
    HangoX
        116
    HangoX  
       2018-06-30 08:55:20 +08:00 via Android
    leader 是让你写团队风格的代码,对团队有很大的好处。
    zjsxwc
        117
    zjsxwc  
       2018-06-30 08:58:04 +08:00 via iPhone
    “最不能忍的是这个。image.setVisible(true) => image.show()

    show 的内部实现就是调用 setVisible,两个方法都是框架自带的 照这种改法,现有代码里的所有 setVisible 调用处都该改了”

    兄弟,你没碰到过以后如果 show 的时候要多加点业务的场景吗?比如不单单是图片直接展示,你还要家点特效这类的,直接 setvisible 就写死了以后不好改。
    wangxn
        118
    wangxn  
       2018-06-30 08:59:24 +08:00 via Android
    游戏行业有这种细致要求的老大真少见。
    还有,visible 是个形容词,setVisible 极为奇怪。
    hadixlin
        119
    hadixlin  
       2018-06-30 09:03:50 +08:00
    这种事儿是想法的的碰撞,领导在给你表达想法的机会,代码审查其实是程序员之间沟通想法的通道.

    光在这里发牢骚,不如去沟通.
    zcjfesky
        120
    zcjfesky  
       2018-06-30 09:52:32 +08:00 via Android
    现在当组长真是… 不审吧,组员可能会上 v2 吐槽你不干活;审得细吧,组员还是会上 v2 吐槽…

    编码规范很重要,你写的代码不是只给你自己看的。如果公司没有编码规范纯靠人工经验,那你得督促公司推进规范文件
    leekafai
        121
    leekafai  
       2018-06-30 09:56:21 +08:00 via Android
    强迫症,不行就分
    bookit
        122
    bookit  
       2018-06-30 10:03:26 +08:00
    和此人讨论一下,看看有没有代码规范,有就学习一下。

    谈不拢就走人呗
    someonedeng
        123
    someonedeng  
       2018-06-30 10:06:13 +08:00
    除了 response 那个,其他改动没什么必要。。
    jmk92
        124
    jmk92  
       2018-06-30 12:38:20 +08:00 via iPhone
    我也遇到过这样的 lender,但是影响更深的是也有一个你这样的同事,处女座是不是?
    Coioidea
        125
    Coioidea  
       2018-06-30 14:59:24 +08:00
    首先你们公司有 code conduct 吗?你或你的 leader 谁遵守了?
    然后公司其他的相关代码你都读过吗?有习惯用法或注释都了解吗?
    一个好的团队应该是有标准化的管理的,无规矩不成方圆
    wizardoz
        126
    wizardoz  
       2018-06-30 16:57:13 +08:00
    我想说我看过的 response 缩写一般是 rsp,res 一般理解为 resource
    scofieldpeng
        127
    scofieldpeng  
       2018-06-30 17:13:24 +08:00
    非常同意 5 楼的做法,代码规范这个东西,每个公司都有不同的规范,作为新人,大忌就是把之前公司和个人习惯带过来。举个例子,for 循环里面可能你在前一家公司用的是 k,v,现在的公司用的是 i,v,你说哪个好?其实都一样,但是如果你把你前面公司的习惯带过来,还是之前说的,10 个人的团队如果和你一样,连一个简单的 for 循环都有 11 种不同的写法,假设有 1 个函数你们 11 个人都维护,你会发现这个函数里面的代码简直就是无法直视,每个人都会说自己的是规范,比起这个,为什么不用 leader 强制的规范,是,可能你说的这些觉得他很不“规范”,但是记住,能让团队高效协作的就是好规范。

    其实规范这东西和人类的语言很相似,全世界不说,就中国就不知道多少种方言,如果没有普通话这个官方指定语言,你觉得你能和其他地方的人流畅沟通,别逗了?那你能说普通话就是最规范最牛逼的么?
    ninestep
        128
    ninestep  
       2018-06-30 23:34:34 +08:00
    你只是改了,你没遇到过你写了一天,看了三分钟给你全删了让你重写的呢,询问原因他的回复是你觉得能用?废话我可是都做过单元测试的,关键后来我用 git 恢复了他说写的不错~~~呵呵;
    这个东西就是因人而异,你可以询问一下原因,如果是因为团队一些约定俗成的规定或者一些规范性问题,那没问题,以后写的时候注意一下,但是如果是针对个人了,那推荐辞职走人吧。
    edoras
        129
    edoras  
       2018-07-01 16:21:50 +08:00 via Android
    有些命名我原先也觉得其实挺没必要,但是看了 clean code 后感觉有些变量 self explain 这样真的再好不过,毕竟后来维护的人不一定懂得。如果非要质疑建议你去商量在哪维护一个规范,这样别人写代码时也能遵守,不会出现一半人用这种 一半人用那种的情况。
    koalli
        130
    koalli  
       2018-07-04 09:57:41 +08:00
    setVisible 应该是 C++或者其他绑定来的接口吧,你直接调用的话提高了耦合,如果将来底层改动的话,就得全局搜索 setVisible 去修改,这里改成统一调用 show 接口我认为是解耦合的做法。同时如果你们的 lua 有自动化测试的话,你直接调用 setVisible 就完蛋了,但是如果你调用统一个 show 接口的话,只需要简单的提供一个测试用的接口就可以搞定了,不知道这么说你能不能 get 到我的点。

    变量名的缩写改动我看着很像是为了减少数据传输才做的。

    其实你如果有疑问可以直接问问你的 leader 为什么这么做,看得出来他的习惯很好,而且能有耐心一点点看你代码给你改动的 leader 真的不多。
    Noriko
        131
    Noriko  
       2018-07-05 22:33:45 +08:00
    initSkinImage => initSkin
    a.do1() a.do2() => a.do1().do2() (这个要看场景适不适合 fluent 模式)
    updateUserRes => updateUserResponse

    这 2 个或 3 个改后好点,其它的一般般,甚至还不如不改。


    说真的,拿任何一个人的代码给别人看,哪怕他写得再好,总有人觉得不符合自己风格,认为按自己改才更好。这世界上没有什么喜好是大家都完全一致认同的。

    你的 leader 负责是负责,但就行为本身没有任何意义。

    team 代码规约新人进来后讲下,大家保证重要原则必须遵守,小细节也能包容个人习惯。

    如果你 leader 改的是你的代码设计,让你代码效率更高,重构后结构更清晰,这种改动必须要感谢;但如果只是改些变量命名(当然前提是你的变量命名不要太差、太随意)、耍些小 tricky,我只会觉得大概率是因为这种 leader 控制欲太强。因为有些 leader 只要 review 不管代码好坏,他不改点代码就觉得浑身不舒服。。。
    1  2  
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   我们的愿景   ·   实用小工具   ·   1107 人在线   最高记录 6543   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 29ms · UTC 18:33 · PVG 02:33 · LAX 11:33 · JFK 14:33
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.