?
我們不但要每天寫代碼，更應(yīng)該時時停下來去看看自己的代碼：下面是最近項目中code review 發(fā)現(xiàn)的一些問題；好的code review 不僅可以減少bug，更加是一個互相學(xué)習(xí)的過程：
?
?
一、代碼背景：很多時候都會碰到數(shù)據(jù)類型的轉(zhuǎn)換，特別是string 和number之間的轉(zhuǎn)換，這個時候處理的不好，碰上復(fù)雜的業(yè)務(wù)場景，在用戶行為不可控的時候，甚至遭到惡意訪問的時候，很容易拋出大量異常，降低應(yīng)用的吐出；
?
?
if (NumberUtils.isNumber(product.getProductGroupId())) {
Integer productGroupId = Integer.valueOf(productSearch.getProductGroupId());
ProductGroupDTO productGroupDTO =productService.findProductGroupDTOByGroupId(productGroupId);
}
?
這段代碼有兩個問題：
?
? 1、使用了?org.apache.commons.lang.NumberUtils：
很多時間我們在引入類的時候，不會特別關(guān)心到底是用的哪一個jar包里面的；對于eclipse等工具的警告和建議也不會去在意；事實上我們應(yīng)該寫一段干凈的代碼，而不是暫時沒有錯誤的代碼；這里應(yīng)該用?org.apache.commons.lang.math.NumberUtils
?
? 2、Integer.valueOf()這個函數(shù)當(dāng)轉(zhuǎn)換失敗的時候會跑出NumberFormatException;對于這里并不需要取記錄這種錯誤數(shù)據(jù)的場景，可以很好的避免這個問題：
修改后如下：
?
?if (StringUtils.isNotBlank(product.getProductGroupId())) {
Integer authProductGroupId = NumberUtils.toInt(product.getproductGroupId(), -1);
ProductGroupDTO productGroupDTO =productService.findProductGroupDTOByGroupId(productGroupId);
}
??
二、代碼背景：很多時候我們都需要做一些參數(shù)合法性的判斷，但是代碼寫的歡了容易忽略判斷的順序有時候也是很重要的，見下面代碼：
?
?
?
if (companyId <= 0 || companyId == null) {
logger.error("companyId:" + (companyId == null ? "is null" : companyId.toString()) + ",please check company id.");
return true;
}
?
?
?使用foundbug 很容易發(fā)現(xiàn)問題：nullpointer early
?
?修改后：
?
?
if ( companyId == null || companyId <= 0 ) {
logger.error("companyId:" + (companyId == null ? "is null" : companyId.toString()) + ",please check company id.");
return true;
}
?
?
三、代碼背景：同樣是參數(shù)合法性的判斷，這次的錯誤就更加隱蔽了：
?
?
if (map == null && map.size() == 0) {}
?
?
修改后的：
?
?
if (map == null || map.size() == 0) {}
?
?
?四、 代碼背景：很多時候會在一個方法里面去構(gòu)建一個對象，首先是滿足一定的條件，構(gòu)建對象，設(shè)置部分屬性，滿足另外一個部分條件，在設(shè)置另外一部分屬性，如果都不滿足，返回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()
}
}
?
? 這個問題的關(guān)鍵其實就是：傳值還是傳引用，假設(shè) isHaveReportOne() ==false;在第二個函數(shù)中我們自以為new 了新的對象；并且設(shè)置了其他的屬性；但是在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取出的數(shù)據(jù)，但是由于涉及到機密，需要將一部分值替換為一個常數(shù)；
?
?
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 );
// 循環(huán)比較如果是在需要被屏蔽的集合里面，則替換為一個常數(shù)
for (DataDto dataDto: reports) {
filterReport(hideSet, dataDto);
}
}
private void filterReport(Set<Integer> hideReports, DataDto report) {
if (report == null) {
return;
}
//需要過濾的報告數(shù)據(jù)
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);
}
}
}
// 獲取 第一種類型需要被屏蔽的數(shù)據(jù)
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;
}
//獲取 另外一種需要被屏蔽的數(shù)據(jù)
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 轉(zhuǎn)換為 set
private void ListTOSet(Set<Integer> hideSet, List<DataItemPropertiesDO> list) {
for (DataItemPropertiesDO prop : list) {
hideSet.add(prop.getReportId());
}
}
?
?
?
第一個問題很容易發(fā)現(xiàn)：nullpointer 是javaer最常見也最防不勝防的問題；很多公司更是作為一個約定不允許任何方法返回null；問題就出現(xiàn)在
?
?
Set<Integer> hideSet = new HashSet<Integer>((hideReportsOne.size() + hideReportsTwo.size()) * 4 / 3);
?
?
?? 這里的
hideReportsOne?，hideReportsTwo?可能為null；?一方面提供服務(wù)的一方 要盡可能的不返回 null 可以考慮 ?Collections.emptyList() 代替 null；另一方面 ?永遠(yuǎn)不能依賴別人的正確性，
?
?第二個問題也不難，? 可以發(fā)現(xiàn) listAllHideReportsOne 和 listAllHideReportsTwo 是非常類似，只是查詢的key不相同,如果將來在新增一個需要過濾的數(shù)據(jù)，現(xiàn)在的方式必然要再新增一個類似的方法，可見可擴(kuò)展性非常不好：
?
一種方案：
?
?
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);
}
}
}