同事代码写的很糟糕,该怎么给建议

今天看了同事写的一个周签到的代码,看了之后,心态爆炸了。该怎么给同事建议呢?

    /**
     * 签到列表
     */
    public function getSignStatus(array $params)
    {
        if (!is_array($params)) return $this->failed();
        $this->setRedis($params['gameCode']);
        $this->setHashKey($params);

        $week = [];
        $weekCount = [];
        $rewardConfigs = $this->_redis->get($this->_key) ? json_decode($this->_redis->get($this->_key), true) : [];
        foreach ($rewardConfigs as $item) {
            if ($item['sign_type'] == SignConfig::TYPE_WEEK) {
                $dayName = $item['day'] - 1;
                $ymd = date('ymd', strtotime("+$dayName day", strtotime(Carbon::now()->startOfWeek())));
                if ($this->_week_redis->exists($this->_sign_key)) {
                    $res = $this->_week_redis->getbit($this->_sign_key, $ymd);
                    if ($res == 1) {
                        $sts = SignConfig::SIGN_STATUS_ACQUIRE;
                    } else {
                        if (date('ymd') > $ymd) {
                            $sts = SignConfig::SIGN_STATUS_OVERDUE;
                        } else if (date('ymd') == $ymd) {
                            $sts = SignConfig::SIGN_STATUS_RECEIVE;
                        } else {
                            $sts = SignConfig::SIGN_STATUS_UNCLAIMED;
                        }
                    }
                } else {
                    $sts = (date('ymd') == $ymd) ? SignConfig::SIGN_STATUS_RECEIVE : SignConfig::SIGN_STATUS_UNCLAIMED;
                }
                $week[] = [
                    'day' => $item['day'],
                    'status' => $sts,
                ];
            } else if ($item['sign_type'] == SignConfig::TYPE_WEEKCOUT) {
                if ($this->_week_redis->exists($this->_week_sign_key)) {
                    $sts = $this->_week_redis->getbit($this->_week_sign_key, $item['day']);
                } else {
                    $sts = SignConfig::SIGN_STATUS_UNCLAIMED;
                }
                $weekCount[] = [
                    'day' => $item['day'],
                    'status' => $sts,
                ];
            }
        }

        $returnData = [];
        if ($this->_week_redis->exists($this->_week_sign_key)) {
            $returnData['week_count_days'] = $this->_week_redis->bitcount($this->_week_sign_key);
        } else {
            $returnData['week_count_days'] = 0;
        }
        if ($this->_week_redis->exists($this->_sign_key)) {
            $returnData['week_days'] = ($this->_week_redis->bitcount($this->_sign_key) != 7) ? $this->_week_redis->bitcount($this->_sign_key) : 0;
        } else {
            $returnData['week_days'] = 0;
        }
        $returnData['week'] = $week;
        $returnData['weekCount'] = $weekCount;
        $this->checkSigned();
        return $this->success($returnData);
}

仅对这个方法说下我的看法:

  1. 很多无用的代码
//这段代码没什么用的吧,参数不是已经限制是数组了吗?
if (!is_array($params)) return $this->failed();

//这行代码是不是应该被开除
$rewardConfigs = $this->_redis->get($this->_key) ? json_decode($this->_redis->get($this->_key), true) : [];

//这个if判断是不是没用
if ($this->_week_redis->exists($this->_week_sign_key)) {
    $returnData['week_count_days'] = $this->_week_redis->bitcount($this->_week_sign_key);
} else {
    $returnData['week_count_days'] = 0;
}

//写成这样就可以了
$weekCount = $this->_week_redis->bitcount($this->_week_sign_key);
  1. 嵌套是不是太深了? 就不展开说了。我觉得一个好的设计,逻辑是比较简单,代码让人看了很舒服的。

这是一个新项目的,还没上线的,之前同事开发的,因为疫情原因,领导让我暂时接手。

如果你不能把一件事很简单的讲清楚,那么你就是还不够了解。
《L02 从零构建论坛系统》
以构建论坛项目 LaraBBS 为线索,展开对 Laravel 框架的全面学习。应用程序架构思路贴近 Laravel 框架的设计哲学。
《L03 构架 API 服务器》
你将学到如 RESTFul 设计风格、PostMan 的使用、OAuth 流程,JWT 概念及使用 和 API 开发相关的进阶知识。
最佳答案

