51Testing软件测试论坛

 找回密码
 (注-册)加入51Testing

QQ登录

只需一步,快速开始

微信登录,快人一步

手机号码,快捷登录

查看: 1627|回复: 0
打印 上一主题 下一主题

[原创] Code Review 总结

[复制链接]
  • TA的每日心情
    无聊
    2024-10-29 09:20
  • 签到天数: 76 天

    连续签到: 1 天

    [LV.6]测试旅长

    跳转到指定楼层
    1#
    发表于 2019-2-27 14:41:12 | 只看该作者 回帖奖励 |倒序浏览 |阅读模式
    就在昨天,我们团队进行了一次代码review大会,此博客记录自己写代码时候被指出的问题,以免再犯同样的错误。

    1.直接返回结果,难调试
    1. return ApiResult.success(userService.get(userId));
    复制代码
    最开始的写法如上所示,其中userService返回的是user对象,ApiResult.success会将对象通过Json返回给前端。
    虽然在调试的过程中可以通过右键 Add to Watches查看该user对象(使用的是Idea),但还是不直观, 不好调。最终最直观的方法如下:
    1. User user = userService.get(userId);
    2. return ApiResult.success(user);
    复制代码
    2.代码结构太长

    在最近的一次迭代中,有一个service方法由于要拿很多数据就写了120行左右,代码都放在一起很难维护,其他开发人员看的欲望都没了。
    通过今天简单的重构一下,将获取相应数据模块的代码抽取出来以后,每个方法都在40行以内,降低了阅读成本,代码也更优雅了。


    3.查询数据库次数过多
    1. for(Integer id : ids){
    2.      userService.get(id);
    3. }
    复制代码
    由于项目是通过Jar包依赖,再多加个接口,需要重新打包发布依赖。
    有些时候为了偷懒,会像上面这样写,这样就不需要在写一个接口了,这样导致的结果就是一个简单的接口查询太多次数据库,当项目越来越大以后,访问量越来越大,数据库可能就撑不住了。

    4.缺少应有的空行
    1. HiVO hiVO = new VO();
    2.         User user = userService.get(userId);
    3.         if(user == null)
    4.             return null;
    5.         hiVO.setGender(user.getGender());
    6.         hiVO.setUserId(userId);
    7.         hiVO.setUserName(user.getUserNickName());
    8.         hiVO.setPosition(user.getPosition());
    9.         int times = hiService.getTimes(userId);
    10.         hiVO.setTimes(times);
    11.         String url = fileResourceService.getUrl(user.getHeaderPicId());
    12.         hiVO.setHeadPicUrl(url);
    13.         List<UserPhotoDTO> userPhoneDTOs = userPhotoService.getByUserId(userId);
    14.         if(userPhoneDTOs != null){
    15.             for(UserPhotoDTO userPhotoDTO : userPhoneDTOs){
    16.                 if(userPhotoDTO.isCover()){
    17.                     hiVO.setCoverPicUrl(userPhotoDTO.getPicUrl());
    18.                 }
    19.             }
    20.             hiVO.setPhotos(userPhoneDTOs);
    21.         }
    复制代码
    上面的代码一眼看去是知道在填充数据,由于没有注释,要看哪里填充了什么数据就有些不太明显了,这个时候适当的添加个空行能起到注释的作用。
    如下图所示:首先填充用户的基本信息,再填充service信息,头像信息等,一目了然。
    1.   HiVO hiVO = new VO();

    2.         User user = userService.get(userId);
    3.         if(user == null)
    4.             return null;
    5.         hiVO.setGender(user.getGender());
    6.         hiVO.setUserId(userId);
    7.         hiVO.setUserName(user.getUserNickName());
    8.         hiVO.setPosition(user.getPosition());

    9.         int times = hiService.getTimes(userId);
    10.         hiVO.setTimes(times);

    11.         String url = fileResourceService.getUrl(user.getHeaderPicId());
    12.         hiVO.setHeadPicUrl(url);

    13.         List<UserPhotoDTO> userPhoneDTOs = userPhotoService.getByUserId(userId);
    14.         if(userPhoneDTOs != null){
    15.             for(UserPhotoDTO userPhotoDTO : userPhoneDTOs){
    16.                 if(userPhotoDTO.isCover()){
    17.                     hiVO.setCoverPicUrl(userPhotoDTO.getPicUrl());
    18.                 }
    19.             }
    20.             hiVO.setPhotos(userPhoneDTOs);
    21.         }
    复制代码

    5.省略{}
    1. for(Integer id : userIds){
    2.     User user = userService.get(id);
    3.     if(user.getTime() > hiUser.getTime())
    4.         return user.getTime();
    5.     else if(user.getTime() < hiUser.getTime())
    6.         return hiUser.getTime();
    7. }
    复制代码
    一开始觉得if省略{}更加简洁,但这是个不好的习惯,别人看着会很累,改也容易犯错。关键的是,在很长的代码中嵌入这样的代码,那就更加难以排查错误,为了降低阅读成本和出错率的话还是把{}加上吧。
    1. for(Integer id : userIds){
    2.     User user = userService.get(id);
    3.     if(user.getTime() > hiUser.getTime()){
    4.         user.getTime();
    5.     }else if(user.getTime() < hiUser.getTime()){
    6.         return hiUser.getTime();
    7.     }
    8. }
    复制代码

    6.命名不直观
    1. Collections.sort(userDTOs, new Comparator<User>() {
    2.             @Override
    3.             public int compare(User o1, User o2) {
    4.                 if(o1.getTimes() > o2.getTimes()){
    5.                     return -1;
    6.                 }else if(o1.getTimes() == o2.getTimes()){
    7.                     return 0;
    8.                 }
    9.                 return 1;
    10.             }
    11. });
    复制代码
    在上面的比较器中,将user变量改为o1,o2就不能一眼知道比较的是什么东西,需要转一个弯才知道比较的是用户的次数。这只是其中的一个案例,例如有一个getData()方法,谁也不知道得到的是什么数据,只有去看实现才能知道,这样的代码很难维护,为了降低维护成本,命名需要更加斟酌考虑。


    分享到:  QQ好友和群QQ好友和群 QQ空间QQ空间 腾讯微博腾讯微博 腾讯朋友腾讯朋友
    收藏收藏
    回复

    使用道具 举报

    本版积分规则

    关闭

    站长推荐上一条 /1 下一条

    小黑屋|手机版|Archiver|51Testing软件测试网 ( 沪ICP备05003035号 关于我们

    GMT+8, 2024-11-22 03:23 , Processed in 0.073595 second(s), 23 queries .

    Powered by Discuz! X3.2

    © 2001-2024 Comsenz Inc.

    快速回复 返回顶部 返回列表