改善一个劣质的C++代码

在C++论坛里发了一个与本文题目相同的贴子如下：

下面是一个我在一个应用系统中发现的我认为很丑陋的一段C++代码.显然,代码的意图是将string中的内容变成相应的小写字母.请大家指出它的问题之所在,并给出改善后的代码.我自已已经有了一个答案,只用一条语句就能实现这个功能.待两日后我给出我的答案.但然,也许你的答案比我的更好.
void IniReader::string_lower(string& str)
{
    int size=str.length();
    string temp;
    char c;
    for(int i=0;i <size;i++)
    {
        c=str.at(i);
        if(c>=65&&c <=90)
        {
            c+=32;
        }
        else
        {
            continue;
        }
        temp=c;
        str.replace(i,1,temp);
    }
}

我给出的改善的代码是：

 

# include  <string>
inline std::string& string_tolower(std::string& str)
{
   std::transform(str.begin(), str.end(), str.begin(), std::tolower);
   return str;
}

 

将函数的接口做了调整。这个函数没有涉及到任何的类的变量，显然不应该属于类函数。将修改后的string作为返回值，将使得使用起来更加方便。例如可以如下使用：

 

std::cout << string_tolower("Hello, UPPER string TO LOWER STRING");

 

函数的实现使用了stl的模版函数transform实现，这就是我说的用一条语句就实现的地方。首先跟贴的那个表示怀疑的朋友现在是否明白了。

海啸朋友最先给出transform的解答，其他几个朋友都写出了transform的改善方法。基本上符合了我的答案。细节的地方做个说明：那个tolower函数最好加上std::的前缀修饰，因为有些系统将tolower定义成了宏，缺少std的修饰说明，有可能会编译出错。另外，这个函数由于简短，适合于放在头文件中，作为inline函数。

 

朋友们在注重改善实现的时候，忘记了我还还有一句：请大家指出它的问题之所在。

 

在这里我举出下面几点：

• 那么temp = c 将构成string类的对象，并且在循环体内，将严重影响效率。而使用replace进行字符替换，显然是小题大做。因为这里要进行的是一个字符的更改，并不是将这个字符替换成一个字符串。
• continue语句可以省去。虽然continue语句好于goto语句，但仍然属于影响正常流程的跳转语句。
• 使用数字65表示字符A，可读性差。
• 格式风格的美观可读性差。比如 if(c>=65&&c <=90) 密密麻麻的挤压在一起，很难看。写成  if ( (c >= 64) && (c <= 90) )会更好看一些。

其实，我还可以举出一些问题。先慢慢来吧。

我只所以说这个程序丑陋，并非是说它一定要用一句tranform来实现就算漂亮了。如果你还不熟悉STL模版库。也可以写成下面这样：

 

for ( int i = 0; i < str.size(); i++ )
  str[i] = tolower(str[i]);

 

如果，你还不知道tolower这个函数，也可以写成下面：

  int c = str[i];
  if ( (c >= 'A') && (c <= 'Z') )
    str[i] = c + ('a' – 'A');

 

 

这里，str.size()由于是inline函数，放在循环判断上不会影响程序的效率，并不需要像原来程序一样，设一个size的中间变量。同样，tolower也是inline函数，也不会增加程序的函数调用开销。

如果看了我这篇的解释，你还不以为然。那我到真希望你来维护一下我看到的这个系统：那可以延长到两页的循环体里，布满了continue；那充满了硬编码的需要你从一堆代码中去寻找含义的魔数；那一个类里面竟然设计了一大堆毫不相关的函数。实在的，我都有快崩溃的感觉。写丑陋的代码，实际上是在给后来使用和维护代码的人挖陷阱。当然，跳进这个陷阱的有可能就是你自己。