其实我个人非常认同 @Remember 提到的沟通技巧,跟非技术类同事沟通(比如客服产品什么的)我会倾向于使用这些技巧。不过跟技术同事沟通代码相关的问题,我现在都是正面指出错误并展开激烈讨论,注意是讨论,最终得出结论就 OK 了。客套和委婉反而容易让问题更加隐蔽,这些隐患叠加起来可能造成更严重的后果。

当然这样做的前提是,你得提前澄清自己:

  1. 只是针对某个问题展开讨论
  2. 并没有针对某人有任何情绪或者意见

要做到以上两点其实挺不容易,需要跟同事有一定默契,如果双方(尤其是自己)没有心理准备,这种日常的讨论很有可能演变为争论,甚至是谩骂。

我现在跟我经常合作的那位同事磨合的差不多了(自认为),之前对方还会有担心我这么激烈的讨论结束后会不会有情绪或者怎样,现在有啥问题我们俩就直接单刀直入,讨论完无论采纳了谁的主张,双方都能了解到对方的思路,从不同的角度看待这个问题。最后还是该开玩笑开玩笑,平时同事关系也没有受到什么影响。这种「平衡」非常难能可贵,只有慢慢磨合才能得来了。

至于楼主贴的那段代码其实我感觉写的还凑合,算不上非常烂😂。如果能加上注释让逻辑更易读或是拆分成多个方法来处理就更清晰了。

6个月前 评论
地平线 (楼主) 6个月前
Wi1dcard (作者) 6个月前
Wi1dcard (作者) 6个月前
Summer 6个月前
讨论数量: 30

现在的我

第一条原则:永远不与他人争论。

因为毫无意义。争论只会让事情更糟糕。

第二条:不要直白指出对方的错误

人在被当面指出错误的情况下会因为自尊心而坚决维护自己,即使知道是自己的错误,也会极力维护。效果和上面等同

第三条:肯定对方的贡献 在肯定对方的贡献的同时,以建议的口气(最好说明之后的造成的影响,比如说以后这里的需求改动了的话,那么),也许对方就能主动承认某方面的不足。

是我 的话,我接手了这段代码,我可能会这样,看不懂或者奇怪的地方,请教一下这里的流程逻辑,顺带可以的话夸一下对方的思维缜密(对你没有任何坏处),多用你觉得呢。比如,这里我们可以封装一下,你觉得呢 ...,这段代码看起来不是很好维护,当初是不是着急上线,代码可以xxx调整一下,你觉得呢...........,当然以上纯属自己的做法

6个月前 评论
goudan 6个月前
Tangqy 6个月前
Summer

一般我会视情况而定。

如果我需要维护同事代码,或者以后可能会跟他写同一块代码,也就是说他的不好的东西会影响到我,那我就会像 @Wi1dcard 讲的一样。先铺垫下(讲下对事不对人,情绪抛一边),然后说出我的看法。

注意这个铺垫非常重要,是必须要有的,否则在对方看来就是对他的不尊重。以高高在上的姿态在批评他,这种情况出现在父母长辈训斥里,很多人都会接受不了,更别说你了。对话也要保持理智,不要人身攻击,否则闹得不愉快那就本末倒置了。要记住目的在于共同精进、提高效率、节省时间。

当然,如果同事的工作跟我没太大关系,这种接受他项目的事情也是一次性的,那我会走 @Remember 的路。主要因为时间宝贵,没有义务去教别人,并且不论你做得多好人家不一定喜欢你教。还有就是有时候你也不一定是对的,在特殊的场景下,你也许也写过这种烂代码... :joy:

6个月前 评论
Wi1dcard 6个月前

建议你们老大更多的使用模块化或微服务, 一人负责一块, 各做各的, 互不干扰(滑稽保命)

6个月前 评论

现在的我

第一条原则:永远不与他人争论。

因为毫无意义。争论只会让事情更糟糕。

第二条:不要直白指出对方的错误

人在被当面指出错误的情况下会因为自尊心而坚决维护自己,即使知道是自己的错误,也会极力维护。效果和上面等同

第三条:肯定对方的贡献 在肯定对方的贡献的同时,以建议的口气(最好说明之后的造成的影响,比如说以后这里的需求改动了的话,那么),也许对方就能主动承认某方面的不足。

