下面的12个C语言的语句是我从实际的应用系统中摘录和简化来的。这些语句都被我列入丑陋代码的类别中。对此,我给出了相应的改善程序和说明。
1.
if ( strlen(s) == 0 )
if ( strlen(s) )
if ( strcmp(s, “”) == 0 )
改善:
# define isNotEmpeyStr(s) ( (s)[0] != ‘/0’ )
# define isEmptyStr(s) ( (s)[0] == ‘/0’ )
这样可以用上面两个宏来判断字符串是否为空,或者字符串是否不为空。
当然,也可以直接写 if ( s[0] == ‘/0’ ) 或者 if ( s[0] != ‘/0’ ),但使用宏,有更好的可读性。
2.
memcpy(s1, s2, 1);
对于一个字节的拷贝,竟然也调用memcpy。看来,C语言可以删除赋值的语法功能了。
改善:
s1[0] = s2[0];
3.
for ( i = 0; i < strlen(s); i++ )
每次循环的循环判断中都有strlen的函数调用,显然降低效率。
改善如下:
len = strlen(s);
for ( i = 0; i < len; i++ )
4.
printf("OK/n");
对于没有格式控制的输出,最好使用puts,fputs等替代。这样有更高的效率。
puts("OK");
5.
char sbuf[1024];
memset(sbuf, 0, sizeof sbuf);
sprintf(sbuf, "select %s from %s where %s = ", col_name, table_name, key);
strcat(sbuf, value);
这里,memset实属多余。而strcat需要扫描 sbuf到有字符串的结尾,然后进行拷贝。
改善如下:
char sbuf[1024];
int pos;
pos = sprintf(sbuf, "select %s from %s where %s = ", col_name, table_name, key);
strcpy(sbuf + pos, value);
6.
str[strlen(str)] = '/0';
写这个语句的程序员大概是被字符串以'/0'结尾的问题搞昏头了。它忘记了一个常识:在有尾巴的地方设置一个尾巴,不是一个多余的行为吗?
7.
sprintf(curtime, "%02d:%02d:%02d:%03d", tt->tm_hour, tt->tm_min, tt->tm_sec, tb.millitm);
curtime[12] = 0;
sprintf函数本身就能在curtime 的后面添加0, 那句curtime[12] = 0的语句实属多余。另外,佩服写这个程序的人,还能算清楚那个字符的长度是12。
8.
if ( ch == 1 ) {
..........
}
if ( ch == 2 ) {
...........
}
if ( ch == 3 ) {
..........
}
改善如下:
if ( ch == 1 ) {
..........
} else
if ( ch == 2 ) {
...........
} else
if ( ch == 3 ) {
……………
}
如果,中间的处理不复杂的话,可以使用switch语句。
9.
nRet=lockWait(c_sem_id);
if(nRet!=0)
{
usleep(100000);
nRet=lockWait(c_sem_id);
if (nRet!=0)
{
usleep(100000);
nRet=lockWait(c_sem_id);
if(nRet!=0)
{
logger->fatal("lock Virtual teller config failed”);
nRet=unlockWait(c_sem_id);
return -1;
}
}
}
这段程序可真快把人雷倒了。大概这个程序员还不知道程序语言的循环语句。不知道如果要求调用lockWait过程是100次的话,他是不是会一直这样写下去。 改善后的代码如下:
const int trytimes = 3;
int try = 0;
while( ( (nret = lockWait(c_sem_id)) != 0 ) && (try++ < trytimes) )
usleep(1e5);
if ( nret < 0 ) {
unlockWait(c_sem_id);
logerr->fatal("lock Virtual teller config failed");
return -1;
}
10
#define SBUF_DBLMIN (-1e20) /* 最小double值 */
char sbuf[16];
double double_val;
………………接收一字符串到sbuf中。
double_val = atof(sbuf);
if ( double_val == SBUF_DBLMIN )
return -1;
这个程序员实在没有搞清楚atof出错的返回问题,使用浮点数与自己定义的所谓最小值比较,更是异想天开的想当然。
关于这个问题,我在另一篇文章中 有更详细的论述。
11
下面是我从一个复杂的输出一系列二进制数据为16进制字符的一段简化的程序,其中c为简化的这个二进制数据的一个字节。
char sbuf[3];
char c;
c已经存放了一个字节的二进制数。
sprintf(sbuf, “%.2x”, c);
我们考虑一个情况,当c的最高位为1的时候,将会出现什么情况。比如: c的值为 0xff。那么它输出到 sbuf里的字串是 ‘ff”吗?那你就和程序的作者犯同样的错误了。
我们知道,在sprintf中,c是作为整型数传给sprintf函数的。而对于char c的变量,当它的值为 0xff时,其实相当于十进制数的-1。想想看,
sprintf(sbuf, ‘%2x”, -1); 会产生什么结果。
也许你现在该明白了,-1的整型数为 0xffffffff,这是假设int是四个字节的情况。那么它输出到sbuf,应该是字串: “ffffffff”。一共是8个字节的长度。而sbuf却只定义有3个字节的空间。显然,会产生溢出。
这个问题,可以修改sprintf调用如下:
sprintf(sbuf, “%.2x”, (unsigned char)c);
12
void rtrim( char* str )
{
while( strlen(str) && str[strlen(str)-1] == ' ')
str[strlen(str)-1] = '/0';
}
void ltrim( char* str )
{
while( str[0] == ' ' )
strcpy( str, str+1 );
}
这两个函数看起来显得整洁清晰。甚至程序的作者还引以为自豪,感觉写得很漂亮。但是,他忘记了C语言不是普通的高级语言,它是面向程序员的高效率的语言。对于这样的基础实用函数的写法,正可以看出对于C语言掌握的功底。
在rtrim中,循环的使用strlen调用,严重影响了程序的效率。
在ltrim中,循环体中的strcpy便将这段程序打落到丑陋之列。
下面是我写的程序,看起来比原来的还长。
void rtrim(char *str)
{
char *ps = str;
/* find <str> tail */
while( *ps != '/0' )
ps++;
ps—;
while( (ps >= str) && (*ps == ' ') )
ps—;
*(++ps) = '/0';
}
void ltrim(char *str)
{
char *pd = str;
while( *str == ' ' )
str++;
while( (*pd++ = *str++) != '/0' );
}