code review --没有完美的代码（一）

 

我们不但要每天写代码，更应该时时停下来去看看自己的代码：下面是最近项目中code review 发现的一些问题；好的code review 不仅可以减少bug，更加是一个互相学习的过程：

 

 

一、代码背景：很多时候都会碰到数据类型的转换，特别是string 和number之间的转换，这个时候处理的不好，碰上复杂的业务场景，在用户行为不可控的时候，甚至遭到恶意访问的时候，很容易抛出大量异常，降低应用的吐出；

 

 

 if (NumberUtils.isNumber(product.getProductGroupId())) {
          Integer productGroupId = Integer.valueOf(productSearch.getProductGroupId());
          ProductGroupDTO productGroupDTO =productService.findProductGroupDTOByGroupId(productGroupId);
   }

 

这段代码有两个问题：

 

  1、使用了 org.apache.commons.lang.NumberUtils：

很多时间我们在引入类的时候，不会特别关心到底是用的哪一个jar包里面的；对于eclipse等工具的警告和建议也不会去在意；事实上我们应该写一段干净的代码，而不是暂时没有错误的代码；这里应该用 org.apache.commons.lang.math.NumberUtils

 

  2、Integer.valueOf()这个函数当转换失败的时候会跑出NumberFormatException;对于这里并不需要取记录这种错误数据的场景，可以很好的避免这个问题：

修改后如下：

 

 if (StringUtils.isNotBlank(product.getProductGroupId())) {

        Integer authProductGroupId = NumberUtils.toInt(product.getproductGroupId(), -1);
        ProductGroupDTO productGroupDTO =productService.findProductGroupDTOByGroupId(productGroupId);
      }

  

二、代码背景：很多时候我们都需要做一些参数合法性的判断，但是代码写的欢了容易忽略判断的顺序有时候也是很重要的，见下面代码：

 

 

 

 if (companyId <= 0 || companyId == null) {
            logger.error("companyId:" + (companyId == null ? "is null" : companyId.toString())    + ",please check company id.");
           return true;
  }

 

 

 使用foundbug 很容易发现问题：nullpointer early

 

 修改后：

 

 

if ( companyId == null || companyId <= 0 ) {
            logger.error("companyId:" + (companyId == null ? "is null" : companyId.toString())    + ",please check company id.");
           return true;
  }

 

 

三、代码背景：同样是参数合法性的判断，这次的错误就更加隐蔽了：

 

 

  if (map == null && map.size() == 0) {}

 

 

修改后的：

 

 

 if (map == null || map.size() == 0) {}

 

 

 四、 代码背景：很多时候会在一个方法里面去构建一个对象，首先是满足一定的条件，构建对象，设置部分属性，满足另外一个部分条件，在设置另外一部分属性，如果都不满足，返回null；看下面代码：

 

 

