劣质代码评析——《写给大家看的C语言书(第2版)》附录B之21点程序(三)

  下面来考察一下main()的总体结构。

29. main()
30. {
31.    int numCards;
32.    int cards[52],playerPoints[2],dealerPoints[2],total[2];
33.    char ans;
34.    
35.    do 
36.    { 
37.       initCardsScreen(cards,playerPoints,dealerPoints,total, &numCards);
38.       dealerGetsCard(&numCards,cards, dealerPoints);
39.       printf("\n");
40.       playerGetsCard(&numCards,cards,playerPoints); 
41.       playerGetsCard(&numCards,cards,playerPoints);
42.       do
43.       {
44.          ans = getAns("Hit or stand (H/S)?");
45.          if ( ans == 'H' )
46.          { 
47.             playerGetsCard(&numCards,cards,playerPoints);
48.          }  
49.       }
50.       while( ans != 'S' );
51.       
52.       totalIt(playerPoints,total,PLAYER);
53.       do
54.       {
55.          dealerGetsCard(&numCards,cards,dealerPoints);
56.       }
57.       while (dealerPoints[ACEHIGH] < 17 );
58.       
59.       totalIt(dealerPoints,total,DEALER);
60.       findWinner(total); 
61.       
62.       ans = getAns("\nPlay again(Y/N)?");  
63.    }
64.    while(ans=='Y');
65.    
66.    return 0;
67. }
68. 

   main()函数中do-while语句循环体部分的含义是这样的

35.    do 
36.    { 
              /*21点游戏*/
62.       ans = getAns("\nPlay again(Y/N)?");  /*询问是否继续*/
63.    }
64.    while ( ans == 'Y' ); 

   与下面写法相比,两者在逻辑上的结构差别很明显

    do 
    { 
             /*21点游戏*/
    }
    while(  getAns("\nPlay again(Y/N)?") == 'Y' );  /*询问是否继续*/

   前者循环体内要做两件事,更精确地说做1.5件事情,因为“询问是否继续”没有完全做利索,一脚门里一脚门外,拖泥带水。而后面的写法在循环体内只关注一件事情,这件事没有与另一件事情搅在一起,而是分处于do-while语句的不同部分,有个清楚的分界线。
  即使从这个视角看,也不能赞成原来代码使用ans 变量的写法。

  既不应该把不同的事情(“21点游戏”和“询问是否继续”)搅在一起来做,也不应该把一件事(“询问是否继续”)分裂为分离的两部分在do-while语句的不同部分完成。

  现在回到循环体内,看一下完整的21点游戏的模拟过程。 

37.       initCardsScreen(cards,playerPoints,dealerPoints,total, &numCards);

   这条语句的主要目的是实现对程序相关数据的初始化。直观感觉是这个函数的参数太多了,通常标志着数据结构设计的失败。 

16. void initCardsScreen(int cards[52],int playerPoints[2],
17. int dealerPoints[2], int total[2], 
18. int *numCards);

   这个函数类型声明写得很不规范。[]内的52、2、2、2毫无必要,代码对齐格式也很成问题。应该为:

 void initCardsScreen( int cards[] ,int playerPoints[] ,
       int dealerPoints[] , int total[] , 
                   int *numCards ) ;

   同理,对应的函数定义中

69. void initCardsScreen( int cards[52],int playerPoints[2],
70.                       int dealerPoints[2], int total[2], 
71.                       int *numCards )
72. {
73.    int sub,val = 1 ;
74.    char firstName[15];
75.    *numCards=52;
76.    
77.    for(sub=0;sub<=51;sub++)
78.    {
79.       val = (val == 14) ? 1 : val;
80.       cards[sub] = val;
81.       val++;  
82.    }
83.    
84.    for(sub=0;sub<=1;sub++)
85.    { 
86.       playerPoints[sub]=dealerPoints[sub]=total[sub]=0;
87.    }
88.    dispTitle();
89.    
90.    if (askedForName==0)
91.    { 
92.       printf("What is your first name?");
93.       scanf(" %s",firstName);
94.       askedForName=1;
95.       printf("Ok, %s,get ready for casino action!\n\n",firstName);
96.       getchar();
97.    }
98.    return;        
99. }

 中,函数的头部同样应该做类似修改

 void initCardsScreen( int cards[],int playerPoints[],
                       int dealerPoints[], int total[], 
                       int *numCards )

   因为形参不可能是数组,[]内的内容无论是多少编译器都将予以忽略。写了也是白写,写它作甚?

  numCards这个形参的名称甚劣,它本来是一个指针,但却居然与它所指向的变量同名。这种写法说明代码作者缺乏最基本的编程素养。

  initCardsScreen()函数的功能据代码作者描述是初始化表示52张牌的数组(写入4套1~13),清屏并显示标题。从这个功能描述就足以认定这个函数的设计非常失败——它的功能太多了。函数应该只做一件事。

  函数体中的 

75.    *numCards=52;

 一句,非常煞有介事,因为这是完全用不着的。只要在main()中的do-while语句的循环体内中写一句 

