返回顶部

OutOfMemory.CN技术专栏-> wiki-> 代码评审歪诗

代码评审歪诗

更多

                             

        贾言      
       

验幻空越重,

       

命循频异长。

       

依轮线日简,

       

接偶正分壮。

     
       

言欢空月虫,

       

明勋品宜昌。

       

依伦先日贱,

       

洁偶正粉妆。

     

贾言

架构师说, 用20个字描述代码评审的内容, 自省也省人。由于是一字一含义, 不连贯, 为了增强趣味性, 每句都增加对应的歪解。只是对常见评审的描述, 不尽之处,欢迎补充!

  验幻空越重  -- 言欢空月虫

  验:公共方法都要做参数的校验,  参数校验不通过明确抛出异常或对应响应码。

     
  1.     java bean验证已经是一个很古老的技术了, 会避免我们很多问题, 可参考:    
              http://beanvalidation.org/ http://www.infoq.com/cn/news/2010/03/javaee6-validation https://www.sitepoint.com/using-java-bean-validation-method-parameters-return-values/      
  2.  
  3.     在接口中也明确使用验证注解修饰参数和返回值, 作为一种协议要求调用方按验证注解约束传参, 返回值验证注解约束提供方按注解要求返回参数  

  幻:在代码中要杜绝幻数,幻数可定义为枚举或常量以增强其可读性

  空:要时刻警惕空指针异常

     
  1.     常见的 a.equals(b) 要把常量放到左侧  
  2.  
  3.     aInteger == 10 如果 aInteger 为空时会抛出空指针异常  
  4.  
  5.     不确认返回集合是否可为空时要做非空判断, 再做for循环  
  6.  
  7.     使用空对象模式, 约定返回空集合, 而非null  
  8.  
  9.     使用StringUtils判断字符串非空          
         

  越:如果方法传入数组下标作为参数,要在一开始就做下标越界的校验,避免下标越界异常

  重:不要写重复代码,重复代码要使用重构工具提取重构

 

  命循频异长 -明勋品宜昌

  命:包/类/方法/字段/变量/常量的命名要遵循规范,要名副其实,这不但可以增加可读性,还可以在起名的过程中引导我们思考方法/变量/类的职责是否合适

有意义很重要, 典型无意义命名:


public static final Integer CODE_39120 = 39120;
public static final String MESSAGE_39120 = "[包裹]与[库房号]不一致,确定装箱?";
public static final Integer CODE_39121 = 39121;
public static final String MESSAGE_39121 = "[包裹]与[箱号]的承运类型不一致,确定装箱?";
 
Rule rule1 = request.getRuleMap().get("1050");

CODE_39120这个名字和幻数没多大区别。

  循:不要在循环中调用服务,不要在循环中做数据库等跨网络操作

  频:写每一个方法时都要知道这个方法的调用频率,一天多少,一分多少,一秒多少,峰值可能达到多少,调用频率高的一定要考虑性能指标,考虑是否会打垮数据库,是否会击穿缓存

  异:异常处理是程序员最基本的素质,不要处处捕获异常,对于捕获了只写日志,没有任何处理的catch要问一问自己,这样吃掉异常,是否合理

下面是一个反例, 在导出文件的controller方法中做了两层的try...catch, 在catch块中记录日志后什么都没做, 这样用户看不到真正想要的内容, 研发也只有看日志才能发现错误, 而“看日志”, 通常只有业务方反馈问题时才会看, 就会导致研发人员发现错误会比现场人员还会晚。


@RequestMapping(value = "/export")
public void export(CityRelationDomain condition, HttpServletResponse response) {
   ZipOutputStream zos = null;
   BufferedWriter bufferedWriter = null;
   try {
      condition.setStart(0);
      condition.setSize(MAX_EXPORT_LINES);
      List list = cityRelationService.getOrdersByCondition(condition);
      response.setCharacterEncoding("GBK");
      response.setContentType("multipart/form-data");
      response.setHeader("Content-Disposition", "attachment;fileName=export.zip");
      zos = new ZipOutputStream(response.getOutputStream());
      bufferedWriter = new BufferedWriter(new OutputStreamWriter(zos, "GBK"));
      bufferedWriter.write("订单类型编码,始发城市-省,始发城市-市,目的城市-省,目的城市-市");
      ZipEntry zipEntry = new ZipEntry("export.csv");
      zos.putNextEntry(zipEntry);
      for (CityRelationDomain domain : list) {
         try {
            bufferedWriter.newLine();
            bufferedWriter.write(CSVExportUtil.trans2CSV(domain.getOrderCode()));
            bufferedWriter.write(',');
            bufferedWriter.write(CSVExportUtil.trans2CSV(domain.getProvinceNameFrom()));
            bufferedWriter.write(',');
            bufferedWriter.write(CSVExportUtil.trans2CSV(domain.getCityNameFrom()));
            bufferedWriter.write(',');
            bufferedWriter.write(CSVExportUtil.trans2CSV(domain.getProvinceNameTo()));
            bufferedWriter.write(',');
            bufferedWriter.write(CSVExportUtil.trans2CSV(domain.getCityNameTo()));
         } catch (Exception e) {
            e.printStackTrace();
         }
      }
      bufferedWriter.newLine();
           bufferedWriter.flush();
           zos.closeEntry();
           bufferedWriter.close();
   } catch (Exception e) {
      e.printStackTrace();
      logger.error("导出CSV文件异常");
   } finally {
      try {
         if (zos != null) {
            zos.close();
         }
         if (bufferedWriter != null) {
            bufferedWriter.close();
         }
      } catch (IOException e) {
         e.printStackTrace();
      }
   }
}

  长:如果一行代码过长,要分解开来;如果一个方法过长,要重构方法;如果一个类过长要考虑拆分类

  依轮线日简 -依伦先日贱

  依:如果调用了外部依赖,一定要搞清楚这个外部依赖可以提供的性能指标,最好约定SLA

  轮:不要重复造轮子,如果已经有成熟类库实现了类似功能,要优先使用成熟类库的方法,这是因为成熟类库中的方法都经过很多人的测试验证,通常情况下我们自己实现的质量最大等同于成熟类库的质量。

  线:要注意我们的jsf服务, web应用,消费消息的worker都是多线程环境,要注意线程安全问题,最典型的HashMap, SimpleDateFormat, ArrayList是非线程安全的,另外如果使用Spring自动扫描服务,那么这个服务默认是单例,其内部成员是多个线程共享的,如果直接用成员变量是有线程不安全的。

