Code Review 总结一

就在08-02昨天，我们团队进行了一次代码review大会,此博客记录自己写代码时候被指出的问题，以免再犯同样的错误。

1.直接返回结果，难调试

return ApiResult.success(userService.get(userId));

最开始的写法如上所示，其中userService返回的是user对象，ApiResult.success会将对象通过Json返回给前端。
虽然在调试的过程中可以通过右键 Add to Watches查看该user对象(使用的是Idea)，但还是不直观， 不好调。最终最直观的方法如下：

User user = userService.get(userId);return ApiResult.success(user);

2.代码结构太长

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

3.查询数据库次数过多

 for(Integer id : ids){     userService.get(id); }

由于项目是通过Jar包依赖，再多加个接口，需要重新打包发布依赖。
有些时候为了偷懒，会像上面这样写，这样就不需要在写一个接口了，这样导致的结果就是一个简单的接口查询太多次数据库，当项目越来越大以后，访问量越来越大，数据库可能就撑不住了。

4.缺少应有的空行

        HiVO hiVO = new VO();        User user = userService.get(userId);        if(user == null)            return null;        hiVO.setGender(user.getGender());        hiVO.setUserId(userId);        hiVO.setUserName(user.getUserNickName());        hiVO.setPosition(user.getPosition());        int times = hiService.getTimes(userId);        hiVO.setTimes(times);        String url = fileResourceService.getUrl(user.getHeaderPicId());        hiVO.setHeadPicUrl(url);        List<UserPhotoDTO> userPhoneDTOs = userPhotoService.getByUserId(userId);        if(userPhoneDTOs != null){            for(UserPhotoDTO userPhotoDTO : userPhoneDTOs){                if(userPhotoDTO.isCover()){                    hiVO.setCoverPicUrl(userPhotoDTO.getPicUrl());                }            }            hiVO.setPhotos(userPhoneDTOs);        }           

上面的代码一眼看去是知道在填充数据，由于没有注释，要看哪里填充了什么数据就有些不太明显了，这个时候适当的添加个空行能起到注释的作用。
如下图所示：首先填充用户的基本信息，再填充service信息，头像信息等，一目了然。

        HiVO hiVO = new VO();        User user = userService.get(userId);        if(user == null)            return null;        hiVO.setGender(user.getGender());        hiVO.setUserId(userId);        hiVO.setUserName(user.getUserNickName());        hiVO.setPosition(user.getPosition());        int times = hiService.getTimes(userId);        hiVO.setTimes(times);        String url = fileResourceService.getUrl(user.getHeaderPicId());        hiVO.setHeadPicUrl(url);        List<UserPhotoDTO> userPhoneDTOs = userPhotoService.getByUserId(userId);        if(userPhoneDTOs != null){            for(UserPhotoDTO userPhotoDTO : userPhoneDTOs){                if(userPhotoDTO.isCover()){                    hiVO.setCoverPicUrl(userPhotoDTO.getPicUrl());                }            }            hiVO.setPhotos(userPhoneDTOs);        }       

5.省略{}

for(Integer id : userIds){    User user = userService.get(id);    if(user.getTime() > hiUser.getTime())        return user.getTime();    else if(user.getTime() < hiUser.getTime())        return hiUser.getTime();}

一开始觉得if省略{}更加简洁，但这是个不好的习惯，别人看着会很累，改也容易犯错。关键的是，在很长的代码中嵌入这样的代码，那就更加难以排查错误，为了降低阅读成本和出错率的话还是把{}加上吧。

for(Integer id : userIds){    User user = userService.get(id);    if(user.getTime() > hiUser.getTime()){        user.getTime();    }else if(user.getTime() < hiUser.getTime()){        return hiUser.getTime();    }}

6.命名不直观

Collections.sort(userDTOs, new Comparator<User>() {            @Override            public int compare(User o1, User o2) {                if(o1.getTimes() > o2.getTimes()){                    return -1;                }else if(o1.getTimes() == o2.getTimes()){                    return 0;                }                return 1;            }});

在上面的比较器中，将user变量改为o1,o2就不能一眼知道比较的是什么东西，需要转一个弯才知道比较的是用户的次数。这只是其中的一个案例，例如有一个getData()方法，谁也不知道得到的是什么数据，只有去看实现才能知道，这样的代码很难维护，为了降低维护成本，命名需要更加斟酌考虑。