个人作业Week2-代码复审
一、代码复审Check List
1.概要部分
- 代码能符合需求和规格说明么？
1)规格部分
输出规格：
用一个文本文件（假设名字叫 sudoku.txt）的形式保存起来，每次生成的txt文件需要覆盖上次生成的txt文件，文件内的格式如下，数与数之间由空格分开，终局与终局之间空一行，行末无空格
-s 实际输出：
4 7 8 9 1 6 2 5 3 1 5 9 8 2 3 7 6 4 2 6 3 5 4 7 8 9 1 7 4 6 1 3 5 9 8 2 8 9 1 4 6 2 5 3 7 5 3 2 7 8 9 1 4 6 3 8 7 6 5 1 4 2 9 6 1 5 2 9 4 3 7 8 9 2 4 3 7 8 6 1 5 4 7 8 9 1 6 2 5 3 1 5 9 8 2 3 7 6 4 2 6 3 5 4 7 8 9 1 7 4 6 1 3 5 9 8 2 8 9 1 4 6 2 5 3 7 5 3 2 7 8 9 1 4 6 3 8 7 6 5 1 4 2 9 6 1 5 2 9 4 3 7 8 9 2 4 3 7 8 6 1 5 4 7 8 9 1 6 2 5 3 1 5 9 8 2 3 7 6 4 2 6 3 5 4 7 8 9 1 7 4 6 1 3 5 9 8 2 8 9 1 4 6 2 5 3 7 5 3 2 7 8 9 1 4 6 3 8 7 6 5 1 4 2 9 6 1 5 2 9 4 3 7 8 9 2 4 3 7 8 6 1 5 ...
-c 实际输出：
3 1 2 7 8 9 4 5 6 4 5 6 3 1 2 7 8 9 7 8 9 4 5 6 3 1 2 2 3 1 9 7 8 6 4 5 6 4 5 2 3 1 9 7 8 9 7 8 6 4 5 2 3 1 1 2 3 8 9 7 5 6 4 5 6 4 1 2 3 8 9 7 8 9 7 5 6 4 1 2 3 3 1 2 7 9 8 4 5 6 4 5 6 3 1 2 7 9 8 7 9 8 4 5 6 3 1 2 2 3 1 8 7 9 6 4 5 6 4 5 2 3 1 8 7 9 8 7 9 6 4 5 2 3 1 1 2 3 9 8 7 5 6 4 5 6 4 1 2 3 9 8 7 9 8 7 5 6 4 1 2 3 3 1 2 8 7 9 4 5 6 4 5 6 3 1 2 8 7 9 8 7 9 4 5 6 3 1 2 2 3 1 9 8 7 6 4 5 6 4 5 2 3 1 9 8 7 9 8 7 6 4 5 2 3 1 1 2 3 7 9 8 5 6 4 5 6 4 1 2 3 7 9 8 7 9 8 5 6 4 1 2 3 ...
2) 输入规格：
对于命令：
a. -c 100，结果如上
b. -c 0,
number should be positive
c. -c abc
invalid number
d. -c
invalid parameter number
e. -s XXXX\liuchang_review\Sudoku\subject.txt
输入内容：
4 7 0 9 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 8 0 1 0 0 6 0 0 0 0 0 0 8 0 1 0 0 2 5 0 0 0 0 0 7 0 0 0 4 0 0 0 0 0 5 1 0 0 0 6 0 0 0 0 0 0 7 0 0 0 0 0 0 8 0 0 0 4 7 0 9 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 8 0 1 0 0 6 0 0 0 0 0 0 8 0 1 0 0 2 5 0 0 0 0 0 7 0 0 0 4 0 0 0 0 0 5 1 0 0 0 6 0 0 0 0 0 0 7 0 0 0 0 0 0 8 0 0 0 ...
输出如上提到。
f. -s 0
number should be positive
e. -s abc
invalid number
f. -s
invalid parameter number
g. 无参数
invalid parameter number
h. -abc
unknown function
总体评价：
作为一个程序，能在不崩溃的前提下，处理正确的输入并输出符合规范的结果，处理异常的输入并返回报错信息，规格实现非常完善。
2) 需求部分：
1. 生成数独：
- 生成不重复的数独终局至文件
- 读取文件内的数独问题，求解并将结果输出到文件
- 在生成数独矩阵时，左上角的第一个数为：（学号后两位相加）% 9 + 1。例如学生A学号后2位是80，则该数字为（8+0）% 9 + 1 = 9，那么生成的数独棋盘应如下（x表示满足数独规则的任意数字）
1. 格式和不重复性：
运行 -c 100000，使用字典树存储和查重：
COUNT = 100000 class TrieNode:
"""A Trie tree specialized to store strings of sudoku solutions.""" desc = {} char = "" def __init__(self): self.desc = {} self.char = "" def insert(self, string):
"""
Each time we come to or create a lower node, directed by the first character of 'string'. If len(string) is 1, we assert the node does not exist.
""" if len(string) == 0: return elif len(string) == 1: if len(self.desc) > 0 and string[0] in self.desc: print("Same sudokus.") input() if string[0] not in self.desc: new_node = TrieNode() #print(new_node) self.desc[string[0]] = new_node new_node.char = string[0] self.desc[string[0]].insert(string[1:]) def display(self): for key, node in self.desc.items(): node.display() class Reader: pos = 0 f = None line = 1 def __init__(self, f): self.f = f def match(self, expect="\n"): c = self.f.read(1) if c == "\n": self.line += 1 if c == expect: return c, True else: return c, False if __name__ == "__main__": f_in = open(r"XXXX\liuchang_review\Sudoku\Sudoku\sudoku.txt", "r") reader = Reader(f_in) root = TrieNode() for sudoku_count in range(COUNT): if sudoku_count%10000 == 0: print(sudoku_count) sudoku_str = "" for j in range(9): #line for k in range(9): char, v = reader.match() sudoku_str += char if k != 8: char, v = reader.match(" ") else: char, v = reader.match("\n") if not v: print("PE in line "+str(reader.line)) if sudoku_count != COUNT-1: char, v = reader.match("\n") if not v: print("PE in line " + str(reader.line)) assert(sudoku_str[0] == ) root.insert(sudoku_str)
结果：未报重复错误和格式错误。
2. 正确性：
调用check_validity()函数在输出前检查每一行每一列每一宫是否填满1-9。
bool check_validity(int s[9][9]) const{ int record = 0; const int mask = 0x1ff; // 9 ones for (int group_x = 0; group_x < 3; group_x++) { for (int group_y = 0; group_y < 3; group_y++) { record = 0; for (int x = 3 * group_x; x < 3 * group_x + 3; x++) { for (int y = 3 * group_y; y < 3 * group_y + 3; y++) { int v = s[x][y]; record |= 1 << (v-1); } } if (record != mask) { return false; } } } for (int line = 1; line <= 9; line++) { record = 0; for (int row = 1; row <= 9; row++) { int v = s[line][row]; record |= 1 << (v - 1); } if (record != mask) { return false; } } for (int row = 1; row <= 9; row++) { record = 0; for (int line = 1; line <= 9; line++) { int v = s[line][row]; record |= 1 << (v - 1); } if (record != mask) { return false; } } return true; }
结果：
全部正确。
- 代码设计是否有周全的考虑？
如上，针对正确和错误的输入进行了测试，程序保持了鲁棒性，说明有周全的考虑
- 代码可读性如何？
一般。
1) 名字的选择：
举例说明：
a) -c模式中，用于量产数独的模板"Templet"。首先，拼写错误，应该是"Template"；第二，什么的template? 我可能会建议叫create_template或者gen(eration)_template。
b) “Templet” 中的函数 fill_line1, fill_line2, fill_line3，大概都是一个建立第一个模板的函数，里面硬编码了模板的一个宫，然后用fill_line去填写整个的模板。fill_line这个名字不知所云，也许改成"build_grip_template”好一点？
c) Template_sudoku 中的成员code。这个是一个全排列变化的自变量，一个长度为9的字符串，叫做code是否不知所云？可能叫seed会好点？
2) 代码的存放：
按照我理解的C++的规范，类的声明应该放在同名的.h中，类的（复杂）方法应该放在对应的同名.cpp中，然而本代码中所有类的声明都放在了Sudoku.h中，定义放在了Sudoku.cpp和puzzle_creator.cpp中，导致浏览非常不方便，需要经常上下滚动查找定义。
3) 代码的结构：
1.生成模式：
面向过程和面向对象的风格夹杂，文件io和算法执行夹杂，导致封装性非常不好。
代码流程解析：
main
读取-c和数字
调用create_sudoku
打开文件
Template_sudoku* tsudo = new Template_sudoku();
Templet* templet = new Templet();
fill_line1();
fill_line2();
fill_line3();
code进行全排列，填充模板
我们可以看到，假如硬编码需要程序运行时生成，是否应该把fill_line1这些函数放在create_sudoku中，把重要的过程放在调用的外层？假如硬编码不需要运行时生成，那么为什么不直接在编译时嵌入？
打开文件和写入文件这类与算法无关的行为，是否应该换个地方来写？打开文件适合放在main中，写入文件应该用专用函数来完成，这类似于“系统调用”的思想。
读入参数和数字这类trivial而比较烦的算法，应该用函数封装。
2. 解数独模式：
main
读入 -s 和文件地址
solve
在写入模式打开"sudoku.txt"
sudoku = new Subject_sudoku(code)
建立9个Group，分别代表行、列、宫。
根据题目把点的值输入到Group中
初始化这些Group
调用fill_sudoku()
获得当前解数量最少的框
去尝试：guess_value()
递归调用fill_sudoku
solve这块的可读性好很多，层次分明，对象初始化函数做了应该做的事情，然后fill_sudoku通过guess_value递归填写数独，逻辑比较清晰，代码简介。命名也比较明晰，比如Group明显代表格子的组，Box虽然可能不够地道（叫digit可能好点）。
4）注释
建议在比较重要的函数开头，把用的算法写出来。代码中有零星的注释，但是感觉作用不是给读者看而是给写程序的人整理思路的。
- 代码容易维护么？
可维护性不是很好。由于读入文件和输出文件都夹在在算法里，所以我们在封装的时候不得不再理解一遍算法才能知道应该把数组格式的输入和输出安排在算法的哪里才能不发生错误。这说明模块之间的独立性差。硬编码的模板信息和用户交互信息散落在函数的各个地方，导致难以查询和修改。
- 代码的每一行都执行并检查过了吗？
作者个人进行了严格的单元测试，对于-c模式进行了1-1000,000的测试，并且检查了数独的合法性以及数独的不重复性。
OpenCppCoverage测试显示代码覆盖率100%。
2. 设计规范部分
- 设计是否遵从已知的设计模式或项目中常用的模式？
C++类设计规范：http://blog.csdn.net/k346k346/article/details/49445137
1. 将类的定义放在头文件中实现。
基本满足，所有类的定义都在Sudoku.h中，但是不同的类没有分文件存放。
2. 尽量将数据成员申明为私有。
不满足。
class Templet { public: string line[9][3]; // line[block][row] const string line1_position_code = "012"; // not change string line2_position_code = "345"; // 3! = 6 types string line3_position_code = "678"; // 3! = 6 types Templet(); void fill_line(const string linetemp[], const string code, const int blockbegin); Templet* fill_line1(); Templet* fill_line2(); Templet* fill_line3(); bool change2next(); void show(); };
3.将成员函数放到类外定义。
满足。
- 有没有硬编码或字符串/数字等存在？
存在。
Templet* Templet::fill_line1() { const string linetemp[3] = { "012", "345", "678" }; fill_line(linetemp, line1_position_code, 0); return this; }
- 代码有没有依赖于某一平台，是否会影响将来的移植（如Win32到Win64）？
不依赖。代码只调用了<iostream>, <vector>等c++标准库，而c++具有跨平台性，因此不会影响移植。
- 开发者新写的代码能否用已有的Library/SDK/Framework中的功能实现？在本项目中是否存在类似的功能可以调用而不用全部重新实现？
不能。部分功能可以调用实现。
for (int i = 0; (c = para[i]) != '\0'; i++) { if (isdigit(c)) { number *= 10; number += c - '0'; if (number > 1'000'000) { cout << "the number is out of range"; return 0; } } else { cout << "invalid number"; return 0; } }
这一段的读入数字完全可以用标准C库的sscanf实现。
- 有没有无用的代码可以清除？（很多人想保留尽可能多的代码，因为以后可能会用上，这样导致程序文件中有很多注释掉的代码，这些代码都可以删除，因为源代码控制已经保存了原来的老代码。）
有。
string get_puzzle() { srand((int)time(0)); // set seed do { /* initial */ number_code = ""; for (int i = 0; i < SIZE * SIZE; i++) { number_code += '0'; } /* set numbers */ set_number_randomly(); /* find solution */ } while (create_final_sudoku()); /* create puzzle */ create_puzzle(); return number_code; } void set_number_randomly() { int position; for (int i = 0; i < SIZE; i++) { // randomly fill 1~9 position = rand() % (SIZE * SIZE); number_code[position] = i + '1'; } }
Templet::show()
3.代码规范部分
- 修改的部分符合代码标准和风格么（详细条文略）?
总体而言风格是统一的，问题上面已经指出了。
4.具体代码部分
- 有没有对错误进行处理？对于调用的外部函数，是否检查了返回值或处理了异常？
对命令行的错误进行了完备的处理，如上。对于调用的外部函数则不完备。主要的调用函数有fopen, fclose, 但部分地方对异常没有处理。
int solve_sudoku(FILE* subject) { Subject_sudoku* sudoku; string code = ""; int number_counter = 0; char c; FILE* fout; fout = fopen("sudoku.txt", "w"); while ((c = fgetc(subject)) != EOF) { if (isdigit(c)) { code += c; number_counter++; } if (number_counter == SIZE * SIZE) { number_counter = 0; sudoku = new Subject_sudoku(code); if (!fill_sudoku(sudoku, fout)) { cout << "no solutions" << endl; }; delete(sudoku); code = ""; } } fclose(fout); return 0; }
缺少对fopen异常的处理，另外根据warning，应该用fopen_s而不是fopen。
- 参数传递有无错误，字符串的长度是字节的长度还是字符（可能是单/双字节）的长度，是以0开始计数还是以1开始计数？
无错误。返回的是字节的长度，但由于只涉及ascii码的问题，不会有问题。从0开始计数。
- 边界条件是如何处理的？Switch语句的Default是如何处理的？循环有没有可能出现死循环？
· 边界条件在于输入的-c 0，这种情况作为异常来处理。并没有出现Switch语句。
所有的for都是用for (int i=0;i<N;i++)风格书写的，并且不出意外不改变i的值，因此不可能出现死循环。
while部分：
while (tsudo->change2next() && ++counter < number) { buffer[index++] = '\n'; tsudo->record(fout, &index, buffer); }
其中change2next是产生全排列的函数，如果已经排完则返回zero，而counter只在这里被改变，因此这里的while不会死循环。
递归：
bool guess_value(Box* box, Subject_sudoku* sudoku, FILE* fout) { //cout << "guess" << endl; int rowno = box->row->number; int columnno = box->column->number; for (int i = 0; i < SIZE; i++) { if (box->posvalue & get_valuebit(i)) { // -- value i+1 is possible //Subject_sudoku* new_sudoku = new Subject_sudoku(*sudoku); Box* box = sudoku->getbox(rowno, columnno); int members_posvalues[3][9]; int posvalue = box->make_certain(i + 1, members_posvalues); if (fill_sudoku(sudoku, fout)) { return true; } //delete(new_sudoku); box->cancel_certain(posvalue, members_posvalues); } } return false; } bool fill_sudoku(Subject_sudoku* sudoku, FILE* fout) { // -- succeed(true) or failed(false) Box* box; box = sudoku->get_minpos_box(); //cout << sudoku->to_string() << endl; if (box == NULL) { sudoku->show(fout); return true; } return guess_value(box, sudoku, fout); }
如果产生死循环，则fill_sudoku会不断调用guess_value，并且guess_value也会继续调用fill_sudoku。前者的条件是box != NULL，但是fill_sudoku每向下调用一层，都必然用guess_value进行填数，因此带空的box会变少直至0，这时fill_sudoku就不会通过guess_value调用自身了。
- 有没有使用断言（Assert）来保证我们认为不变的条件真的满足？
没有。最好在测试阶段assert一下以便debug。
- 对资源的利用，是在哪里申请，在哪里释放的？有没有可能导致资源泄露（内存、文件、各种GUI资源、数据库访问的连接，等等）？有没有可能优化？
1. new：
//int solve_sudoku(FILE* subject); sudoku = new Subject_sudoku(code);
delete: fill_sudoku返回后。
不会泄露。
2. new:
Template_sudoku* tsudo = new Template_sudoku();
delete: 与本人确定，忘记释放了。但是这个new在一次运行中只运行一次，随进程结束空间由操作系统释放。但是记得delete是不会错的。
3. new:
for (int i = 0; i < SIZE; i++) { rows[i] = new Group(i); columns[i] = new Group(i); blocks[i] = new Group(i); }
delete: Subject_sudoku的析构函数中释放。
总体来说，这些new从当前应用看来都不会造成内存泄漏，但是2如果反复执行会有风险。
优化：实际上，三者的占用最大空间都是已知的，所以new是不必要的。前两者可以直接在栈空间中存储，第三个可以在堆空间中开辟一片大空间，避免反复new降低效率。
- 数据结构中是否有无用的元素？
没有。
5. 效能
根据助教的评测结果：
1. -c
| INDEX | NumberID | -c 1 | -c 5 | -c 100 | -c 500 | -c 1000 | -c 50000 | -c 1000000 |
|---|---|---|---|---|---|---|---|---|
| 1 | 15061183 | 0.149 | 0.046 | 0.052 | 0.038 | 0.038 | 0.096 | 1.347 |
因为采用了硬编码和系统内置的排序算法生成数独，排名非常靠前，生成的效率很优秀。
2. -s
| Index | NumberID | -s 1.txt | -s 5.txt | -s 100.txt | -s 500.txt | -s 1000.txt | -s 50000.txt | -s 1000000.txt |
|---|---|---|---|---|---|---|---|---|
| 1 | 15061183 | 0.048 | 0.05 | 0.07 | 0.07 | 0.078 | 2.099 | 39.193 |
排名同样非常靠前，但是笔者进行测试的时候发现，如果是如下的数独：
4 7 0 9 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 8 0 1 0 0 6 0 0 0 0 0 0 8 0 1 0 0 2 5 0 0 0 0 0 7 0 0 0 4 0 0 0 0 0 5 1 0 0 0 6 0 0 0 0 0 0 7 0 0 0 0 0 0 8 0 0 0
测试结果：
作者采用了回溯的解法，通过递归调用和快速判断可能解，效率很高，但是在面对17字数独这种回溯比较深的数独时，就比较慢了。
6.可读性
前面已经提到，可读性不够好，建议针对函数的功能进行注释，同时注意文本存放和变量取名的合理性。
7.可测试性
作者针对-c的各种数量和-s进行了单元测试，但是这样的功能测试粒度可能不够精细。
应该针对各个对象的方法进行测试，保证对象和对象方法的可靠性。比如对于Group可以进行填数测试，看它是否能返回期望的可能解。
二、设计一个代码规范
1. 风格对比
用cpp_lint修正程序，风格对比：
1.cpp_lint : 添加版权信息
我：不添加版权信息
2. cpp_lint: 使用using-declaration
我：使用using-directive
3. cpp_lint: { 不换行
我：花括弧偶尔换行
4. cpp_lint: 用四个空格
我：用tab
5. cpp_lint: //和comment中间有空格
我：没有空格
6. cpp_lint: 一行不超过80字符
我：一行VS工作区显示得下就行
7. cpp_lint: 删除行尾空格
我：没注意这个
8. cpp_lint: 推荐使用reinterpret_cast
我：只知道c-style的强制类型转换
2. 工具提供的代码规范里有哪些部分是你之前没有想到的？
1. 版权信息很重要，不论开源与否，自己写的代码应该署名
2. using-declaration 可以减少编译器负担，应该使用
3. 应该统一成不换行的
4. tab 在不同文本编辑器上显示效果不一，八个空格非常臃肿，应该用四个空格
5. // 和comment中间加空格让代码更加清爽
6. 不超过80个字符可以让代码在一些控制台上一行完整显示
7. 行尾空格偶尔不小心加一个不重要吧？
8. 既然是c++就应该有c++的风格，原来c的强制类型转换可读性不是很好，用c++的会比较容易看懂。
3. 希望使用的代码规范：
1.·文件版权署名
2. 不要用 using namespace std; 用 using std::xxx;
3. { 不换行
4. 类名以外的标识符用小写 xxx_xxx 的风格，单词尽量不缩写，难以区分的地方用_, 类名用开头大写的驼峰法
5. 类声明增加注释，成员声明处增加注释，带有算法的函数增加注释
6. 用 4 个空格。