public ReportView  buildReportView  (){
   ReportView  reportView  = null;
   if (isHaveReportOne()) {   
            reportView = buildReportOneView(ReportDTOOne);
   }
   if ((isHaveReportTwo()) {
            setReportTwoDTO(reportView , ReportDTOTwo);
            reportView.setOther();
}
    

 private  void  setReportTwoDTO(ReportView reportView , ReportTwoDTO reportTwoDTO){

     if(reportView = null)
     {
         reportView   = new  reportView()
     }
 }

 

  这个问题的关键其实就是：传值还是传引用，假设 isHaveReportOne() ==false;在第二个函数中我们自以为new 了新的对象；并且设置了其他的属性；但是在reportView.setOther()确任然跑出NullPointerException,

 

 

public ReportView  buildReportView  (){
   ReportView  reportView  = null;
   if (isHaveReportOne()) {   
            reportView = buildReportOneView(ReportDTOOne);
   }
   if ((isHaveReportTwo()) {
            reportView =    setReportTwoDTO(reportView , ReportDTOTwo);
            reportView.setOther();
}

 

 

 

private  ReportView  setReportTwoDTO(ReportView reportView , ReportTwoDTO reportTwoDTO){

     if(reportView = null)
     {
         reportView   = new  reportView()
     }
     return reportView   ;
 }

 

 

五、在使用ibatis的时候，通常xml，DAO，DO都是自动生成的，这个时候update会有两个方法，一种是全部更新，另外一个只是更新不为null的值。这个时候使用全部更新要特别注意，如果是不允许用户删除某个字段的修改，最好使用第二种更新。

 

 

六、代码背景：List<DataDto> reports 保存了从DB取出的数据，但是由于涉及到机密，需要将一部分值替换为一个常数；

 

 

public void filterHideItem(List<DataDto> reports) {

        if (reports == null || reports.isEmpty()) {

            return;

        }

        List<DataItemPropertiesDO> hideReportsOne = listAllHideReportsOne();

        List<DataItemPropertiesDO> hideReportsTwo = listAllHideReportsTwo();

        Set<Integer> hideSet = new HashSet<Integer>(( hideReportsOne .size() + hideReportsTwo.size()) * 4 / 3);

        ListTOSet(hideSet, hideReportsOne );

        ListTOSet(hideSet, hideReportsTwo );

       // 循环比较如果是在需要被屏蔽的集合里面，则替换为一个常数

        for (DataDto dataDto: reports) {

            filterReport(hideSet, dataDto);

        }

    }

    private void filterReport(Set<Integer> hideReports, DataDto  report) {

        if (report == null) {

            return;

        }
    //需要过滤的报告数据
        List<NormalDataDto> list = report.getNormalReports();

        for (NormalDataDto normalDto : list) {

            if (hideReports.contains(normalDto.getId())) {

                normalDto.getSummaryKeyValueMap().put(Constants.KEY_ONE,

                                                      Constants.VALUE);

                normalDto.getSummaryKeyValueMap().put(Constants.KEY_TWO,

                                                      Constants.VALUE);

            }

        }

    }

    // 获取 第一种类型需要被屏蔽的数据

    private List<DataItemPropertiesDO> listAllHideReportsOne() {

        DataItemPropertiesDO param = new DataItemPropertiesDO();

        param.setKey(Constants.KEY_ONE);

        param.setName(Constants.ATTRIBUTE_HIDE);

        param.setValue(Constants.ATTRIBUTE_HIDE_TRUE_VALUE);

        List<DataItemPropertiesDO> list = service.listItemPropertiesByParam(param);

        if (list == null || list.isEmpty()) {

            return null;

        }

        return list;

    }

   //获取 另外一种需要被屏蔽的数据

    private List<DataItemPropertiesDO> listAllHideReportsTwo() {

        
        DataItemPropertiesDO param = new DataItemPropertiesDO();

        param.setKey(Constants.KEY_ONE);

        param.setName(Constants.ATTRIBUTE_HIDE);

        param.setValue(Constants.ATTRIBUTE_HIDE_TRUE_VALUE);

        List<DataItemPropertiesDO> list = service.listItemPropertiesByParam(param);

        if (list == null || list.isEmpty()) {

            return null;

        }

        return list;
    }

    // 将List 转换为 set

    private void ListTOSet(Set<Integer> hideSet, List<DataItemPropertiesDO> list) {

        for (DataItemPropertiesDO prop : list) {

            hideSet.add(prop.getReportId());

        }

    }

 

 

 

第一个问题很容易发现：nullpointer 是javaer最常见也最防不胜防的问题；很多公司更是作为一个约定不允许任何方法返回null；问题就出现在

 

 

 Set<Integer> hideSet = new HashSet<Integer>((hideReportsOne.size() + hideReportsTwo.size()) * 4 / 3);

 

 

   这里的 hideReportsOne ，hideReportsTwo 可能为null； 一方面提供服务的一方 要尽可能的不返回 null 可以考虑  Collections.emptyList() 代替 null；另一方面  永远不能依赖别人的正确性，

 

 第二个问题也不难，  可以发现 listAllHideReportsOne 和 listAllHideReportsTwo 是非常类似，只是查询的key不相同,如果将来在新增一个需要过滤的数据，现在的方式必然要再新增一个类似的方法，可见可扩展性非常不好：

 

一种方案：

 

 

     private List<String>               hideItemKeys;

      public ItemHideFilterProcessor() {

        hideItemKeys = new ArrayList<String>(2);

        hideItemKeys.add(Constants.KEY_ONE);

        hideItemKeys.add(Constants.KEY_TWO);

    }
 
    public void filterHideItem(List<DataDto> reports) {

        if (reports == null || reports.isEmpty()) {

            return;

        }

        List<ItemHideReport> itemHideReportList = buildItemHideReports();

        if (itemHideReportList.isEmpty()) {

            return;

        }

        for (DataDto dataDto : reports) {

            filterReport(itemHideReportList, dataDto );

        }

    }

    private void filterReport(List<ItemHideReport> hideReportSetList, DataDto report) {

        if (report == null) {

            return;

        }

        List<NormalDataDto> list = report.getNormalReports();

        for (NormalDataDto normalDto : list) {

            for (ItemHideReport hideReport : hideReportSetList) {

                if (hideReport.isHide(normalDto.getId())) {

                    normalDto.getSummaryKeyValueMap().put(hideReport.getItemKey(),

                                                          CertifiedReportConstants.CONFIDENTIAL);

                }

            }

        }

    }

    private List<ItemHideReport> buildItemHideReports() {

        List<ItemHideReport> list = new ArrayList<ItemHideReport>(hideItemKeys.size());

        for (String hideKey : hideItemKeys) {

            Set<Integer> reportSet = listAllItemHideReportsByKey(hideKey);

            if (reportSet != null) {

                list.add(new ItemHideReport(hideKey, reportSet));

            }

        }

        return list;

    }

    private Set<Integer> listAllItemHideReportsByKey(String key) {

        DataItemPropertiesDO param = new DataItemPropertiesDO();

        param.setKey(key);

        param.setName(Constants.ATTRIBUTE_HIDE);

        param.setValue(Constants.ATTRIBUTE_HIDE_TRUE_VALUE);

        List<DataItemPropertiesDO> list = itemService.listItemPropertiesByParam(param);

        if (list == null || list.isEmpty()) {

            return null;

        }

        Set<Integer> set = new HashSet<Integer>(list.size() * 4 / 3);

        for (DataItemPropertiesDO prop : list) {

            set.add(prop.getReportId());

        }

        return set;

    }

    class ItemHideReport {
        String       itemKey;
        Set<Integer> hideReports;

        public ItemHideReport(String itemKey, Set<Integer> hideReports) {

            this.itemKey = itemKey;

            this.hideReports = hideReports;

        }

        public String getItemKey() {

            return itemKey;

        }

        public boolean isHide(Integer reportId) {

            return hideReports.contains(reportId);

        }

    }

}