?
我們不但要每天寫代碼，更應該時時停下來去看看自己的代碼：下面是最近項目中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);
}
}
}