什么是CodeReview及CodeReview的目的

  1. CodeReview 不是针对某个人的批评会，也不是翻旧账的行为。CodeReview 是为了规范团队编码规范，提高整体代码质量，培养良好的编码习惯而采取的一种手段。
  2. 主要目的是推动团队成员遵守被按规范执行编码规范，发现代码工程中的异味代码和不遵守规范的代码，强调规范，提升个人意思。
  3. Coding 可以看成是一项工程，是多个人通力合作的产出。任何小的细节遗漏或没做好都可能导致线上故障。所有，要对每一行代码负责。
  4. 代码的可读性和易维护性是对每一个工程师的基本要求。写出来的代码是为了后续更容易的维护和功能的更迭，不仅仅只是能运行。
  5. Laravel 编码规范见文档

Talk is cheap show me the code.

Middleware codereview

文件 app/Exceptions/Handler.php

  1. <?php
  3. /**
  4.  * Report or log an exception.
  5.  *
  6.  * @param  \Exception  $exception
  7.  * @return void
  8.  */
  9. public function report(Exception $exception)
  10. {        // 非配置文件代码中，不要直接使用 env 函数获取 .env 配置
  11.     if (env('APP_ENV') !== 'local') {
  12.       // 此处的 if 可以提升，合并到上一级，可读性更好
  13.         if (app()->bound('sentry') && $this->shouldReport($exception)) {
  14.             app('sentry')->captureException($exception);
  15.         }
  16.     }
  17.     parent::report($exception);
  18. }

根据智众 Laravel 规范 以及 SRP 原则重构后如下。

  1. <?php
  3. /**
  4.  * Report or log an exception.
  5.  *
  6.  * @param  \Exception  $exception
  7.  * @return void
  8.  */
  9. public function report(Exception $exception)
  10. {        
  11.     if ($this->shouldReportToSentry($exception)) {
  12.         app('sentry')->captureException($exception);
  13.     }
  14.     parent::report($exception);
  15. }
  17. // 判断是否应该的函数最好以  is should can could 等开头
  18. // SRP 原则，最小责任。
  19. // 将判断是否应该被sentry捕获的异常单独提出来作为一个函数
  20. public function shouldReportToSentry(Exception $exception): bool
  21. {
  22.       !\in_array(config('app.env'), ['production', 'staging', 'prod']) &&
  23.       app()->bound('sentry') &&
  24.       $this->shouldReport($exception);
  25. }

文件 app/Http/Middleware/CheckSimpleSign.php

  1. <?php
  3. namespace App\Http\Middleware;
  5. use App\Models\ZOauthClients;
  6. use App\Traits\ResponseFormat;
  7. use App\Utils\Sign;
  8. use Carbon\Carbon;
  9. use Closure;
  10. use Illuminate\Support\Facades\Cache;
  12. class CheckSimpleSign
  13. {
  14.     use ResponseFormat;
  15.     protected $response;
  16.     const MAX_TIMESTAMP_SKEW = 60;
  17.     const CACHE_KEY="mmc:client_id:";
  18.     /**
  19.      * Handle an incoming request.
  20.      *
  21.      * @param  \Illuminate\Http\Request  $request
  22.      * @param  \Closure  $next
  23.      * @return mixed
  24.      */
  25.     public function handle($request, Closure $next)
  26.     {        
  27.         // try 的异常是什么异常？
  28.         try{
  29.             // 变量命名规范 参考 PSR2
  30.             // 小写字母开头的驼峰写法
  31.             $_client_id=$request->input('_client_id');
  32.             $_timestamp=$request->input('_timestamp');
  33.             $_sign=$request->input('_sign');
  34.             //兼容未传验参参数无需校验
  35.             if(!$_client_id && !$_timestamp && !$_sign){
  36.                 return $next($request);
  37.             }
  38.             if(!$_client_id || !$_timestamp || !$_sign){
  39.                 return $this->jsonError(421, '缺少参数',0);
  40.             }
  41.               // 无意义
  42.             if(strtotime(date("Ymdhis",$_timestamp))!=$_timestamp){
  43.                 return $this->jsonError(421, '参数异常',0);
  44.             }
  45.             $version=$this->_client_version($_client_id);
  46.             if(!$version){
  47.                 return $this->jsonError(421, '参数异常.',0);
  48.             }
  49.             $signParams=[
  50.                 "_client_id"=>$_client_id,
  51.                 "_timestamp"=>$_timestamp,
  52.             ];
  54.             $sign=Sign::getSign($signParams,$version);
  55.             if($sign!=$_sign){
  56.                 return $this->jsonError(421, '签名异常.',0);
  57.             }
  58.             return $next($request);
  59.         }catch (\Exception $e){
  60.             return $this->jsonError(421, $e->getMessage(),0);
  61.         }
  62.     }
  64.       // 函数名命名
  65.     private function _client_version($client_id){
  66.         $cache_key=self::CACHE_KEY.$client_id;
  67.         $version=Cache::get($cache_key);
  68.         if($version){
  69.             return $version;
  70.         }
  71.         $client_info=ZOauthClients::getInfoByClientId($client_id);
  72.         if(empty($client_info)){
  73.             return "";
  74.         }
  75.         Cache::add($cache_key,$client_info['version'],self::MAX_TIMESTAMP_SKEW);
  76.         return $client_info['version'];
  77.     }
  78. }

