评playerc网友的"求比指定数大且最小的“不重复数”问题"
问题见:对Alexia(minmin)网友代码的评论及对“求比指定数大且最小的‘不重复数’问题”代码的改进 、算法:求比指定数大且最小的“不重复数”问题的高效实现 。
playerc网友的代码如下(求比指定数大且最小的“不重复数”问题):
1 #include <stdio.h>
2 #include <stdlib.h>
3 #include <errno.h>
4
5 #define NUMBER_STR_MAX_LENGTH (128)
6 unsigned find(unsigned);
7
8 int main(void)
9 {
10 unsigned num = 0;
11
12 printf("Please input Unsigned Number : ");
13 if (scanf("%u", &num) != 1) {
14 perror("input error!");
15 return 1;
16 }
17
18 printf("%u\n", find(num));
19 return 0;
20 }
21
22 void str_reverse(unsigned char *str,unsigned length)
23 {
24 unsigned char *prv = NULL;
25 unsigned char *cur = NULL;
26 unsigned char swap = '\0';
27
28 for (prv = str, cur = str + (length-1)
29 ; prv < cur; ++prv, --cur){
30 swap = *prv;
31 *prv = *cur;
32 *cur = swap;
33 }
34 }
35
36 unsigned find(unsigned number)
37 {
38 unsigned char number_str[NUMBER_STR_MAX_LENGTH] = {'\0'};
39 unsigned char *prv = NULL;
40 unsigned char *cur = NULL;
41 unsigned char *finded = NULL;
42 unsigned int number_length = 0;
43 unsigned int result = 0;
44 int carry = 0;
45
46 ++number;
47 number_length = sprintf(number_str, "%u", number);
48 str_reverse(number_str, number_length);
49
50 for (cur = number_str + (number_length-1), finded = number_str
51 ; cur > finded - 1 ; --cur ){
52
53 if (prv != NULL && *prv == *cur){
54 finded = cur;
55 carry = 1;
56 do {
57 *cur += carry;
58 carry = (*cur - '0') / 10u;
59 *cur = (*cur-'0') % 10u + '0';
60 ++cur;
61 } while (carry != 0 && cur < (number_str + number_length));
62 if (carry > 0){
63 *cur = '0' + carry ;
64 ++number_length;
65 }
66 }
67 prv = cur;
68 }
69
70 /* set rests as 010101 */
71 for (carry = 0; cur > number_str - 1; --cur){
72 *cur = carry + '0';
73 carry ^= 1u;
74 }
75 str_reverse(number_str, number_length);
76 sscanf(number_str,"%u",&result);
77 return result;
78 }
总体印象:作者写得很认真,也学习了很多新知识并努力地应用这些新知识。有结构化程序设计的意识,但还不够熟练。
认真体现在
#define NUMBER_STR_MAX_LENGTH (128)
实际上那个括号是没必要的,但写上更好。
不过128这个值显得太贪心了,因为输入的数是放在unsigned num之中,目前的计算机的unsigned类型无论如何也不可能达到十进制128位。(4个字节能存储的unsigned类型最大是G的量级,8字节是应该是10的20次方的量级)
#include <stdlib.h>
没看出有这个必要,后面没有用到stdlib.h中的内容。
#include <errno.h>
这个同样没必要。作者大概以为调用perror()函数需要,其实perror()函数的原型在stdio.h中。
unsigned find(unsigned);
这个很好。不过这里同时缺少str_reverse()的函数类型声明。可能str_reverse()是后补的,要么就是忘记了。
main()写得还行,但有些画蛇添足。
if (scanf("%u", &num) != 1) {
perror("input error!");
return 1;
}
这里作者认为没有正确读入num时会发生错误,但实际上这不能算是错误。如果输入一个非十进制整数,例如输入a字符,scanf("%u", &num)的返回值得0,上面代码段的运行结果将是:
input error!: No error
这就有些滑稽了。常规上这段应该这样写
if (scanf("%u", &num) != 1) {
printf("input error!\n");
return 1;
}
perror()一般用在这样的场合:
pow(10,1234);
perror("pow函数");
这段代码的输出将是:
pow函数: Result too large
main()中调用的是find()函数,按理应该先分析一下,但这个函数写得太长也太复杂,还是放到最后分析。先来看一下str_reverse()函数。
void str_reverse(unsigned char *str,unsigned length)
{
unsigned char *prv = NULL;
unsigned char *cur = NULL;
unsigned char swap = '\0';
for (prv = str, cur = str + (length-1)
; prv < cur; ++prv, --cur){
swap = *prv;
*prv = *cur;
*cur = swap;
}
}
这个函数总的感觉就是写得啰嗦不堪而且惊心动魄。
首先prv绝对是多余的,它除了接受str的值没有任何其他用处。反过来也可以说str除了把自己的值赋给prv以外没有其他任何用处,就像一个只会传球的二传手。这两个变量其实只需要一个。
cur这个变量其实意义也不大,因为可以用str和length表达。
unsigned char swap = '\0';
这里的初始化毫无意义,真正的C程序员不会做这种无厘头的初始化。这种无厘头地进行初始化的作风,我的印象是java语言之后兴起的,是很多无脑的C程序员向某些脑残的java程序员学来的。
这个变量定义的位置也不对,应该定义在for语句的循环体内。
这段代码惊心动魄的地方体现在:
str + (length-1)
如果length的值为0u,要知道(length-1)可绝对不是-1。
--cur
这也很吓人,因为length为0u或1u时这是未定义行为。
当然,这两种情况也许并没有出现。但作者事先想没想到则是另一回事。如果作者事先没想到,是代码作者的失职。如果事先想到了的话,则说明作者不称职:这要多浪费多少脑细胞啊。成天想这些无聊的事情,怎么可能再有精力去想程序功能的实现呢?这就跟有些人喜欢成天“摸着石头过河一样”,实际上那边明明有座大桥——稍微动点脑筋不难发现,这种惊险其实完全可以避免。
swap = *prv;
*prv = *cur;
*cur = swap;
这段应该用函数实现,应该把这个东西与reverse尽可能地分开,而不是搅在一起。
如果我写str_reverse(),大概会这样
void str_reverse( unsigned char * , unsigned ); void exchang( unsigned char * , unsigned char * ); void exchang( unsigned char * p1 , unsigned char * p2 ) { unsigned char temp = * p1 ; * p1 = * p2 ; * p2 = temp ; } void str_reverse( unsigned char * str , unsigned length ) { while ( length > 1u ) { exchang( str , str + length - 1u ); str ++ ; length -= 2u ; } }
最后来看一下find()函数。
unsigned find(unsigned number)
{
unsigned char number_str[NUMBER_STR_MAX_LENGTH] = {'\0'};
unsigned char *prv = NULL;
unsigned char *cur = NULL;
unsigned char *finded = NULL;
unsigned int number_length = 0;
unsigned int result = 0;
int carry = 0;
++number;
number_length = sprintf(number_str, "%u", number);
str_reverse(number_str, number_length);
for (cur = number_str + (number_length-1), finded = number_str
; cur > finded - 1 ; --cur ){
if (prv != NULL && *prv == *cur){
finded = cur;
carry = 1;
do {
*cur += carry;
carry = (*cur - '0') / 10u;
*cur = (*cur-'0') % 10u + '0';
++cur;
} while (carry != 0 && cur < (number_str + number_length));
if (carry > 0){
*cur = '0' + carry ;
++number_length;
}
}
prv = cur;
}
/* set rests as 010101 */
for (carry = 0; cur > number_str - 1; --cur){
*cur = carry + '0';
carry ^= 1u;
}
str_reverse(number_str, number_length);
sscanf(number_str,"%u",&result);
return result;
}
这个函数的功能是找大于number的最小不重复数。
这个函数显然是有些过于庞大了,变量也太多,同时无意义的初始化依然存在。
变量太多的代码明显不容易把控,据说人类大脑最多能同时思考7个变量,这里加上形参已经有8个了。我相信作者写这个函数的时候一定很痛苦,因为我阅读起来更痛苦。
造成变量太多的原因一般是因为不会写代码。
首先,这种人成天把算法挂在嘴上,却不懂得设计好的数据结构本身才是算法成立的基础。毫不夸张地说,数据结构就是静态的算法。
最近看的几个代码,都只用到了数组或字符串,说明这些作者根本不懂得什么叫数据结构,以至于看到结构体这种简单的数据结构都发蒙、都觉得复杂。这些人多半是中了谭浩强的流毒,被“算法是程序的灵魂”这句华而不实的狗屁口号所迷惑,“总觉得算法最难了”,实际上不懂得数据结构根本就没资格谈算法。卖糖葫芦的老头都懂得把糖山楂穿在一起,要是这些人去卖糖葫芦,非一个粒一个粒去卖不可。
造成变量太多的另一个原因是不懂得结构化程序设计,把所有的功能都堆积在一个函数中完成。要完成的任务多,所需要的变量也多。
不懂得分层,把所有的变量都放在一起定义也会造成变量过多。变量定义的位置,原则上应该是作用域越小越好。应该用到才定义,而不是一次都定义完。至于外部变量,则通常是更恶心的写法。
number_length = sprintf(number_str, "%u", number);
str_reverse(number_str, number_length);
到这里才看明白,作者的number_str居然是要存储字符串。这是很蹩脚的数据结构设计,理由是后面进行加法运算时会非常别扭(这让我想起了那些C#“程序员”)。作者只是想利用一下现成的sprintf(),但sprintf()并不适合使用在这里,最后贪小便宜吃大亏。后面不得不自己写一个str_reverse(),再搭上别别扭扭的“字符”加法运算。所以使用sprintf()并没有得到任何便利,这与库函数的本意相违背。在这里应该自己写一个函数,并且用number_str存储各位数字,而不是存储与数字相对应的字符。
for (cur = number_str + (number_length-1), finded = number_str
; cur > finded - 1 ; --cur ){
if (prv != NULL && *prv == *cur){
finded = cur;
carry = 1;
do {
*cur += carry;
carry = (*cur - '0') / 10u;
*cur = (*cur-'0') % 10u + '0';
++cur;
} while (carry != 0 && cur < (number_str + number_length));
if (carry > 0){
*cur = '0' + carry ;
++number_length;
}
}
prv = cur;
}
这个for语句写得太庞大了,很难看清楚。加上作者使用了“{在右侧}在左侧”的代码风格,可读性进一步恶化。实际上“{在右侧}在左侧”这种风格并不适合初学者,把{}同时写在左侧的风格并不丢人。
这个for语句中有一个很明显的错误,就是“finded - 1”这个表达式是未定义行为。从风格上来说也有些不伦不类:
for (cur = number_str + (number_length-1), finded = number_str
; cur > finded - 1 ; --cur ){
/*……*/
prv = cur;
}
很明显不如写成
for (cur = number_str + (number_length-1), finded = number_str ; cur > finded - 1 ; prv = cur , --cur ){ /*……*/ }
循环体内部除了繁琐、复杂、笨重以外,看了好几遍,没有发现什么错误。但要注意这并不是一种表扬。没有发现错误是因为代码太复杂,以至于难以发现有明显的错误。而优秀的代码则是简单,简单到明显没有错误。明显没有错误和没有明显的错误绝对是两码事,天壤之别。
for语句之后的
/* set rests as 010101 */
for (carry = 0; cur > number_str - 1; --cur){
*cur = carry + '0';
carry ^= 1u;
}
显然应该用一个函数完成。这里面同样有用数组名减1的错误。但这段代码完成了在后面只一次加010101的功能,这是这段代码的唯一亮点。要说一下的是carry这个变量,在这里的意义和前面代码中完全不同,这个变量在这个函数里至少有三种含义,代码混乱不堪也就顺理成章了。
最后两句
str_reverse(number_str, number_length);
sscanf(number_str,"%u",&result);
看着就累,sscanf()这个小便宜占得得不偿失。一句话,作者选择了错误的数据结构。