关于编程、重构等 42条建议上

最新推荐文章于 2021-03-30 11:36:43 发布

置顶黄黄臭臭

最新推荐文章于 2021-03-30 11:36:43 发布

阅读量760

点赞数 1

分类专栏：编程语言一起分享 HackerNews 文章标签：重构编程

HackerNews 同时被 3 个专栏收录

7 篇文章 0 订阅

订阅专栏

编程语言

6 篇文章 0 订阅

订阅专栏

一起分享

4 篇文章 0 订阅

订阅专栏

原文来自：
The Ultimate Question of Programming, Refactoring, and Everything
https://software.intel.com/en-us/articles/the-ultimate-question-of-programming-refactoring-and-everything

译文来自：http://blog.csdn.net/huanglong8/article/details/56029783

先给大目录，大家可以挑着看。。。

前言
 1. 不要去做编译器优化的工作
 2. 0与1是两码事
 3. 复制代码检查多次
 4. 三目运算符括起来用
 5. DllMain中不要做太多复杂的事
 6. 指针被显式转换为整型
 7. 不要在循环中调用alloca函数
 8. 析构函数中抛出异常是很危险的
 9. 使用0作为终止字符
 10. 避免使用多个短小的ifdef
11. 不要尝试在一行中做很多事
 12. 复制粘贴小心最后一行
 13. 表格式的代码布局
 14. 光有好的编译器和编码风格是不够的
 15. 在代码中使用枚举
 16. 看还能怎么做同样不要压缩一行
 17. 使用提供的功能清除私人数据
 18. 用一种编程语言的知识不完全适用于另一种语言
 19. 正确的调用构造函数
 20. EOF检查可能不够
 21. 检查文件结束字符是否真的EOF
22. 不要使用 pragma warningdefaultX
23. 让程序自己算长度
 24. 不要覆盖虚函数
 25. 不要把this和nullptr做比较
 26. 阴险的VARIANT_BOOL
27. 狡诈的BSTR类型
 28. 用简单函数不要用宏
 29. 在迭代器中使用i不要用后缀i
30. VC和sprintf函数
 31. CC中数组参数传递问题
 32. 危险的printf
33. 永远不要引用空指针
 34. 保证类型的区间大小可用
 35. 添加枚举记得修改switch
36. 如果电脑发神经请检查内存
 37. 注意在do while中的continue
38. 从现在开始使用nullptr而不是null
39. 代码怎么又不正确了
 40. 使用静态代码分析
 41. 避免向项目中添加新库
 42. 不要使用empty命名函数
 结论

主要说了在编程方面的42条建议，这些建议可以帮助你避免错误，节省时间和精力。原作者是 Andrey Karpov - “Program Verification Systems”的技术总监，它的团队致力于研发PVS-Studio静态代码分析器。检查了大量的开源项目后，整理出了一些建议，并且每个建议都给出了一个实际的例子，以此说明这个问题的现实性。以 C / C ++程序员为主，但通常可以应用所有范例中。
这里写图片描述

前言

作者是C/C++语言的牛人，是微软VC++ MVP，作者写作的意图就是让你写出更安全，更高效的C代码，作者也喜欢分享，才有了这42条建议。

1. 不要去做编译器优化的工作

此内容来自mysql项目，在PVS-Studio分析器中诊断出如下错误：

static int rr_cmp（uchar * a，uchar * b）
{
  if（a [0]！= b [0]）
    return（int）a [0]  - （int）b [0];
  if（a [1]！= b [1]）
    return（int）a [1]  - （int）b [1];
  if（a [2]！= b [2]）
    return（int）a [2]  - （int）b [2];
  if（a [3]！= b [3]）
    return（int）a [3]  - （int）b [3];
  if（a [4]！= b [4]）
    return（int）a [4]  - （int）b [4];
  if（a [5]！= b [5]）
    return（int）a [1]  - （int）b [5]; <<<< ====
  if（a [6]！= b [6]）
    return（int）a [6]  - （int）b [6];
  return（int）a [7]  - （int）b [7];
}

说明
观察上述代码，其实外人也不敢保证它是对的还是错的，但经过mysql上下文分析后，发现，偶尔返回错误的值，原因就是，程序员复制了代码块“if（a [1]！= b [1]）return（int）a [1] - （int）b [1];”。然后他开始更改索引，忘记将“1”替换为“5”。这种小的且偶发性的逻辑错误很难找到。

正确代码

if（a [5]！= b [5]）
  return（int）a [5]  - （int）b [5];