Controller codereview

文件 app/Http/Controllers/Api/MMC/BloodSugarController.php

  1. <?php
  2. //获取医院中心编号 部门暂不加入
  3. private function _getHospCd($hospId) {
  4.     $sql = "select * from z_mmc_no_range  where hosp_id =  '$hospId' and mmc_min != ''  limit 1";
  6.     $ret = DBSelectOne($sql);
  8.     if (isset($ret['mmc_min']) && $ret['mmc_min']) {
  9.         return $ret['mmc_min'];
  10.         }
  12.     return false;
  13. }

操作DB，不要直接拼接 SQL 。要使用Laravel中ORM提供的写法。

大部分 Controller 没有分层。业务逻辑代码，大段堆积在 Controller 中。 Controller 职责过重。至少拆分一层 Service 层。Service 负责处理具体业务逻辑。Controller 仅仅负责分发客户端请求到Request验证参数，然后将具体处理业务逻辑分发到Service，获取并放回 Service 处理结果。对DB的操作建议写在Model中，如有必要，也可以写在Service中。

app/Http/Controllers/Api/Auth/WinAppAuthController.php

app/Http/Controllers/Api/Config/SysConfigController.php

app/Http/Controllers/Api/EyeGround/AuthController.php

app/Http/Controllers/Api/EyeGround/MmcEyesGroundController.php

app/Http/Controllers/Api/EyeGround/PatientController.php

app/Http/Controllers/Api/EyeGround/SystemNoticeController.php

app/Http/Controllers/Api/EyeGround/ViewEyesController.php

app/Http/Controllers/Api/EyeGround/VistelController.php

app/Http/Controllers/Api/MMC/BgController.php

app/Http/Controllers/Api/MMC/BloodSugarController.php

Utils 及 Service codereview