是我 的话,我接手了这段代码,我可能会这样,看不懂或者奇怪的地方,请教一下这里的流程逻辑,顺带可以的话夸一下对方的思维缜密(对你没有任何坏处),多用你觉得呢。比如,这里我们可以封装一下,你觉得呢 ...,这段代码看起来不是很好维护,当初是不是着急上线,代码可以xxx调整一下,你觉得呢...........,当然以上纯属自己的做法

6个月前 评论
goudan 6个月前
Tangqy 6个月前
lyxxxh

file

file

我是这样的 :see_no_evil:

他这个功能不会复用吧,如果不复用,写在一个方法也无所谓了。

同事: 可以一个方法写完 你为什么要分多个?

我: 。。。。

6个月前 评论
lyxxxh (作者) 6个月前
xiaoguo0426 6个月前
╰ゝSakura 6个月前
xiaoguo0426 6个月前
Wi1dcard 6个月前

:flushed: 想知道你理想中的代码是怎样的,具体的业务逻辑代码一层一层判断不正常吗?

6个月前 评论

引用论坛的一句话:

与人为善,比聪明更重要!

如果这段代码需要你来维护或者和你的业务有关系, 首先应该放平心态,然后了解业务需求,向对方多请教当时的情况。完全了解业务后,在着手重构即可。

如果你只是看着难受,我觉得可以先不沟通比较好吧。

6个月前 评论
lyxxxh

有点讨厌一个方法做完所有事情的

平时我喜欢用phpunit来调试里面的方法, 如果都写在一个方法 会给我增加很大的麻烦

6个月前 评论
青风百里

可直说,干这一行或许不需要套话官话。

6个月前 评论
╰ゝSakura

:relieved:问清楚逻辑后,自己重写吧

6个月前 评论

其实我个人非常认同 @Remember 提到的沟通技巧,跟非技术类同事沟通(比如客服产品什么的)我会倾向于使用这些技巧。不过跟技术同事沟通代码相关的问题,我现在都是正面指出错误并展开激烈讨论,注意是讨论,最终得出结论就 OK 了。客套和委婉反而容易让问题更加隐蔽,这些隐患叠加起来可能造成更严重的后果。

当然这样做的前提是,你得提前澄清自己:

  1. 只是针对某个问题展开讨论
  2. 并没有针对某人有任何情绪或者意见

要做到以上两点其实挺不容易,需要跟同事有一定默契,如果双方(尤其是自己)没有心理准备,这种日常的讨论很有可能演变为争论,甚至是谩骂。

我现在跟我经常合作的那位同事磨合的差不多了(自认为),之前对方还会有担心我这么激烈的讨论结束后会不会有情绪或者怎样,现在有啥问题我们俩就直接单刀直入,讨论完无论采纳了谁的主张,双方都能了解到对方的思路,从不同的角度看待这个问题。最后还是该开玩笑开玩笑,平时同事关系也没有受到什么影响。这种「平衡」非常难能可贵,只有慢慢磨合才能得来了。

至于楼主贴的那段代码其实我感觉写的还凑合,算不上非常烂😂。如果能加上注释让逻辑更易读或是拆分成多个方法来处理就更清晰了。

6个月前 评论
地平线 (楼主) 6个月前
Wi1dcard (作者) 6个月前
Wi1dcard (作者) 6个月前
Summer 6个月前

建议带把菜刀到他工位上

6个月前 评论
勺颠颠

操尝造花园一所;造成,操往观之,不置褒贬,只取笔于门上书一“活”字而去。人皆不晓其意。修曰:“门内添活字,乃阔字也。丞相嫌园门阔耳。”于是再筑墙围,改造停当,又请操观之。操大喜,问曰:“谁知吾意?”左右曰:“杨修也。”操虽称美,心甚忌之 :joy:

6个月前 评论
小李世界

嗯。贴的代码应该很多公司都这样。要做到一个方法一个功能,那算是跟 Laravel 框架一样完美了。当然我同意你的观点,事情早做早好,不然未来还是要分的。

我向来直话直说,对事不对人。我觉得对你有好处的时候,会跟你反馈或提醒,我不会闭着不说。

爱听不爱听随人,中国人向来比较多是报喜不报忧,喜欢吉利话,少做一件事就少犯错误,对不起,我不是那种人。

别人批评我、建议我,反正别人说话,对的时候,我至少都会回:“嗯”、“是”,这样别人说的话才有意义。