两个典型的错误代码片段:

  1)无视SimpleDateFormat非线程安全


@Service
public class AService {
    private static final SimpleDateFormat FORMAT = new SimpleDateFormat("yyyy-MM-dd");

    public void doSomething() {
        //use FORMAT
    }
}

 

  2)使用Service成员变量


@Service
public class BService {
    private Pojo b;

    public void doB() {
         b = getB();
         process(b);
    }
}

 

  日:打印日志和设定合理的日志级别,如有必要要添加if条件限定是否打印日志,在日志中使用JSON序列化,生成长字符串的toString()都要做if限定打印,否则配置的日志级别没达到,也会做大量字符串拼接,占用很多gc年轻代内存.  另外一定要通过log4j打印日志而不是直接把日志打印到控制台。

 

典型错误示例:


@Service
public class FooService {
    private static final Logger LOGGER = LoggerFactory.getLogger(FooService.class);

    public void doFooThing(Foo foo) {
        LOGGER.debug("get parameter foo {}", JSONObject.toString(foo));
        try {/*do something*/} catch (Exception ex) {ex.printStackTrace();}
    }
}

 

  简: 尽可能保持整体设计的简洁, 方法实现的简洁, 要根据情况使用内存缓存, redis 缓存, jmq 异步处理。 这里的简需要把握好分寸。

  接偶正分壮 - 洁偶正粉妆

  接:接口是用来隔离变化的,如果一个业务有几种不同的形态,但都有相同的处理,那么可以定义接口来隔离业务形态的不同,在服务调用处,通过业务类型字段来获得不同的服务类。  而不要实现一个类,然后在类的各个方法中都根据业务类型做if else或更复杂的各种判断。

典型示例:

  做法1:

public interface BarService {    void doBarThing(Bar b);
    
    void doBarFatherThing(Bar b);
}
public class BarServiceImpl  implement BarService{
    public void doBarThing(Bar b) {
        if (b.getType() == BarType.A) {
            //do some logic
        } else (b.getType() == BarType.B) {
            //do some B type logic
        }
        //do other doBarThing logic
    }
    
    public void doBarFatherThing(Bar b) {
        if (b.getType() == BarType.A) {
            //do some logic
        } else (b.getType() == BarType.B) {
            //do some B type logic
        }
        //do other doBarFatherThing logic
    }
}

 

做法 2 :

public interface BarService {
    void doBarThing(Bar b);
    
    void doBarFatherThing(Bar b);
}
public class BarServiceFactory {
    public BarService getBarService(BarType type) {
        // get bar service logic
    }
}
//如果有公共逻辑就定义, 没有就不定义
public class BaseBarService implement BarService {
    public void doBarThing(Bar b) {
        //do other doBarThing logic
    }
    
    public void doBarFatherThing(Bar b) {
        //do other doBarFatherThing logic
    }
    
}
public class TypeABarService extends BaseBarService  implement BarService {
    public void doBarThing(Bar b) {
        // doATypeThing
        super.doBarThing(b);
    }
    
    public void doBarFatherThing(Bar b) {
        // do bar type A service
super.doBarFatherThing(b); //如果需要就调用, 不需要就不调用父类
    }
    
}

 

  做法2的好处是将不同类型的逻辑解耦,各自发展,不会相互影响,如果添加类型也不必影响现有类型逻辑。

 

  偶:认识系统之间的耦合关系,通过同步数据来做两个系统之间的交互是一种很强的耦合关系,会使数据接收方依赖于数据发送方的数据库定义,如果发送方想改数据结构,必须要求下游接收方一起修改;通过接口调用是一种常见的系统耦合关系,接口的提供方要保证接口的可用性,接口的调用方要考虑接口不可用时的应对方案; mq消息是一种解耦的方法,两个系统不存在实时的耦合关系。但是mq解耦的方式不能滥用,在同一系统内不宜过多使用mq消息来做异步,要尽可能保证接口的性

  能,而不是通过mq防止出问题后重新消费。

 

  正:模块之间依赖关系要正向依赖,不能让底层模块依赖于上层模块;不能让数据层依赖于服务层也不能让服务层依赖于UI层;也不能在模块之间形成循环依赖关系。

 

  分:分而治之,复杂的问题要分解成几个相对简单的问题来解决,首先要分析出核心问题,然后分析出核心的入参是什么,结果是什么,入参通过几步变化可以得出结果。

 

  壮:时刻注意程序的健壮性,从两个方面实践提升健壮性:

  1)契约,在设计接口时定义好协议参数,并在实现时第一时间校验参数,如果参数有问题,直接返回给调用方; 如果出现异常情况, 也按异常情况约定应对策略

2)  考虑各种边界条件的输出, 比如运单号查询服务, 要考虑用户输入错误运单时怎么返回, 有边界的查询条件, 如果用户查询条件超过边界了, 应该返回什么

  3   )为失败做设计,如果出问题了有降级应对方案。

推荐阅读:
支持

1

反对

0

发表评论