我们不但要每天写代码，更应该时时停下来去看看自己的代码：下面是最近项目中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);
}
}
}