app/Utils/CommonUnits.php 工具类功能也要内聚。既然是 OOP 需要有 OO 的概念。需要具体拆分。

  1. <?php
  3. namespace App\Utils;
  4. use Illuminate\Support\Carbon;
  6. class CommonUnits
  7. {
  8.     /**
  9.      * 根据生日获取年龄
  10.      * @param string $birth 生日
  11.      * @param string $nowDate 当前日期
  12.      * @param int $type 1周岁 2虚岁
  13.      * @return int|null
  14.      */
  15.     public static function getAgeByBirth($birth,$nowDate="",$type=1){
  16.         if(!$nowDate) $nowDate=Carbon::now()->format("Y-m-d");
  17.         $birthTime = strtotime($birth);
  18.         $nowTime = strtotime($nowDate);
  19.         if($birthTime === false || $nowTime===null || $birthTime<0 || $nowTime<0){
  20.             return null;
  21.         }
  22.         if($birthTime>$nowTime){
  23.             return null;
  24.         }
  25.         list($birthYear, $birthMonth, $birthDay) = explode('-', $birth);
  26.         list($currentYear, $currentMonth, $currentDay) = explode('-', $nowDate);
  27.         if($type==1) {//周岁
  28.             $age = $currentYear - $birthYear - 1;
  29.             if ($currentMonth > $birthMonth || $currentMonth == $birthMonth && $currentDay >= $birthDay){
  30.                 $age++;
  31.             }
  32.         }else if($type==2){//虚岁
  33.             $age = $currentYear - $birthYear;
  34.         }
  35.         return $age;
  36.     }
  38.     /**
  39.      * @param $date
  40.      * @param string $format
  41.      * @return null|string
  42.      */
  43.     public static function getDateByDateTime($date,$format="Y-m-d"){
  44.         $dateTime=strtotime($date);
  45.         if(!$dateTime || $dateTime<0){
  46.             return null;
  47.         }
  48.         return Carbon::createFromFormat($format,$date)->format($format);
  49.     }
  51.     /**
  52.      * 数组笛卡尔集组合
  53.      * @param $arr
  54.      * @return array|mixed
  55.      * @example CommonUnits::combineDika([[1,2], [3,4,5]]);
  56.      */
  57.     public static function combineDika($arr)
  58.     {
  59.         $result = array_shift($arr);
  60.         while ($arr2 = array_shift($arr)) {
  61.             $arr1 = $result;
  62.             $result = array();
  63.             foreach ($arr1 as $v) {
  64.                 foreach ($arr2 as $v2) {
  65.                     if (!is_array($v)) $v = array($v);
  66.                     if (!is_array($v2)) $v2 = array($v2);
  67.                     $result[] = array_merge_recursive($v, $v2);
  68.                 }
  69.             }
  70.         }
  71.         return $result;
  72.     }
  74.     /**
  75.      * 数据脱敏
  76.      * @param $string 需要脱敏值
  77.      * @param int $start 开始
  78.      * @param int $length 结束
  79.      * @param string $re 脱敏替代符号
  80.      * @return bool|string
  81.      * 例子:
  82.      * dataDesensitization('18811113683', 3, 4); //188****3683
  83.      * dataDesensitization('乐杨俊', 0, -1); //**俊
  84.      */
  85.     public static function dataDesensitization($string, $start = 0, $length = 0, $re = '*')
  86.     {
  87.         if (empty($string)){
  88.             return false;
  89.         }
  90.         $strarr = array();
  91.         $mb_strlen = mb_strlen($string);
  92.         while ($mb_strlen) {//循环把字符串变为数组
  93.             $strarr[] = mb_substr($string, 0, 1, 'utf8');
  94.             $string = mb_substr($string, 1, $mb_strlen, 'utf8');
  95.             $mb_strlen = mb_strlen($string);
  96.         }
  97.         $strlen = count($strarr);
  98.         $begin = $start >= 0 ? $start : ($strlen - abs($start));
  99.         $end = $last = $strlen - 1;
  100.         if ($length > 0) {
  101.             $end = $begin + $length - 1;
  102.         } elseif ($length < 0) {
  103.             $end -= abs($length);
  104.         }
  105.         for ($i = $begin; $i <= $end; $i++) {
  106.             $strarr[$i] = $re;
  107.         }
  108.         if($start>0) {
  109.             if ($begin >= $end || $begin >= $last || $end > $last) return false;
  110.         }else if($start===0){
  111.             if ($begin > $end || $begin >= $last || $end > $last) return false;
  112.         }
  113.         return implode('', $strarr);
  114.     }

该类包含，根据生日获取年龄、数组笛卡尔集组合、数据脱敏、以及根据日期字符串获取date对象。

需要适当的对功能进行区分，来小心的构造类对象。不要所有不相干的功能都堆叠在一个类中。

日期相干的功能使用 Carbon\Carbon 库应该可以满足 90% 以上的需求，其他的也可以用 Carbon 中的方法来具体实现。

app/Services/YanDiNotMmcCompleteService.php

存在大段函数（170行）来实现一个功能。这样的函数大都可以拆分成更具体的职责函数来实现。大段函数，如果在功能更迭时需要改动，代码阅读成本以及测试成本很高，且容易引入bug。

  1. <?php
  3. public function completion($yanDiInfo)
  4. {
  5.     Log::info("开始计算", $yanDiInfo);
  6.     $userId = $yanDiInfo['user_id'];
  7.     $hospId = $yanDiInfo['hosp_id'];
  8.     $deptId = $yanDiInfo['dept_id'];
  9.     $picture = json_decode($yanDiInfo['img_ids'], true);
  10.     $left_num = 0;
  11.     $right_num = 0;
  12.     if (isset($picture['left'])) {
  13.         $left_num = count($picture['left']);
  14.     }
  15.     if (isset($picture['right'])) {
  16.         $right_num = count($picture['right']);
  17.     }
  18.     $isMmc = 0;
  19.     $scores = [
  20.         'pictureScore' => 0, //眼底图片得分
  21.         'baseScore' => 0, //基本信息得分
  22.         'historyScore' => 0,//病史信息得分
  23.         'inspectScore' => 0,//实验室检查必填得分
  24.         'inspectNotRequiredScore' => 0, //实验室检查选填得分
  25.         'urineScore' => 0, // 实验室检查选填-尿常规得分
  26.     ];
  28.     ## 第一步 眼底图片上传 20分
  29.     $scores['pictureScore'] = sizeof($picture) == 1 ? 10 : 20;
  30.     ##  (此为糖网问卷，mmc问卷相关需要单独查询出来)
  31.     $questionnaireAnswer = TwUserNewSurvey::query()->from('z_user as u')
  32.             ->selectRaw('s.*,u.name,u.sex,u.bday,u.cell,7 as company_id')
  33.             ->leftJoin('tw_user_new_survey as s', 's.user_id', '=', 'u.id')
  34.             ->where('u.id', $userId)->get()->toArray()[0] ?? [];
  36.         //多选题组装
  37.     $multipleArr = [];
  38.     foreach ($questionnaireAnswer as $k => $v) {
  39.         if ($questionnaireAnswer[$k] && $questionnaireAnswer[$k] !== '0.00') {
  40.             // 处理每种情况数据
  41.             if (array_key_exists($k, self::MULTIPLE_MAPPING)) {
  42.                 $res = json_decode($questionnaireAnswer[$k], true);
  43.                 if (is_array($res)) {
  44.                     foreach ($res as $val) {
  45.                         $multipleArr[$k . $val] = self::MULTIPLE_MAPPING[$k][$val] ?? '';
  46.                     }
  47.                 } else {
  48.                     $multipleArr[$k . $questionnaireAnswer[$k]] = self::MULTIPLE_MAPPING[$k][$questionnaireAnswer[$k]] ?? '';
  49.                 }
  50.             }
  51.             //处理问卷中字典
  52.             if (array_key_exists($k, self::TW_ANSWER_MAPPING)) {
  53.                 $questionnaireAnswer[$k] = $this->twTransAnswer($questionnaireAnswer[$k], self::TW_ANSWER_MAPPING[$k]);
  54.             }
  55.         }else{
  56.             // 排除 sex   0 男  1 女
  57.             if ($k !== 'sex') {
  58.                 // 设置默认值为 ''
  59.                 $questionnaireAnswer[$k] = '';
  60.             }
  61.         }
  62.     }
  64.     ## 遍历问卷答案计算得分
  65.     foreach ($questionnaireAnswer as $k => $v) {
  66.         ## 基本信息
  67.         if (array_key_exists($k, self::BASE_SCORE)) {
  68.             if ($k === 'sex') {
  69.                 $scores['baseScore'] += self::BASE_SCORE[$k];
  70.                 continue;
  71.             }
  72.             if ($v && $v !== '0.00' && $v !== '0000-00-00') {
  73.                 $scores['baseScore'] += self::BASE_SCORE[$k];
  74.             }
  75.         }
  76.         ## 病史信息
  77.         if (array_key_exists($k, self::HISTORY_SCORE)) {
  78.             if ($v && $v !== '[]') {
  79.                 $scores['historyScore'] += self::HISTORY_SCORE[$k];
  80.             }
  81.         }
  82.         ## 实验室检查
  83.         if (array_key_exists($k, self::INSPECT_REQUIRED_SCORE)) {
  84.             if ($v && $v !== '[]' && $v !== '0.00') {
  85.                 $scores['inspectScore'] += self::INSPECT_REQUIRED_SCORE[$k];
  86.             }
  87.         }
  88.         if (array_key_exists($k, self::INSPECT_NOT_REQUIRED_SCORE)) {
  89.             if (in_array($k, ['pro', 'glu', 'leu', 'ery', 'nit']) && $v) {
  90.                 $scores['urineScore'] = 2;
  91.                 continue;
  92.             }
  93.             if ($v && $v !== '0.00') {
  94.                 $scores['inspectNotRequiredScore'] += self::INSPECT_NOT_REQUIRED_SCORE[$k];
  95.             }
  96.         }
  97.     }
  98.     Log::info("得分" . $userId .'----'. $scores['baseScore']);
  99.     // 实验室检查选填得分,最多获得10分
  100.     $scores['inspectNotRequiredScore'] = $scores['inspectNotRequiredScore'] + $scores['urineScore'];
  101.     $scores['inspectNotRequiredScore'] = $scores['inspectNotRequiredScore'] > 10 ? 10 : $scores['inspectNotRequiredScore'];
  102.     // 实验室检查总得分
  103.     $scores['inspectTotalScore'] = $scores['inspectScore'] +     $scores['inspectNotRequiredScore'];
  105.     $completeTotal = $scores['pictureScore'] + $scores['baseScore'] + $scores['historyScore'] + $scores['inspectTotalScore'];
  106.     $insertData = [
  107.         'user_id' => $userId,
  108.         'hosp_id' => $hospId,
  109.         'dept_id' => $deptId,
  110.         'is_mmc' => $isMmc,
  111.         'complete_picture' => $scores['pictureScore'],
  112.         'complete_base' => $scores['baseScore'],
  113.         'complete_history' => $scores['historyScore'],
  114.         'complete_inspect' => $scores['inspectTotalScore'],
  115.         'complete_total' => $completeTotal,
  116.         'complete_inspect_not_require' => $scores['inspectNotRequiredScore'],
  117.     ];
  118.     YanDiComplete::query()->updateOrCreate(['user_id' => $userId, 'dept_id' => $deptId], $insertData);
  119.     Log::info("sex". $questionnaireAnswer['sex']);
  120.     $insertAnswerData = [
  121.         'no' => date("Ymd"),
  122.         'hosp_id' => $yanDiInfo['hosp_id'],
  123.         'hosp_name' => $yanDiInfo['hosp_name'],
  124.         'dept_name' => $yanDiInfo['dept_name'],
  125.         'is_mmc' => $isMmc ? '是' : '否',
  126.         'user_id' => $userId,
  127.         'name' => $questionnaireAnswer['name'] ?? '',//姓名
  128.         'sex' => isset($questionnaireAnswer['sex']) ? ([0 => '男', 1 => '女'][$questionnaireAnswer['sex']] ?? '未知') : '未知',//性别
  129.         'bday' => $questionnaireAnswer['bday'] ?? '',//'出生日期',
  130.         'cell' => $questionnaireAnswer['cell'] ?? '',//'手机号',
  131.         'has_cell' => isset($questionnaireAnswer['cell']) && $questionnaireAnswer['cell'] ? '是' : '否',//'是否手机号验证',
  132.         'height' => $questionnaireAnswer['height'] ?? '',//'身高',
  133.         'weight' => $questionnaireAnswer['weight'] ?? '',//'体重',
  134.         'sbp' => $questionnaireAnswer['sbp'] ?? '',//'收缩压',
  135.         'dbp' => $questionnaireAnswer['dbp'] ?? '',//'舒张压',
  136.         'education' => $questionnaireAnswer['education'] ?? '',//'教育',
  137.         'province' => $this->getProvinceName($questionnaireAnswer['province'] ?? ''),//'目前的长期居住地',
  138.         'left_num' => $left_num, // 左眼图片数量
  139.         'right_num' => $right_num, // 右眼图片数量
  140.         'is_tnb' => $questionnaireAnswer['is_tnb'] ?? '',//'糖尿病史',
  141.         'tnb_start_time' => $questionnaireAnswer['tnb_start_time'] ?? '', //糖尿病开始时间
  142.         'is_high_blood' => $questionnaireAnswer['is_high_blood'] ?? '',//'高血压史',
  143.         'high_blood_start_time' => $questionnaireAnswer['high_blood_start_time'] ?? '',//'高血压开始时间',
  144.         'other_jzxjb_type' => $questionnaireAnswer['other_jzxjb_type'] ?? '',
  145.         'is_zfg' => $questionnaireAnswer['is_zfg'], //脂肪肝
  146.         'is_jzxjb' => $questionnaireAnswer['is_jzxjb'], //甲状腺疾病
  147.         'glu0' => $questionnaireAnswer['glu0'] ?? '',//'空腹血糖',
  148.         'hba1c' => $questionnaireAnswer['hba1c'] ?? '',//'糖化血红蛋白'
  149.         'cr' => $questionnaireAnswer['cr'] ?? '',//'血肌酐',
  150.         'ua' => $questionnaireAnswer['ua'] ?? '',//'血尿酸',
  151.         'tg' => $questionnaireAnswer['tg'] ?? '',//'甘油三酯TG',
  152.         'tc' => $questionnaireAnswer['tc'] ?? '',//'高密度脂蛋白胆固醇HDL-c',
  153.         'hdlc' => $questionnaireAnswer['hdlc'] ?? '',//'低密度脂蛋白胆固醇LDL-c',
  154.         'ldlc' => $questionnaireAnswer['ldlc'] ?? '',//'总胆固醇TC',
  155.         'record_at' => $yanDiInfo['created_at'],
  156.     ];
  157.     $insertAnswerData = array_merge($insertAnswerData, $multipleArr);
  158.     Log::info("aa", $insertAnswerData);
  159.     //dd($insertAnswerData);
  160.     YanDiAnswer::query()->where([
  161.         'no' => date("Ymd"),
  162.         'user_id' => $userId,
  163.         'dept_id' => $deptId,
  164.     ])->delete();
  165.     YanDiAnswer::query()->updateOrCreate([
  166.         'no' => date("Ymd"),
  167.         'user_id' => $userId,
  168.         'dept_id' => $deptId,
  169.     ], $insertAnswerData);
  170.     return true;
  171. }

重构，将子步骤抽取出来，单独成为函数，做到操作清晰，调用明确。

  1. <?php
  3. public function calculateCompletionScore($eyesInfo)
  4. {        
  5.       // 获取用户问卷填写详情
  6.     $answers = $this->getUserAnswers($eyesInfo['user_id']);
  7.     // 按照计算的方式组装处理 answer
  8.         $answers = $this->rebuildAnswerStructure($answers);
  9.     // 遍历计算完成度分数
  10.       $scores = $this->calculateScoreOfAnswers($answers);
  11.       ...
  12.     更新相关表结构
  13. }

除必要的注释外，被注释的不必要代码。一定不要带入生产环境。要删除干净，保持代码的洁净。

总结

  1. 代码分层要清晰，Controller 职责的接受 Request 并将验证的 Request 派送到 Service 层进行业务逻辑处理。Controller 一定不可以包含对业务逻辑的处理。具体业务逻辑的处理一定要在 Service 层编码。
  2. 代码职责问题。一个函数只允许有一个单一职责，如果代码函数超过 100行，一般都可以进行拆分和提炼出更小职责的函数。SRP 原则能有益于后续维护和需求变更。且如果有单元测试覆盖。能更好的的实践单元测试。利于单元测试的代码的编写。
  3. 一个项目的开发不是一个人的事情，是一个团队的事情。所以，养成良好的开发习惯和团队规范是必要的事情。这样才能持续写出易维护、运行稳定的代码。
  4. 具体的类的职责也不要超过类本身的职责。CommonUtils 就是一个错误的 OOP 抽象示范。
  5. 命名规范可以通过工具来保证。保持一致的命名规范和代码格式，是工程化的前提。
  6. 不允许直接拼接 SQL。使用 ORM 来进行数据库的操作。不允许在循环体中，进行数据库操作。会造成不可控的运行时间。
  7. 单例模式不一定使用于所有场景。不要盲目的 COPY 和应用。能静态声明（static）的方法可以尽量静态声明。
  8. 不要节省代码注释，你能读懂的，不代表所有人都能懂原来Code的意图。更多的注释能帮助更容易读懂代码。