LCMS Code Review

概述

lcms项目前期开发人员最多时有10人，而且时间跨度大（从开始开发到最终上线有1年）。多人合作下，代码的质量也会有影响。加上大众对交付物有一些硬性要求，所以code review很有必要。

本文记录了一些在code review中，一些写代码的不好的习惯，以及对SSH的不理解或者理解不深的地方。

Code Review

• 代码书写
  大众有专门的checkstyle规范（svw_checkstyle_configuration.xml）、代码模板（svw_codeStyle_codeTemplates_configuration.xml）和书写格式（svw_codeStyle_formatter_configuration.xml）要求，以及findBugs要求。
  在IDE中安装对应的checkstyle和findbugs插件，可以有效减少开发过程中的一些简单错误

• 输入条件边界检验
  很多输入参数需要做非空判断后再使用，防止空指针等异常

• 异常不处理
  开发中，异常很容易被忽略
  对于无需处理的异常，需要打印日志logger.error(“异常详细”, e)

    public void inputExcel() {
        try {
            //业务代码
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

• DAO异常错误处理
  很多前台页面的保存、更新操作，需要给客户反馈成功与否，所以就会出现这样的代码

    /**
     * 
     * <p>
     * Description:添加或者更新对象
     * </p>
     * 
     * @param model 对象
     * @return 布尔值
     */
    @Override
    public boolean saveOrUpdate(ContainerTransmit model) {
        boolean containerTraFlag;
        containerTraFlag = true;
        //执行添加或者修改
        try {
            Session session;
            session = this.sessionFactory.getCurrentSession();
            session.saveOrUpdate(model);
            session.flush();
        } 
        catch (HibernateException e) {
            logger.error("", e);
            containerTraFlag = false;
        }
        return containerTraFlag;
    }

异常被catch到，返回false，然后前台提示“保存失败”；表面上看上去没有问题，但一个处理流程往往不止一个方法操作数据库，异常没被抛出，事务不会回滚，导致被修改或保存的数据出现问题。
修改后的做法为把异常抛出，然后通过统一的异常管理，提示客户保存失败。

• IO流未关闭，或者错误关闭

/**
     * 下载
     * 
     * @date 2015-10-29 下午2:25:39
     * @param request 用户请求
     * @param response 服务器响应
     * @param storeName 服务器上文件名
     * @param contentType 文件类型
     * @param realName 导出文件名
     * @throws IOException 输入流异常
     */
    public static void download(HttpServletRequest request, HttpServletResponse response, String storeName,
            String contentType, String realName) throws IOException {

        BufferedInputStream bis = null;
        BufferedOutputStream bos = null;
        try {
            response.setContentType("text/html;charset=UTF-8");
            request.setCharacterEncoding("UTF-8");

            String ctxPath;
            ctxPath = request.getSession().getServletContext().getRealPath(UPLOADDIR);
            String downLoadPath;
            downLoadPath = ctxPath + FILE_SEPARATOR + storeName;

            long fileLength;
            fileLength = new File(downLoadPath).length();

            response.setContentType(contentType);
            response.setHeader(CONTENT_DISPOSITION, ATTACHMENT
                    + new String(realName.getBytes(GBK), "ISO8859-1"));
            response.setHeader("Content-Length", String.valueOf(fileLength));
            response.setCharacterEncoding(ISO8859);
            bis = new BufferedInputStream(new FileInputStream(downLoadPath));
            bos = new BufferedOutputStream(response.getOutputStream());
            byte[] buff;
            buff = new byte[FILE_SIZE];
            int bytesRead;
            while (-1 != (bytesRead = bis.read(buff, 0, buff.length))) {
                bos.write(buff, 0, bytesRead);
            }

        } catch (FileNotFoundException e) {
            logger.error(storeName + "文件找不到" + e.getMessage(), e);
        } catch (IOException e) {
            logger.error(e.getMessage(), e);
        } finally {
            //关闭流
            if (bis != null) {
                bis.close();
            }
            if (bos != null) {
                bos.close();
            }

        }
    }

• 错误使用finally，使得try中的异常被finally覆盖

 /**
     * 
     * <p>
     * Description: 导入库存盘点记录
     * </p>
     * 
     * @param file 文件
     * @param name 文件名
     * @param request 请求
     * @param response 响应
     * @param checkDesc 盘点描述
     * @param checkDate 盘点日期
     * @throws IOException 异常
     * @throws IllegalStateException 异常
     * @return 返回页面
     */
    @SuppressWarnings({ "finally" })
    @RequestMapping(value = "/importStockCheck.do")
    public ModelAndView importStockCheck(String name, @RequestParam("file") MultipartFile[] file,
            HttpServletRequest request, HttpServletResponse response, @RequestParam("checkDesc") String checkDesc,
            @RequestParam("checkDate") String checkDate) throws IllegalStateException, IOException {
        ModelAndView model;
        model = new ModelAndView();

        String checkFileOutPath;
        checkFileOutPath = null;
        List<List<Object>> inputList;
        inputList = new ArrayList<List<Object>>();
        ReadSheet readSheet;
        readSheet = new ReadSheet();
        InputStream fis;
        fis = null;
        OutputStream toClient;
        toClient = null;
        String realPath;
        realPath = null;
        File fileIn;
        fileIn = null;
        String error;
        error = null;
        // 取得文件名
        SysUser user;
        String fileName = null;
        boolean isDelete;
        isDelete = true; // 是否删除临时模版，默认删除
        //获得当前登录人的信息
        user = getCurrentUser(request);
        logger.info("importStockCheck begin......");
        try {
            for (MultipartFile myfile : file) {
                if (myfile.isEmpty()) {
                    logger.info("file is empty");
                } else {
                    fileName = myfile.getOriginalFilename();
                    // 生成唯一的模版名称，防止多人同时导入共用模版而发生的并发问题
                    fileName = StrUtil.getUniqueFileName(fileName);
                    realPath = request.getSession().getServletContext().getRealPath("/export");
                    //可以使用FileUtils来保存文件，这里不再列出代码
                    FileUtils.copyInputStreamToFile(myfile.getInputStream(),
                            new File(realPath, fileName));
                    fileIn = new File(realPath, fileName);
                    //校验导入条数
                    String rowLimitStr;
                    rowLimitStr = this.dictService.findDictValueByDictKeyAndParentKey("IMPORT_EXP_COUNT", "SYS_CONFIG");
                    if (rowLimitStr == null || "".equals(rowLimitStr.trim()) || "0".equals(rowLimitStr)) {
                        logger.info("请设置导入限制条数字典值");
                        model.addObject("dowlonded", "false");
                        model.addObject("checkFileOutPath", "rowNumDict");
                    } else {
                        int rowNumLimit;
                        rowNumLimit = Integer.parseInt(rowLimitStr);
                        boolean rowNum;
                        rowNum = AbstractValid.checkExcelRowNum(fileIn, rowNumLimit);
                        if (!rowNum) {
                            logger.info("导入数据不能超过" + rowNumLimit + "条");
                            model.addObject("dowlonded", "false");
                            model.addObject("checkFileOutPath", "rowNum");
                            model.addObject("rowNumLimit", rowNumLimit);
                        } else {
                            StockCheckExcel stockCheckExcel;
                            stockCheckExcel = new StockCheckExcel();
                            error = stockCheckExcel.checkFile(fileIn, user);
                            checkFileOutPath = stockCheckExcel.getFileOutPath();
                            if (StockStandardExcel.CONTENT_ERROR_FLAG.equals(error)
                                    || StockStandardExcel.LOCATIONNUM_NOT_EXIST.equals(error)
                                    || StockStandardExcel.EXCEPTION_ERROR_FLAG.equals(error)
                                    || StrUtil.EMPTY_ERROR_FLAG.equals(error)) {
                            } else {
                                //如果不是以上错误正确数据，进行插入操作
                                logger.info("readExcelForData......");
                                inputList = readSheet.readExcelForData(fileIn);
                                //转换并保存数据
                                this.stockCheckService.inputExcel(this.stockCheckService.turnData(inputList), 
                                        user, checkDate, checkDesc, file[0]);
                            }

                        }
                    }
                }
            }
        } catch (IOException e) {
            logger.error("", e);
        } finally {
            logger.info("importStockCheck end......");
            return importFinally(fis, toClient, fileIn, checkFileOutPath, model, error, isDelete);
        }

    }

现象：页面导入Excel，提示成功，但数据库没有记录。
原因：catch仅仅是一个IOException，其他异常均没有catch并写入日志，这样一旦非IOException发生，均会被finally给覆盖，这样看日志是看不出问题的。（发生了一个Excel字段类型转换错误）

• 在service层使用session

//ForecastWeekImpl.java
 /**
     * 零件预测转换需求
     * <p>
     * Description: task
     * </p>
     * 
     * @param versionNumList 版本号集合
     * @param flag 布尔值
     * @param containerVersionNum 料箱版本号
     * @return 版本号集合
     */
    @SuppressWarnings("rawtypes")
    @Override
    public List<String> convertWeek(List<String> versionNumList, boolean flag, String containerVersionNum) {
        List<String> returnVersion;
        returnVersion = new ArrayList<String>();
        IBaseDao baseDao;
        baseDao = (BaseDaoImpl) SpringContextUtil.getBean("baseDao");
        Session session = null;
        Transaction tx = null;
        String minStartWeek = null;
        Map<String, String> loopSizeList;
        try {
            session = baseDao.getCurrentSessionFactory().openSession();
            tx = session.beginTransaction(); //开启事物
            minStartWeek = this.getMinStartWeek(session); //获得最低的起始周数据并都根据今后进行计算
            ......

/**
     * 
     * <p>
     * Description: 获得最小的起始周数据
     * </p>
     * 
     * @param session session对象
     * @return 结果集
     */
    @SuppressWarnings("unchecked")
    private String getMinStartWeek(Session session) {
        SQLQuery query;
        String sql;
        sql = "select min(t.start_week) startWeek from TI_TT_SB2_SUPP_AGGREGATE_DATA t"
                + " where t.version_no in (select max(tts.version_no) versionNum"
                + " from TI_TT_SB2_SUPP_AGGREGATE_DATA tts" + " group by tts.plant)";
        query = session.createSQLQuery(sql).addScalar("startWeek", StandardBasicTypes.STRING);
        List<String> list;
        list = new ArrayList<String>();
        list = query.list();
        if (list.isEmpty()) {
            return null;
        } else {
            return list.get(0);
        }
    }

• 在DAO层处理业务

//SupplierInverstDaoImpl.java
  @Override
    public void traInvestStock(AssetStock assetStock, Stock stock, SupplierInverstDetail invDetail,
            SupplierInverst model, SysUser user) {
        AssetStock assetSideStock;
        String type;
        Long quantity;
        quantity = invDetail.getQuantity();
        type = model.getType();
        //若类型为调整-，库存减
        if ("3".equals(type)) {
            quantity = 0 - invDetail.getQuantity();
        }
        assetSideStock = this.findAssetStock(assetStock);
        if (assetSideStock != null) {
            assetSideStock.setQuantity(assetSideStock.getQuantity() + quantity);
            assetSideStock.setUpdateDate(new Date());
            assetSideStock.setUpdateUser(user.getEmployeeNum());
            try {
                this.assetStockDao.saveOrUpdate(assetSideStock, user);
            } catch (StaleObjectStateException e) {
                // 乐观锁异常，提示数据已被修改，请重新提交
                throw new BusinessException("资产方库存数据已被修改，请重新提交。");
            }
        } else {
            assetStock.setDelFlag(StrUtil.NOT_DEL);
            assetStock.setCreateDate(new Date());
            assetStock.setAssetNum(model.getAssetNum());
            assetStock.setPlant(model.getLocation().getPlant());
            assetStock.setContainerNum(invDetail.getContainerNum());
            assetStock.setCreateUser(user.getEmployeeNum());
            assetStock.setQuantity(0 + quantity);
            try {
                this.assetStockDao.saveOrUpdate(assetStock, user);
            } catch (ConstraintViolationException e) {
                // 唯一性约束异常
                throw new BusinessException("唯一性约束异常。");
            }
        }
        Stock sto;
        sto = this.findStock(stock);
        if (sto != null) {
            sto.setStock(sto.getStock() + quantity);
            sto.setUpdateDate(new Date());
            sto.setUpdateUser(user.getEmployeeNum());
            try {
                this.stockDao.saveOrUpdate(sto, user);
            } catch (StaleObjectStateException e) {
                // 乐观锁异常，提示数据已经被修改，请重新提交
                throw new BusinessException("工位器具库存数据已被修改，请重新提交。"); 
            }
        } else {
            stock.setDelFlag(StrUtil.NOT_DEL);
            stock.setCreateDate(new Date());
            stock.setLocationNum(model.getLocationNum());
            stock.setContainerNum(invDetail.getContainerNum());
            stock.setCreateUser(user.getEmployeeNum());
            stock.setStock(0 + quantity);
            try {
                this.stockDao.saveOrUpdate(stock, user);
            } catch (ConstraintViolationException e) {
                // 唯一性约束异常，提示业务主键冲突
                throw new BusinessException("唯一性约束异常，提示业务主键冲突。");
            }
        }
    }

破坏了MVC结构，使代码不容易理解，同时有可能造成一些bug。

• 在Controller层中处理业务

    //ContainerCmcController.java
    /**
     * <p>
     * Description: 添加工位器CMC 生成发箱记录并打印
     * </p>
     * @param traDateStr 发出日期
     * @param ifPlantHidden 分工厂产权
     * @param containerTransmit 工位器具
     * @param request 请求
     * @param response 响应
     * @throws Exception 异常
     * @return 结果
     */
    @RequestMapping(value = "/toCmcSendAll.do")
    @ResponseBody
    public Object toCmcSendAll(@RequestParam(value = "traDateStr") String traDateStr,
            @RequestParam(value = "ifPlantHidden") String ifPlantHidden,
            @ModelAttribute ContainerTransmit containerTransmit, 
            HttpServletRequest request,
            HttpServletResponse response) throws Exception {

      ...

      //执行添加或更新操作
      this.containerCmcService.saveCmcSend...
      //生成pdf并打印
      this.containerCmcService.createCmcPdf...
      ......    

    }

因为controller是没有事务保护的，所以调用的每个service方法执行完毕后都会立即提交事务，一旦一个service方法出现异常，已经执行过的方法也不会回滚。

• Controller、Dao中使用全局变量
  在Struts2的action中，可以写全局变量,因为action是多实例的；而springMVC默认是单实例的。dao属于spring bean，默认scope（bean作用域）是singleton

@Controller
@RequestMapping(value = "/user")
public class UserController extends BaseController<SysUser> {
    /**
     * userService
     */
    @Autowired
    @Qualifier("userService")
    private IUserService userService;
//    /**
//     * 用户查询条件
//     */
//    private SysUser filter;

//    /**
//     * viewModel
//     */
//    private ModelAndView viewModel;
......

@Repository("userDao")
public class UserDaoImpl extends BaseDaoImpl<SysUser, Serializable> implements IUserDao {
    /** 日志 **/
    private static Logger logger = Logger.getLogger(UserDaoImpl.class);

//    /**
//     * 用于查询的对象
//     */
//    private Query query;

    /** likes 查询使用 % **/
    private final String likes = "%";
    ......