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();
   
2. 
   
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());
   
10. 
    
11.         int times = hiService.getTimes(userId);
    
12.         hiVO.setTimes(times);
    
13. 
    
14.         String url = fileResourceService.getUrl(user.getHeaderPicId());
    
15.         hiVO.setHeadPicUrl(url);
    
16. 
    
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()方法，谁也不知道得到的是什么数据，只有去看实现才能知道，这样的代码很难维护，为了降低维护成本，命名需要更加斟酌考虑。