别人说的观点和自己的三观不一样,那就你说你的,我说我的。

别人说的话明显不对,拿出证据,不要怂,就是干!

6个月前 评论

与人为善,比聪明更重要! 首先应该放平心态向对方多请教当时的情况。完全了解业务后,在着手重构即可。

6个月前 评论
Summer

一般我会视情况而定。

如果我需要维护同事代码,或者以后可能会跟他写同一块代码,也就是说他的不好的东西会影响到我,那我就会像 @Wi1dcard 讲的一样。先铺垫下(讲下对事不对人,情绪抛一边),然后说出我的看法。

注意这个铺垫非常重要,是必须要有的,否则在对方看来就是对他的不尊重。以高高在上的姿态在批评他,这种情况出现在父母长辈训斥里,很多人都会接受不了,更别说你了。对话也要保持理智,不要人身攻击,否则闹得不愉快那就本末倒置了。要记住目的在于共同精进、提高效率、节省时间。

当然,如果同事的工作跟我没太大关系,这种接受他项目的事情也是一次性的,那我会走 @Remember 的路。主要因为时间宝贵,没有义务去教别人,并且不论你做得多好人家不一定喜欢你教。还有就是有时候你也不一定是对的,在特殊的场景下,你也许也写过这种烂代码... :joy:

6个月前 评论
Wi1dcard 6个月前
JeffLi

@Remember 你的原则很对。。。。一般人很难做到别人直面指出错误 ,自己能理性接受的

6个月前 评论
zhm1992

不要说出来,不要试图重构,你写烦了可能也这样

6个月前 评论

这就炸了,,,说明你心态不行,看看这个:

file

6个月前 评论

见过这样的没 😊

//将手机号 中间的四位换成 *
foreach ($remarks as $k => $v) {
    $phone[] = $v['phone'];
}
foreach ($phone as $k => $v) {
    $a[$k][0] = $v[0];
    $a[$k][1] = $v[1];
    $a[$k][2] = $v[2];
    $a[$k][3] = '*';
    $a[$k][4] = '*';
    $a[$k][5] = '*';
    $a[$k][6] = '*';
    $a[$k][7] = $v[7];
    $a[$k][8] = $v[8];
    $a[$k][9] = $v[9];
    $a[$k][10] = $v[10];
}
foreach ($a as $k => $v) {
    $remarks[$k]['phone'] = $v[0].$v[1].$v[2].$v[3].$v[4].$v[5].$v[6].$v[7].$v[8].$v[9].$v[10];
}
6个月前 评论
cbasil 6个月前
Remember 6个月前
xiaoguo0426 6个月前

我会说,你这代码是狗啃出来的吧,看老子给你这狗东西优化一下,睁大你的狗眼学习学习!

6个月前 评论
aodaobi 6个月前

说实话,代码逻辑算是不错了,好歹一目了然,虽然多了一些可有可无的代码。而且他很好的完成了工作,你还想要求怎么样? 看不惯,你自己重写就行。如果你是他的LEADER,你可以说他。如果不是,那还是算了吧

6个月前 评论
Complicated

这代码还可以的,你是没见过更恶心的,我现在维护的这点份代码,,改个问题,都无从下手,简直了

6个月前 评论

重构他的代码,讲解你重构的依据。

6个月前 评论
phperShine

我公司同时也是 真的挺烦人的 和他说 他就说能跑就行了 :cry:

6个月前 评论
Complicated

@phperShine 对,最烦这种能run就行的,不考虑后期维护,扩展,还有其他维护人员的感受,不能有点职业素养

6个月前 评论

额外提个话题,遇到像 @phperShine 说的这种人时,该如何处理 遇到这种时,真不知怎说...

6个月前 评论

不错了,你是没见过,更扯淡的 :confused:

6个月前 评论

我觉得不要太过直白,不是黑程序员,很多程序员都是迷之自信的,很多时候很容易变成争吵,还是得慢慢来,要换位思考.毕竟都是同事,如果你想着走人了,也不要怼,因为你永远不知道面试下一个公司时会不会打电话给你前同事问你的情况.

6个月前 评论

文人相轻 何况程序员 耐心沟通

6个月前 评论

文人相轻

6个月前 评论

请勿发布不友善或者负能量的内容。与人为善,比聪明更重要!