[BUAA_SE_2017]代码复审-Week2

代码复审

CheckList

1.概要部分代码能符合需求和规格说明么?符合,经过-c及-s合法参数测试,程序均能生成、求解相应数独。
代码设计是否有周全的考虑?对于非法输入,程序处理不够周全。
-c 100000000 电脑死机无法动弹
-c -1 依旧会生成一个数独至Sudoku.txt中
-s 非法内容文件 程序错误退出
代码可读性如何?通过清晰的变量名称以及明确分工的函数,代码可读性非常高。
代码容易维护吗?代码容易维护,函数分工相当明确、逻辑清晰,因此函数之间耦合度不高,维护难度也不高。
代码的每一行都执行并检查过了吗?程序中几乎没有冗余的代码,在输入合法以及一些不合法的情况下,代码每一行基本都执行到。
2.设计规范部分设计是否遵从已知的设计模式或项目中常用的模式?没有用到设计模式,运用了面向对象的思想(虽然就一个类),充分实现了功能。
有没有硬编码或字符串/数字等存在?存在,设置为局部常量,用以判断某格数字合法性。 const int complete = 0x3fe;
代码有没有依赖于某一平台,是否会影响将来的移植(如Win32到Win64)?个人感觉并不影响:程序中include的都是C++ 标准库头文件,对移植无影响。
#include "SudokuSolver.h"
#include <cstdio>
#include <utility>
#include <cassert>
#include <ctime>
#include <iostream>
#include <cstdlib>
开发者新写的代码能否用已有的Library/SDK/Framework中的功能实现?在本项目中是否存在类似的功能可以调用而不用全部重新实现?不能,程序中的函数分工已经非常明确,并且各处已最大化利用已有函数,因此代码冗余少。
有没有无用的代码可以清除?(很多人想保留尽可能多的代码,因为以后可能会用上,这样导致程序文件中有很多注释掉的代码,这些代码都可以删除,因为源代码控制已经保存了原来的老代码。)程序设计者再最后一次commit前自我进行了一次冗余代码清楚的工作,因此程序中不存在大段注释掉的代码,但有两行被优化被注释了的代码存在:
std::string SudokuSolver::generateN(int n, SudokuBoard &board)
{
    std::string r;
    //for (int i = 0; i < n; i++)
        //r += generate(board).toString() + ' ';
    _solveCount = 0;
    _solveLimit = n;
    solutions = new std::vector>();
    generate(board);
    for (auto& s : *solutions)
        r += *s + ' ';
    delete solutions;
    return r;
}
3.代码规范部分修改的部分符合代码标准和风格么(详细条文略)?代码风格整体一致,一目了然,较为清晰。但是for循环或if语句之后存在不加花括号的情况,与其它多行for或if有一些矛盾,有待改进。
4.具体代码部分有没有对错误进行处理?对于调用的外部函数,是否检查了返回值或处理了异常?进行了错误处理:
try
{
    n = std::stoi(arg);
    printf("%d ", n);
}
catch (...)
{
    printf("invalid argument %s", arg);
    return 0;
}

对外部调用函数,也检查了返回值:如strcmp并判断返回值与0的大小关系、fopen并判断返回值是否为NULL等。
参数传递有无错误,字符串的长度是字节的长度还是字符(可能是单/双字节)的长度,是以0开始计数还是以1开始计数?字符串均是char*来实现,无误。
边界条件是如何处理的?Switch语句的Default是如何处理的?循环有没有可能出现死循环?除了遍历数独边界条件设置9*9之外,其它遍历方式均采用c++的for(:)遍历模式,且循环体中不会更改循环条件,因此不会产生死循环。
有没有使用断言(Assert)来保证我们认为不变的条件真的满足?仅在单元测试中使用了assert来进行判断。
对资源的利用,是在哪里申请,在哪里释放的?有没有可能导致资源泄露(内存、文件、各种GUI资源、数据库访问的连接,等等)?有没有可能优化?一个较典型的例子:
for (auto p : puzzles)
{
    SudokuBoard *s = solver.solve(*p);
    fprintf(output, "%s ", (*(s->toString())).c_str());
    delete s;
}