建议
我们当初在思考为什么不写成循环时，我们得知，程序员为了优化代码，帮助编译器做了展开循环的工作。当然在实际开发中，我们可能也会这样做，来提升轻微的速度或是减少内存。但现在的编译器都很聪明，为什么非要这么做。如果一个简单的循环可以帮助理解，并减少犯错的机会，那不是很好？

static int rr_cmp（uchar * a，uchar * b）
{
  for（size_t i = 0; i <7; ++ i）
  {
    if（a [i]！= b [i]）
      return a [i]  -  b [i]; 
  }
  return a [7]  -  b [7];
}

它的优点就是易于阅读和理解，并且不太可能会写错。通常来讲简单的代码通常才是正确的代码，不要尝试去做编译器做的工作。

2. >0与==1是两码事

代码片段来自 CoreCLR项目，其中针对memcmp(…) == -1做了说明，当然还有诸如此类的函数。

bool operator（）（const GUID＆_Key1，const GUID＆_Key2）const
  {return memcmp（＆_ Key1，＆_Key2，sizeof（GUID））== -1; }

说明
int memcmp（const void * ptr1，const void * ptr2，size_t num）;
是将ptr1和ptr2中的前num字节进行比较，内存内容相等返回0，不等则返回非0。
<0 - 前num的第一个不同字节的数值比较，ptr1的小（如果计算为unsigned char值）。
== 0 - 两个内存块的内容相等。
>0 - 前num的第一个不同字节的数值比较，ptr1的大（如果计算为unsigned char值）。
不要将诸如memcmp（），strcmp（），strncmp（）等函数的结果与常量1和-1进行比较。
有趣的是，很多代码都这么写，并且还长期的运行着，从来没有出错，那我认为那是运气，因为当这些函数的行为被更改。例如换编译器，或重写事先，将导致你的代码出bug。
正确代码

bool operator（）（const GUID＆_Key1，const GUID＆_Key2）const
  {return memcmp（＆_Key1，＆_Key2，sizeof（GUID））<0; }

建议
遵从函数确定的实现及返回，不要依赖于经验等，如果文档说返回大于小于0，也不要将他和特定的数字进行比较。
顺便讲一下，这个函数返回1024的实际情况，这个错误是mysql/MariaDB中导致的，主要是由于访问后返回的token对其256位内进行比较，一旦超出，则永远都会返回true，哪怕这个人不知道密码。在password.c中

