评playerc网友的"求比指定数大且最小的“不重复数”问题"

　　问题见：对Alexia(minmin)网友代码的评论及对“求比指定数大且最小的‘不重复数’问题”代码的改进、算法：求比指定数大且最小的“不重复数”问题的高效实现。

 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 }

View Code

　　总体印象：作者写得很认真，也学习了很多新知识并努力地应用这些新知识。有结构化程序设计的意识，但还不够熟练。

　　认真体现在

#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()这个小便宜占得得不偿失。一句话，作者选择了错误的数据结构。

posted @ 2013-10-12 16:26 garbageMan 阅读(1016) 评论(5) 编辑收藏举报

会员力量，点亮园子希望

刷新页面返回顶部

garbageMan

评playerc网友的"求比指定数大且最小的“不重复数”问题"

公告