numCards = 52 ;

 就可以了,用函数调用的方法为变量numCards初始化是根本不值得的。

  读到这里,不难发现在main()中的这些变量定义的位置也不恰当,这些变量应该在do-while语句之内定义。 

   do 
    { 
      int numCards = 52 ;
      int cards[52],playerPoints[2],dealerPoints[2],total[2];

          /*21点游戏*/
    }
    while(  getAns("\nPlay again(Y/N)?") == 'Y' );  /*询问是否继续*/

   变量的定义应该尽量局部化。 

77.    for(sub=0;sub<=51;sub++)
78.    {
79.       val = (val == 14) ? 1 : val;
80.       cards[sub] = val;
81.       val++;  
82.    }

   这个写法很业余,而且51是一个Magic Number,14也是。专业一点的写法是 

    for ( sub = 0 ; sub < 52 ; sub++ )
    {
       cards [ sub ]  =  sub  %  13  +  1 ;
    }

   所以val这个变量也同样是多余的。

 

84.    for(sub=0;sub<=1;sub++)
85.    { 
86.       playerPoints[sub]=dealerPoints[sub]=total[sub]=0;
87.    }

   “sub<=1”的写法很业余,专业人员一般写为“ sub < 2 ”。

  “playerPoints[sub]=dealerPoints[sub]=total[sub]=0;”这个写法通常会受到非议,比较讲究风度的程序员一般这样写: 

  playerPoints[sub]
= dealerPoints[sub]
= total[sub]
= 0 ;

   还应该注意到的是,这几个数组的各个元素由于初始化为0,其实也不用通过函数调用这么复杂的方法来实现,最简洁初始化方法是在main()中直接初始化:

    do 
    { 
      int numCards = 52 ;
      int cards[52] , 
            playerPoints[2] = { 0 } ,
            dealerPoints[2] = { 0 } ,
            total[2] = { 0 } ;

          /*21点游戏*/
    }
    while( getAns("\nPlay again(Y/N)?") == 'Y' );  /*询问是否继续*/

   结论不难得出:变量定义位置不当,会让代码笨拙无比,会给自己带来很多麻烦。现在也可以确认initCardsScreen()这个函数参数过多,实际上只需要对cards[]数组初始化。 

88.    dispTitle();

   这行代码的作用据作者说是清屏,但是这个函数调用在initCardsScreen()这个函数中明显是一种错位。从这个函数的定义看

215. void dispTitle(void)
216. {
217.    int i = 0 ;
218.    while(i<25)
219.    { 
220.         printf("\n");
221.         i++; 
222.    }
223.    printf("\n\n*Step right up to the Blackjack tables*\n\n");
224.    return ;
225. }

   其作用是输出了25个新行字符,然后输出字符串"\n\n*Step right up to the Blackjack tables*\n\n"。注意,此时光标移动到屏幕的左下方,而真正的清屏,光标是在屏幕的左上方。所以作者认为自己写了一个“比通常使用的清屏函数更加特别的清屏函数”,显然只是一种一厢情愿的错觉,他写得太“特别”了,已经特别到了算不上是清屏函数的程度了。

  即使纯粹从写法上来讲,这个函数写得也很拙劣。下面的写法要整洁干净得多。 

void dispTitle( void )
{
   int i ;
   for( i = 0 ; i < 25 ; i ++ )
   {
        putchar('\n');
   }
   puts("\n\n*Step right up to the Blackjack tables*\n");
}

   现在回到initCardsScreen()函数定义部分。

90.    if (askedForName==0)
91.    { 
92.       printf("What is your first name?");
93.       scanf(" %s",firstName);
94.       askedForName=1;
95.       printf("Ok, %s,get ready for casino action!\n\n",firstName);
96.       getchar();
97.    }

   这段代码的毛病很多。首先,这段代码根本就不应该出现在initCardsScreen()函数定义之中,因为这和初始化没关系。
  其次,即使出现,也应该像dispTitle()那样抽象为一个函数为好。
  第三,现在终于弄清代码作者设置askedForName这个外部变量的用意了,但这种用意不但愚蠢而且笨拙。因为这个事情完全应该安排在main()中进行,大致的写法如下

int main(void)
{
   char firstName[15];

   dispTitle();
   /*这里读入firstName*/

   do{
        /*21点游戏*/
   }while( getAns("\nPlay again(Y/N)?") == 'Y' );  /*询问是否继续*/

   return 0;
}

   完全没有必要笨拙且愚蠢地动用危险的外部变量,那种写法完全是初学者的水准,写在书上给其他初学者做为示范是极其不负责任的行为。

  看到这里有些无语……,还能说什么呢?用星爷的话来形容,“失败中的失败”。

  第四,作者声称 

96.       getchar();

 会招致编译器的警告,并要读者心安理得地忽视这个警告(ignore compiler warning here)。说实话,我见过这种由于单独调用getchar()函数而不使用其返回值的警告,但是要读者忽视警告则是一种误导和教唆。
  这里如果有警告,应该用下面方法去除:

(void) getchar();

 

posted @ 2013-07-09 08:07  garbageMan  阅读(1802)  评论(38编辑  收藏  举报