my_bool check（...）{
  return memcmp（...）;

有关这个问题更详述的信息参考：MySQL / MariaDB中的安全漏洞。

3. 复制代码，检查多次

这个错误来自Audacity项目，代码如下：

sampleCount VoiceKey :: OnBackward（....）{
  ... ...
  int atrend = sgn（buffer [samplesleft-2]  - 
                   buffer [samplesleft  -  1]）;                          
  int ztrend = sgn（buffer [samplesleft-WindowSizeInt-2]  - 
                   buffer [samplesleft  -  WindowSizeInt-2]）;
  ... ...
}

说明
其实一眼看去，如果你不是这个项目的参与者，或者是刚接手这个项目，你很难明白错误在哪里，即便指出你也不能保证这就是错误。buffer [samplesleft-WindowSizeInt-2]出现此错误同样因为复制粘贴，程序员复制了代码字符串，但忘记将2替换为1。这是一个很low的错误，但我们每个人都会犯，这也就是为什么强调的原因。
正确的代码

int ztrend = sgn（buffer [samplesleft-WindowSizeInt-2]  - 
                 buffer [samplesleft  -  WindowSizeInt-1]）;

建议
复制代码快可以帮助你高效的完成敲码工作，但请你要多检查他几次，因为那些实体，数字一旦反生bug，则将很难被找出，稍后我们还是会讨论复制粘贴，让你印象深刻。

4. ？:三目运算符括起来用

片段来自Haiku项目，？：运算符优先级低于-运算符导致。

bool IsVisible（bool ancestorsVisible）const
{
  int16 showLevel = BView :: Private（view）.ShowLevel（）;
  return（showLevel  - （ancestorsVisible）？0：1）<= 0;
}

说明
一同来看看C/C++运算符优先级。三目运算符具有非常低的优先级，低于/+<等等，它也比负优先级低，因此，当混合使用时，需要更加注意。
程序员认为运算顺序是：

（showLevel  - （ancestorsVisible？0：1））<= 0

然后呵呵

（（showLevel  -  ancestorsVisible）？0：1）<= 0

错误明显，这也是很简单的代码，却很容易犯错，如果你有包含三目的非常复杂的算式，那。。。这么难读就不要搞这么长的算式。
正确的代码

return showLevel  - （ancestorsVisible？0：1）<= 0;

建议
如果你的三目混合其中，那么请加上括号，以避免这种错误，也帮助程序员去阅读你的代码。当然我也不排斥直接使用三目，当然，只有你需要一个表达式，表达式中只有三目时，你可以不写括号，这样看起来更简洁。

5. DllMain中不要做太多复杂的事

此段来自LibreOffice项目，由PVS检测出，CreateThread函数不应该由DllMain函数调用。

BOOL WINAPI DllMain（HINSTANCE hinstDLL，
                     DWORD fdwReason，LPVOID lpvReserved）
{
  ....
  CreateThread（NULL，0，ParentMonitorThreadProc，
                （LPVOID）dwParentProcessId，0，＆dwThreadId）;
  ....
}

说明
在一定条件下，DllMain不得不做一些事情，当我在这个主函数下调用Win API时，莫名奇妙的发现它并不工作，我也没设法弄清楚当时的问题。
再进行PVS开发工作时，我突然意识到之前失败的原因。在DllMain函数中，你只能执行非常有限的操作，原因就是Dll可能还没有完全被加载到内存中，你无法在这里面调用其他的API。当然，现在诊断出的结果是警告，以此来提醒程序员，是不是应该放到其他中去。
细节
使用DllMain更多的细节应该参考MSDN上的文章，动态链接库实践。这里列举一些摘要。

DllMain被调用时内存加载处于锁定状态，这意味着在它之中调用的函数有一定的限制，因此，在主函数入口中，应该只进行一小部分的初始化任务，如果你调用了任何直接或间接尝试获取加载器锁的函数，那么程序可能会导致死锁或崩溃，而这种错误可能直接影响到整个进程或线程中。某DllMain的一个好的初始化方法就是尽可能延长初始化，这样使您更安全的使用API。当然不是所有任务都可以延长初始化的，如果某些不正确的资源或配置，应该立即退出，而不是通过做其他的工作继续浪费时间。

不要在DllMain中执行以下任务：

不要直接或间接的调用LoadLibrary或LoadLibraryEx，导致死锁或崩溃滴。
不要直接或间接的调用GetStringTypeA，GetStringTypeEx或GetStringTypeW，同上滴。
与其他线程同步，同上滴。
获取或等待同步对象，锁啊这类的释放，同上滴。
使用ColnitializeEx初始化COM线程。
调用注册表函数。这个是加载advapi32.dll后才能用的。
调用CreateProcess。如果这个进程又加载另一个DLL。
调用ExitThread。在卸载期间如果推出线程，哦哦。
调用CreateThread。如果不同步，是可以的，就是有点风险。
创建命名管道或其他对象（限win2000），命名对象由终端服务DLL提供。如果这个Dll没有初始化，则可能哦崩溃。
使用CRT的内存管理。如果CRT的DLL未初始化，也有问题。
调用User32.dll或Gdi32.dll中的函数，因为这里面的函数可能加载了另一个dll。
使用托管代码。
正确的代码
LibreOffice的代码可能不工作，这是偶发的。修复这个不简单，因为要重构，以使DllMain函数尽可能短。
建议
很难建议，因为谁知道突然发生个莫名其妙的bug，但读完这里，你至少知道，在写这样的代码时，应该注意哪些问题，从而避免。还有不要告诉我在dllmain要干嘛干嘛，我也不知道，这一切都得靠你。

6. 指针被显式转换为整型

此片段来自IPP Samples项目，看这代码

void write_output_image（....，const Ipp32f * img， 
                        ....，const Ipp32s iStep）{
  ... ...
  img =（Ipp32f *）（（unsigned long）（img）+ iStep）;
  ... ...
}

说明
程序员将指针移动一定数量的字节，此代码在win32下正确，因为指针大小和long类型同样。但是如果换成64位版本，就会造成高位丢失。
linux使用不同的数据类型。可能也会有类似问题，但很多linux程序也都会进入到windows中，所以，还是使用intprt_t这样的类型来定义比较妥。
此错误如图：
这里写图片描述
这种错误有时也很难注意到，因为导致指针高位丢失也只能是在程序运行了好长时间后才偶然出现的问题。
正确的代码
使用size_t，INT_PTR，DWORD_PTR，intrptr_t来定义。

img =（Ipp32f *）（（uintptr_t）（img）+ iStep）;

参见EXP36-C
建议
使用指针类型来存储指针。如果要编译64位版本，首先需要检查所有代码，尤其是使用指针强转的地方，来避免麻烦。
这里还有一个学习资源，去看吧。64位C/C++应用开发的经验教训

7. 不要在循环中调用alloca函数

这个bug在Pixie项目中找到，在循环中使用alloca函数，造成堆栈溢出。

inline void triangulatePolygon（....）{
  ... ...
  for（i = 1; i <nloops; i ++）{
    ... ...
    do {
      ... ...
      do {
        ... ...
        CTriVertex * snVertex =
          （CTriVertex *）alloca（2 * sizeof（CTriVertex））;
        ... ...
      } while（dVertex！= loops [0]）;
      ... ...
    } while（sVertex！= loops [i]）;
    ... ...
  }
  ... ...
}

说明
上面示例显示了3个嵌套循环，而每次都会申请内存，如果是VC++项目，通常堆栈内存大小只有1M，这样就会造成溢出。虽然windows是可以更改堆栈内存的默认值的，但这不能作为解决问题的办法。

建议
如果有一个循环并需要分配一个临时缓冲区，请用以下方法。

提前分配内存，然后所有操作都只使用一个缓冲区。
是循环体成为一个单独的函数进行调用。
不要用alloca，用malloc或者新的函数，但使用后需要考虑释放它，并且这样会消耗很多时间。

8. 析构函数中抛出异常是很危险的

此问题在LibreOffice项目中得到。其意思是说，让析构函数尽可能做安全的事情，如果在析构函数中抛出异常可能导致整个系统崩溃。

virtual〜LazyFieldmarkDeleter（）
{
  dynamic_cast <Fieldmark＆>
    （* m_pFieldmark.get（））。ReleaseDoc（m_pDoc）;
}

说明
这个析构函数中，在使用dynamic_cast强转时，可能会抛出异常，异常后C++调用terminate终止程序，导致崩溃。
修改
不使用引用，是指针，并进行安全判断。

virtual〜LazyFieldmarkDeleter（）
{
  auto p = dynamic_cast <Fieldmark *> m_pFieldmark.get（）;
  if（p）
    p-> ReleaseDoc（m_pDoc）;
}

建议
使析构函数尽可能简单，不要使用内存分配和文件读取。
如果有可能发生异常，可以通过catch来抑制它

virtual〜LazyFieldmarkDeleter（）
{
  try
  {
    dynamic_cast <Fieldmark＆>
      （* m_pFieldmark.get（））。ReleaseDoc（m_pDoc）;
  }
  catch（...）
  {
    assert（false）;
  }
}

这样也能避免问题。

9. 使用’\0’作为终止字符

此片段来自Notepad++项目。

TCHAR headerM [headerSize] = TEXT（“”）;
... ...
size_t Printer :: doPrint（bool justDoIt）
{
  ... ...
  if（headerM！='\ 0'）
  ... ...
}

说明
结束字符固然是正确的，但想象一下这么写

if（headerM！= 0）

这里并没有意义，因为他始终为真。但是由于程序员在这段代码中使用’\0’字符，我们假设程序员是想检查一个字符，并且需要找出字符串是否为空，但这里他犯了一个错误，就是指针和数值弄错了。
正确的代码

TCHAR headerM [headerSize] = TEXT（“”）;
... ...
size_t Printer :: doPrint（bool justDoIt）
{
  ... ...
  if（* headerM！= _T（'\ 0'））
  ... ...
}

建议
0可以表示NULL,false，空’\0’，所以请不要懒。
使用以下符号：

0 整数0
nullprt c++指针
NULL c中的空指针
‘\0’,L’\0’,_T(‘\0’)
0.0，0.0f 浮点0
false FALSE表示假。
坚持这些规则，可以让你的代码看的更清晰。

10. 避免使用多个短小的#ifdef

此片段来自CoreCLR项目。

heap_segment * gc_heap :: get_segment_for_loh（size_t size
#ifdef MULTIPLE_HEAPS
                                           ，gc_heap * hp
#endif // MULTIPLE_HEAPS
                                           ）
{
#ifndef MULTIPLE_HEAPS
    gc_heap * hp = 0;
#endif // MULTIPLE_HEAPS
    heap_segment * res = hp-> get_segment（size，TRUE）;
    if（res！= 0）
    {
#ifdef MULTIPLE_HEAPS
        heap_segment_heap（res）= hp;
#endif // MULTIPLE_HEAPS
  ....
}

说明
过多的#if不仅影响阅读，还造成混淆，让你的代码看着像是对的，其实不对。
我们展开宏看看。

heap_segment * gc_heap :: get_segment_for_loh（size_t size）
{
  gc_heap * hp = 0;
  heap_segment * res = hp-> get_segment（size，TRUE）;
  ....

声明hp，初始化null，然后。。。居然在用。。。
正确的代码
这个错误仍然存在，你可以看另一个文章在CoreCLR中的25处可疑代码
建议
尝试拒绝#ifdef，或者是#ifdef大一点。可以使用模版，当然还是要多思考。

11. 不要尝试在一行中做很多事

此来自Godot Engine项目

static real_t out（real_t t，real_t b，real_t c，real_t d）
{
  return c *（（t = t / d-1）* t * t + 1）+ b;
}

说明
看看那个t，使用时被修改。有时程序员真的很想把好几行代码写成一行，这样看的是有多简洁，但往往给自己带来麻烦。这样可能造成未定义的错误。
修改

static real_t out（real_t t，real_t b，real_t c，real_t d）
{
  t = t / d-1;
  return c *（t * t * t + 1）+ b;
}

建议
不要自作聪明，也不要试图压缩，代码可以长，可以丑，但必须可读。

12. 复制粘贴，小心最后一行

此错误出现在Source SDK中。

inline void SetX（float val）;
inline void SetY（float val）;
inline void SetZ（float val）;
inline void SetW（float val）;

inline void Init（float ix = 0，float iy = 0，
                  float iz = 0，float iw = 0） 
{
  SetX（ix）;
  SetY（iy）;
  SetZ（iz）;
  SetZ（iw）;
}

说明
去看看最后一行的效果我相信你有更多的体会。这里就不说了。
修复

{
  SetX（ix）;
  SetY（iy）;
  SetZ（iz）;
  SetW（iw）;
}

建议

布局应该让代码更突出。
复制粘贴要专注
记得检查最后一行
分享文章。

13. 表格式的代码布局

来自ReactOS项目

void adns__querysend_tcp(adns_query qu, struct timeval now) {
  ...
  if (!(errno == EAGAIN || EWOULDBLOCK || 
        errno == EINTR || errno == ENOSPC ||
        errno == ENOBUFS || errno == ENOMEM)) {
  ...
}

说明
很容易找到错误，发生这种错误的原因是，格式不好看。
正确的代码

if（！（errno == EAGAIN || errno == EWOULDBLOCK || 
      errno == EINTR || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM））{

建议
布局和表格式对齐往往是好的，但一定要谨慎，对并列的应当并列对齐，确保区间区分。
还有一个更有意思的例子

oCell._luminance = 2220 * iPixel._red +
                     7067 * iPixel._blue +
                     0713 * iPixel._green;

程序员为了代码对齐，在713前面加了个0，你可以在程序中试试，0作为第一个数字时，这个数值表示8进制。。。

static char * token_equivs1 [] =
{
  ....
  ，“KW_IF”
  ，“KW_IGNOREPAT”
  ，“KW_INCLUDES”
  ，“KW_JUMP”
  ，“KW_MACRO”
  ，“KW_PATTERN”
  ....
};

这样写，我建议慢慢习惯他，这样不仅能让你的代码看的更整齐，也能避免错误。

14. 光有好的编译器和编码风格是不够的

此来自PostgreSQL。

Datum pg_stat_get_activity(PG_FUNCTION_ARGS)
{
  ....
  if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
             sizeof(zero_clientaddr) == 0))
  ....
}

说明
右括号放错了位置导致整个语句都变了。sizeof（zero_clientaddr）== 0表达式总是false，因为任何对象的大小都>0，这导致memcmp函数比较0个字节，这样做后，返回总是0.
正确的代码

if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
           sizeof(zero_clientaddr)) == 0)

建议
有一种风格我不认同，就是yoda条件风格。他的缺点胜过他的优点。

if (0 == memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
                sizeof(zero_clientaddr)))

常量写在左边，因为它使条件不太好读，如果括放错了位置，也不会提醒我。
在讨论错误时应该遵从，代码审查，单元测试，手动测试，静态代码分析等等。

黄黄臭臭

关注

1
点赞
踩
2

收藏

觉得还不错? 一键收藏
0
评论
关于编程、重构等 42条建议上

原文来自： The Ultimate Question of Programming, Refactoring, and Everything https://software.intel.com/en-us/articles/the-ultimate-question-of-programming-refactoring-and-everything译文来自：http://blog.csdn
复制链接

扫一扫