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