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