51Testing软件测试论坛

标题: Code Review 总结 [打印本页]
作者: 海上孤帆    时间: 2019-2-27 14:41
标题: Code Review 总结
就在昨天，我们团队进行了一次代码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();

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

  11.         int times = hiService.getTimes(userId);
  12.         hiVO.setTimes(times);

  14.         String url = fileResourceService.getUrl(user.getHeaderPicId());
  15.         hiVO.setHeadPicUrl(url);

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

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()方法，谁也不知道得到的是什么数据，只有去看实现才能知道，这样的代码很难维护，为了降低维护成本，命名需要更加斟酌考虑。






欢迎光临 51Testing软件测试论坛 (http://bbs.51testing.com/) Powered by Discuz! X3.2