资源需要时申请,无需时释放做的较好;但是由于在-c情况下对第二个参数的判断不足,导致_solveLimit变量可以非常大,驻留在内存中的char过多,导致了占用完所有内存(电脑就这么挂了)。可以通过设置阈值来阶段性输出,释放内存空间。
数据结构中是否有无用的元素?没有,整体思路采用二进制来实现,没有冗余元素。
5.效能代码的效能(Performance)如何?最坏的情况是怎样的?因为该程序生成与求解采用同一套算法,经过优化剪枝后,该回溯算法效能表现较好,但是求解的时间还是较长,原因在于每一次生成只需要回溯即可产生不同解,而每一次求解需要重新遍历才能求出答案,当数独仅存在唯一解时(最坏情况),耗时最长。
代码中,特别是循环中是否有明显可优化的部分(C++中反复创建类,C#中string的操作是否能用StringBuilder 来优化)?该程序由c++实现,存在反复创建类的情况,但是程序运行分析中该创建时间耗时并不显著,无须优化。(在此提出一点,优化策略是每次寻找可行最小的那个格子进行填充,但每次寻找需要遍历整个数独,或许在这上面耗时过多?)
对于系统和网络调用是否会超时?如何处理?不会超时,程序仅对文件进行读写,并判断了文件结束符,因此不会;不存在网络调用。
6.可读性代码可读性如何?有没有足够的注释?代码可读性强:变量、函数命名准确;代码风格总体一致,逻辑清晰有序;按嵌套使用缩进,容易阅读、修改;关键地方有注释,言简意赅。
7.可测试性代码是否需要更新或创建新的单元测试?需要进行更新单元测试,用以检验命令行参数的合法性,避免无效输入或无效文件格式。
还可以有针对特定领域开发(如数据库、网页、多线程等)的核查表。无。

设计代码规范

个人项目使用C#语言开发,因此选用StyleCop来进行检查。

  • 工具提供的代码规范和你个人的代码风格有什么不同?

    • 所有方法名称需以大写开头;
    • 判断语句各部分需加上括号;
    • 注释语句需加在每行一个空格后,而我认为在哪里都可以,只要便于提示;
  • 工具提供的代码规范里有哪些部分是你之前没有想到的?

    • private成员变量要全部定义在public成员变量之后;
    • 所有public成员变量必须定义为private;
    • The variable name 'sHeads' begins with a prefix that looks like Hungarian notation;
    • if、for、while与左括号之间需加一个space;
    • Statements or elements wrapped in curly brackets must be followed by a blank line;
  • 为什么要这样规范?这样的规范有意义吗?

    • 通过StyleCop进行了检查,每一个.cs文件几乎都能发现80+处不符合代码规范的地方,大体分为这么几类:
      • Spacing:运算符、关键字前后一空格,借此可以将其完全区分出来,提高可读性;
      • Layout:花括号换行问题,尚不清楚为何右花括号需要换行后多空一行;
      • Naming:不使用匈牙利命名法;
      • Readability:成员变量前加上this;
      • Maintainability:将类成员变量定义为private;使用小括号清晰界定运算符优先级。便于维护。
      • Ordering & Documentation:涉及到调用外部库的using顺序;需添加文档。
    • 这些规范是有意义的。从程序可读性角度来说,关键字、运算符以及花括号的使用规范,很大程度上决定了代码的格式,这些要求是必须遵守的;其次,从程序可维护的角度来看,代码的书写要尽可能提升逻辑表达的清晰度,即一目了然,无须再根据代码进行人为的第二次理解或计算(容易造成错误,使得debug时又衍生出更多bug);最后,从文档角度来说,代码规范不仅体现在代码中,对代码功能的说明更加重要,因此文档也是代码规范一个不可或缺的部分。
  • 代码规范:

    • Spacing, Layout, Readability, Maintainability 以及 Ordering & Documentation是必须严格遵守的,但是对于Naming而言,究竟使用匈牙利命名法还是其它命名法,有待深究。对于C#来说,每一个变量类型都是需要定义的,使用匈牙利命名法或许显得多余;但对于其它语言来说,可能变量类型不需要定义…… Naming应该根据项目的不同来进行规定。
原文地址:https://www.cnblogs.com/whynotRW/p/7620290.html