劣质代码评析——猜数字问题（上）
【题目】
猜数字（又称 Bulls and Cows ）是一种大概于20世纪中期兴起于英国的益智类小游戏。一般由两个人玩，也可以由一个人和电脑玩，在纸上、在网上都可以玩。这种游戏规则简单，但可以考验人的严谨和耐心，而这也正是程序员所需要的优秀品质。
标准规则如下：
通常由两个人玩，一方出数字，一方猜。出数字的人要想好一个没有重复数字的4位数，不能让猜的人知道。猜的人就可以开始猜。每猜一个数字，出数者就要根据这个数字给出几A几B，其中A前面的数字表示位置正确的数的个数，而B前的数字表示数字正确而位置不对的数的个数。如正确答案为 5234，而猜的人猜 5346，则是1A2B，其中有一个5的位置对了，记为1A，而3和4这两个数字对了，而位置没对，因此记为 2B，合起来就是1A2B。接着猜的人再根据出题者的几A几B继续猜，直到猜中（即4A0B）为止。
【样本】
View Code
1. #include <stdio.h> 2. // 引入随机函数srand()、rand()所在的头文件 3. #include <stdlib.h> 4. // 引入时间函数time()所在的头文件 5. #include <time.h> 6. // 引入字符查找函数strchr()所在的头文件 7. #include <string.h> 8. int main() 9. { 10. // 定义保存目标数字的字符数组 11. char bull[5] = ""; 12. srand((int)time(0)); 13. // 利用rand()函数产生一个四位随机数， 14. // 并利用sprintf()函数将其转换成字符串，保存在bull字符数组中 15. sprintf(bull, 16. "%d", rand()%10000); 17. // 定义表示猜测数字的字符数组 18. char cow[5] = ""; 19. // 定义表示猜测状况的A和B 20. int A = 0; 21. int B = 0; 22. // 猜测次数，最开始是第一次 23. int count = 1; 24. // 构造一个循环结构，只要没有完全猜对， 25. // 就继续下一次猜测 26. while(4!=A) 27. { 28. // 判断是否超过次数限制 29. // 最多猜测10次 30. if(count > 10) 31. { 32. puts("sorry,you lost. :"); 33. // 跳出猜测循环，结束游戏 34. break; 35. } 36. // 输出当前次数 37. printf("%d :",count); 38. // 获取用户输入的猜测数字 39. // 因为需要与目标数字进行比对， 40. // 所以将输入作为字符串（%s），而不是作为一个整数（%d） 41. scanf("%s",cow); 42. // 采用循环结构，逐个字符比对输入数字（cow）和目标数字（bull） 43. for(int i = 0; i<4; ++i) 44. { 45. // 判断当前字符是否相同 46. if(bull[i] == cow[i] ) 47. { 48. // 如果相同，则表示数字正确且位置正确， 49. // A的值增加1 50. ++A; 51. } 52. else // 否则，判断当前数字是否是正确数字 53. { 54. // 在目标字符串中查找当前字符 55. char* p = strchr( bull, cow[i]); 56. // 如果p不是一个空指针（C语言中用NULL或者0表示）， 57. // 则表示目标字符串中有这个字符，当前字符是正确数字， 58. // B的值增加1 59. if(0 != p) 60. ++B; 61. } 62. } 63. // 输出此次猜测的结果 64. printf("%s : %dA%dB\n",cow,A,B); 65. // 如果完全猜测正确，则输出最终结果，结束游戏 66. if(4 == A) 67. { 68. printf("you win! the bull is %s.",bull); 69. break; 70. } 71. // 否则，开始下一次猜测，次数增加1，A和B数据归零 72. ++count; 73. A = 0; 74. B = 0; 75. } 76. return 0; 77. }
——陈良乔 ，《C程序设计伴侣》，人民邮电出版社，2012年10月，p39~42
【评析】
对于初学者来说，这个题目有一定难度和挑战性。但也正因为如此，从中更能发现初学者常犯的一些毛病。
从整体上看，整个源程序只有一个main()函数，没有其他的自定义函数。这是初学者常见的一个毛病——一main()到底。(参见拙著《品悟C》 第7章 问题4 <将main()函数进行到底>，P253)
View Code
1. #include <stdio.h> 2. // 引入随机函数srand()、rand()所在的头文件 3. #include <stdlib.h> 4. // 引入时间函数time()所在的头文件 5. #include <time.h> 6. // 引入字符查找函数strchr()所在的头文件 7. #include <string.h>
这种注释风格有两个特点：第一丑陋凌乱；第二浪费铺张，是一种把散文“膨化”成诗歌格式的作风。用这种格式投稿，不但可以劳累读者眼球还可以扩张版面多赚稿费。但如果在工作这样写代码，同事会骂你，老板会K你。对比一下下面的写法，相信不难看出故意“膨化”的代码是否丑陋。
#include <stdio.h> #include <stdlib.h> // 引入随机函数srand()、rand()所在的头文件 #include <time.h> // 引入时间函数time()所在的头文件 #include <string.h> // 引入字符查找函数strchr()所在的头文件
8. int main()
main()的这种写法因为与现代C语言的精神相违背，因此不被提倡。较好的写法是
int main ( void )
10. // 定义保存目标数字的字符数组 11. char bull[5] = "";
这两行代码同样有浪费篇幅之嫌。bull作为程序的主要数据结构，选择字符数组并不明智。代码作者选择这种结构的目的是为了使用现成的库函数strchr()。使用现成的库函数无可指责，但在这个问题中字符数组这种数据结构会引起其他方面的问题。利弊相权，并不合算。
此外这里的5是一个MAGIC NUMBER。
这段代码的另一个问题是对数组毫无意义地进行初始化，这是一种无聊的行为。
12. srand((int)time(0));
这一句代码的目的是设置种子数(seed)。这行代码虽然很短，但有很多潜在的问题值得讨论。
首先time(0)这个写法，我的看法是属于C++风格，非常不C。在C语言中应该写time(NULL)。不过也有人认为在C代码中写time(0)无伤大雅。这可能是一个见仁见智的问题。
另外一个问题是关于(int) time(0)这个表达式的，这个问题则并非是见仁见智那么简单。由于srand()的函数原型为:
void srand (unsigned int);
因此
srand((int)time(0));
在本质上其实就是
srand((unsigned)(int)time(0));
这是因为根据C语言规则，编译器总会在这里进行隐式类型转换。与下面两种写法比较一下
srand((unsigned) time(NULL)); srand(time(NULL));
就会发现(int)这个类型转换相当地无聊，它徒劳地进行了一次毫无意义的、无谓的类型转换，但最终还是要转换为unsigned类型。就像歌里唱的那样：“终点又回到起点”，但代码作者还没“发觉”。因此这是一种画蛇添足的行为，说明了代码作者语法不清、逻辑混乱。
(int) time(0)中的这个类型转换还存在其他一些问题。我们都知道，time()函数的返回值的类型为time_t，C语言并没有完全明确这种类型，只要求它是一种实数类型(real type)并且能够表示相应的时间。实数类型包括整数类型和实浮点类型，具体选择哪种类型由编译器自行确定。
当time_t为浮点类型时(说实话没见过，但这种可能性是存在的)，如果time(0)的返回值超出了所要转换成的整数类型的表示范围时，代码尽管可以通过编译，但程序实行的却是一种未定义行为(UB，undefined behavior)。
由于time(0)的返回值为一正数，而 int类型所能表示的最大正整数显然小于unsigned类型所能表示的最大正整数，因此对time(0)进行(int)这样的转换显然比进行(unsigned)转换存在更大的发生UB危险性。
如果time_t为整数类型(多数的编译器中是long类型)，在进行long => int 的转换时，如果被转换的值在int的表示范围内，不会有任何问题。但是如果被转换的值不在int类型表示的范围内时，结果有两种可能：implementation-defined或者是引发一个implementation-defined的信号(signal)，前者意味着可移植性很差，后者则意味着程序被中断或挂起等待进一步的处理，这简直是在自寻烦恼、自讨苦吃。但是如果是进行long => unsigned的转换，则根本不存在这些问题。
因此，如果你追求代码的至简，这行代码应该写为
srand(time(NULL));
如果你追求清晰和明确，则应该写为
srand((unsigned) time(NULL));
13. // 利用rand()函数产生一个四位随机数， 14. // 并利用sprintf()函数将其转换成字符串，保存在bull字符数组中 15. sprintf(bull, 16. "%d", rand()%10000);
这几行代码有两个问题。
头一个，不清楚代码作者为什么要把sprintf()函数调用掰成了两行，这个函数调用并不长，完全没有任何必要掰成两行写。估计这又是“膨化”作风在作妖搞怪。
第二个问题是，rand()%10000得到的并非是一个“四位”整数，它也有可能得到一位数、两位数或三位数，因此这行代码是错误的。
也就是说，这个程序可能生成一个非四位数，然后让你按照四位来猜。更通俗一点说，这个游戏程序会“耍赖”，你可能绝对猜不出它的那个自称“四位数”的伪“四位数”。
对于有些读者来说，这一点可能看起来不明显。其实你只要把第32行代码修改为
puts("sorry,you lost. :,原来的数为："); puts(bull);
然后再运行程序就清楚了。我的一次运行结果是
1 :1111
1111 :1A3B
2 :1222
1222 :0A1B
3 :2122
2122 :1A0B
4 :3133
3133 :1A0B
5 :4244
4244 :0A0B
6 :4144
4144 :1A0B
7 :5155
5155 :1A0B
8 :6166
6166 :2A2B
9 :6177
6177 :2A0B
10 :6188
6188 :2A0B
sorry,you lost. :,原来的数为：
619
看到这个结果了吧，你永远也猜不中。这样会耍赖皮的程序让人会感到索然无趣。
由于不满足任务的基本功能要求，至此已经可以判定这是一个错误的程序了。这指东打西的程序是一种根本上的错误。后面的讨论与程序的正确性无关，纯粹是为了分析代码中的其他毛病。
事实上生成一个四位伪随机数是一个简单的小学数学问题。由于四位正整数一共有9999-999个，所以可以先用rand()生成一个[9999-999-1,0]区间的伪随机数，即
rand()%(9999-999-1)
然后再加上1000得到的就是四位伪随机数:
rand()%(9999-999-1)+1000
这样产生的伪随机数能够保证一定是4位正整数，但是还不能确保一定是“没有重复数字的4位数”，这个问题将在后面解决。需要说明的是这个问题在样本代码中同样存在。
17. // 定义表示猜测数字的字符数组 18. char cow[5] = "";
这种数据结构的选择和bull是一脉相承的，两者同样不甚合理。这里的初始化同样是画蛇添足。
这个数组的定义的位置也是不恰当的，它应该定义在后面的while循环语句的内部。理由是它只在那个语句的内部被用到。定义变量的原则是在哪儿用就定义在哪儿，尽量局部化。这就像不应该把卫生间用的卫生纸摆在客厅一样，是同一个道理。
22. // 猜测次数，最开始是第一次 23. int count = 1;
24. // 构造一个循环结构，只要没有完全猜对， 25. // 就继续下一次猜测 28. // 判断是否超过次数限制 29. // 最多猜测10次
从中不难发现样本代码作者思路混乱、前后不一。前面摆出了一副无限循环的架势，后来又说“最多猜测10次”。在这种混乱的思路指导下得到的代码必然丑陋不堪。
20. int A = 0; /*……*/ 26. while(4!=A) 27. { /*……*/ 73. A = 0; 74. B = 0; 75. }
站在一定的高度来看这段代码就会发现它的可笑之处，因为程序执行到第26行时A的值只可能为0而永远不可能为4。因而while(4!=A)虽然显得一本正经但其实却是煞有介事的装模作样。
28. // 判断是否超过次数限制 29. // 最多猜测10次 30. if(count > 10) 31. { 32. puts("sorry,you lost. :"); 33. // 跳出猜测循环，结束游戏 34. break; 35. }
从这段代码来看，其实while语句完成的只是一个for语句的功能。代码的结构应该这样：
for ( count = 0 ; count < 10 ; count ++ )
{
//猜数
}
puts("sorry,you lost.");
把这结构和样本中的
26. while(4!=A) 27. { /*……*/ 30. if(count > 10) 31. { /*……*/ 34. break; 35. } ++count; /*……*/ 75. }
这个结构对比一下，就会发现样本代码结构的蹩脚可笑十分明显。
41. scanf("%s",cow);
这行代码存在潜在的安全问题。由于cow的类型是char [5]，所以在执行这条语句时用户一个手滑、多输入了一个字符就可能导致灾难。
如果实在要用scanf()完成输入，至少应该写成：
scanf("%4s",cow);
42. // 采用循环结构，逐个字符比对输入数字（cow）和目标数字（bull） 43. for(int i = 0; i<4; ++i) 44. { 45. // 判断当前字符是否相同 46. if(bull[i] == cow[i] ) 47. { 48. // 如果相同，则表示数字正确且位置正确， 49. // A的值增加1 50. ++A; 51. } 52. else // 否则，判断当前数字是否是正确数字 53. { 54. // 在目标字符串中查找当前字符 55. char* p = strchr( bull, cow[i]); 56. // 如果p不是一个空指针（C语言中用NULL或者0表示）， 57. // 则表示目标字符串中有这个字符，当前字符是正确数字， 58. // B的值增加1 59. if(0 != p) 60. ++B; 61. } 62. }
这段代码膨化得非常严重，简直可以称得上是膨化主义的典范。如果把它做一下“缩水”处理，剩下的干货连一半行数都用不到。
由于前面bull数组中可能得不到正确的数字(不是四位数的情况)，所以这段代码没有意义。因为谁也不可能在一种错误的数据的基础上得到正确的算法。
即使不存在前面的错误，这段代码在逻辑上也是错的。例如当bull表示1234，而用户输入的cow是1122时，这段代码输出结果将是1A3B，而按照游戏规则，结果应该是1A1B。（感谢 王爱学志 网友指出）
抛开正确性问题不谈，把如此臃长的结构塞在一个while循环语句内部也是极其丑陋的，很显然，尽管代码作者口口声声大喊大叫地嚷着似是而非的“搭积木”的口号，但其实根本尚未领悟结构化程序设计思想。这里的功能其实应该用一个函数实现。
65. // 如果完全猜测正确，则输出最终结果，结束游戏 66. if(4 == A) 67. { 68. printf("you win! the bull is %s.",bull); 69. break; 70. }
这段中的“4==A”这种把常量写在==前面的写法，感觉矫情得有些变态。
此外，break;语句不如直接写return 0;。
至此，样本代码评析完毕，下面进行重构。（未完待续）
续文链接：http://www.cnblogs.com/pmer/archive/2012/10/20/2731910.html
