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

上一篇：关于编程、重构等 42条建议 上
下一篇：关于编程、重构等 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.net/huanglong8/article/details/56666790

其实内容是比较多的，并且我也是通过google翻译进行快速查看，好了，这里缩短字符，让大家更快的吸收。

15. 在代码中使用枚举

项目来自SourceSDK，问题是Reason == PUNTED_BY_CANNON.

enum PhysGunPickup_t{  PICKED_UP_BY_CANNON,  PUNTED_BY_CANNON,  PICKED_UP_BY_PLAYER,};enum PhysGunDrop_t{  DROPPED_BY_PLAYER,  THROWN_BY_PLAYER,  DROPPED_BY_CANNON,  LAUNCHED_BY_CANNON,};void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason){  ....  if( Reason == PUNTED_BY_CANNON )  {    PlayPuntSound();   }  ....}

简述
这里意思是说，不要直接使用枚举的内容，而是应该把枚举类型带上，因为编译器不知道是什么和什么比较，并且枚举在标准C中也不是安全的，所以这样写也比较容易混淆，作者试图修改正确如下：

if( Reason == LAUNCHED_BY_CANNON ){  PlayPuntSound(); }

但我们看这段代码是挺怪的，其实就是换了内容，但仍然做的不好，因为如果这个项目你接收了，你永远不知道表达的是什么意思。所以作者建议应该这么写：

enum class PhysGunDrop_t{  DROPPED_BY_PLAYER,  THROWN_BY_PLAYER,  DROPPED_BY_CANNON,  LAUNCHED_BY_CANNON,};void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason){  ....  if( Reason == PhysGunDrop_t::LAUNCHED_BY_CANNON )  {    PlayPuntSound();   }  ....}

好了，其实也是根据个人习惯养成的，尤其是写了很长一段的C，用了大量的宏，可能短时间内难以改变，但必须要慢慢改变。

16. 看还能怎么做，同样不要压缩一行

这个来自KDE4，试图写A=（B==C）

void LDAPProtocol::del( const KUrl &_url, bool ){  ....  if ( (id = mOp.del( usrc.dn() ) == -1) ) {    LDAPErr();    return;  }  ret = mOp.waitForResult( id, -1 );  ....}

说明
其实这样写有时候并不会出错，因为我们在使用fopen等函数时，都会这么用，但这并不是一个好习惯。比较级的优先级高于赋值，所以先执行比较，并且将1.0赋值给变量。
修改

id = mOp.del(usrc.dn());if ( id == -1 ) {Recommendation

建议
这样很难么？很丑么？当然，作为译者我也经常这么写，但至少我会加括号。。。

17. 使用提供的功能清除私人数据

来自Apache，memset和RtlSecureZeroMemory都用来清除缓冲区。可偏偏用了memset。

static void MD4Transform(  apr_uint32_t state[4], const unsigned char block[64]){  apr_uint32_t a = state[0], b = state[1],               c = state[2], d = state[3],               x[APR_MD4_DIGESTSIZE];    ....  /* Zeroize sensitive information. */  memset(x, 0, sizeof(x));}

说明
作者说编译器在编译的过程中，可能会删除那些即使不调用也不会影响程序的代码，如果删除了memset，那么缓冲区就不会被清空。但这个也只是编译器理论，有兴趣可以参考：

• 安全么？你测试？
• 安全清除私人数据
• 编译器会删除memset函数，刷新缓冲区。
• C中零内存的注意事项
• 编译器优化

修复

memset_s（x，sizeof（x），0，sizeof（x））;orRtlSecureZeroMemory（x，sizeof（x））;

VS从C11开始提供了这个函数，你们可以试试。如果有必要，你可以创建自己的安全性的函数。

errno_t memset_s(void *v, rsize_t smax, int c, rsize_t n) {  if (v == NULL) return EINVAL;  if (smax > RSIZE_MAX) return EINVAL;  if (n > smax) return EINVAL;  volatile unsigned char *p = v;  while (smax-- && n--) {    *p++ = c;  }  return 0;}

void secure_zero(void *s, size_t n){    volatile char *p = s;    while (n--) *p++ = 0;}

18. 用一种编程语言的知识不完全适用于另一种语言

来自Rutty项目，大致意思是说不要用C语言的想法去完成Java的工作。

static void tell_str(FILE * stream, char *str){  unsigned int i;  for (i = 0; i < strlen(str); ++i)    tell_char(stream, str[i]);}

这代码一目了然，不解释了。但是，这样的东西经常出现在Pascal或Delphi代码中，因为循环终止的条件仅计算一次，所以没关系，可。。。C等低级语言可不这么聪明了。

program test;var  i   : integer;  str : string;function pstrlen(str : string): integer;begin  writeln('called');  pstrlen := Length(str);end;begin  str := 'a pascal string';  for i:= 1 to pstrlen(str) do     writeln(str[i]);end.

修改

static void tell_str(FILE * stream, char *str){  size_t i;  const size_t len = strlen(str);  for (i = 0; i < len; ++i)    tell_char(stream, str[i]);}

多思考就好，优化也很简单。。。

19. 正确的调用构造函数

来自LibreOffice项目

Guess::Guess(){  language_str = DEFAULT_LANGUAGE;  country_str = DEFAULT_COUNTRY;  encoding_str = DEFAULT_ENCODING;}Guess::Guess(const char * guess_str){  Guess();  ....}

说明
程序员因为懒就这么写了。但这就出错了，因为临时的函数，临时的变量，完了好全部销毁。
修改

Guess::Guess(const char * guess_str){  Guess();  ....}orthis->Guess();

这个例子不见得好，因为也很危险，可能导致微妙的错误，这样的调用也不是合适的。

class SomeClass{  int x, y;public:  SomeClass() { new (this) SomeClass(0,0); }  SomeClass(int xx, int yy) : x(xx), y(yy) {}};

代码安全，工作良好，因为只包含了简单的数据类型，不会造成任何危险。
这有一个例子，显式调用构造函数造成错误。

class Base { public:  char *ptr;  std::vector vect;  Base() { ptr = new char[1000]; }  ~Base() { delete [] ptr; } }; class Derived : Base {   Derived(Foo foo) { }   Derived(Bar bar) {      new (this) Derived(bar.foo);   }  Derived(Bar bar, int) {      this->Derived(bar.foo);   }}

所以我们应该 “new（this）Derived（bar.foo）;” 或“this-> Derived（bar.foo）”。
建议
C++11允许构造函数调用其他并行的构造函数。例如：

Guess::Guess(const char * guess_str) : Guess(){  ....}

好吧，这是特性。

• 维基 优化对象结构
• C++11常见问题 委托构造函数
• MSDN 统一初始化

20. EOF检查可能不够

来自SETI@home项目

template <typename T>std::istream &operator >>(std::istream &i, sqlblob<T> &b) {  ....  while (!i.eof())   {    i >> tmp;    buf+=(tmp+' ');  }  ....}

说明
嗯，译者也是这么写的，有什么不对，因为在流对象读写中，eof确实是判断结束符号，但是如果流一旦反生错误，那这里就死着么。我们应该是用bad，fail来检查流的状态。
修复

template <typename T>std::istream &operator >>(std::istream &i, sqlblob<T> &b) {  ....  while (i >> tmp)   {    buf+=(tmp+' ');  }  ....}

建议
不仅要使用eof检查结束，还应该使用bad，fail检查状态。如果使用bool类型进行判断，那也是正确的。

21. 检查文件结束字符是否真的EOF

此来自Computational Network Toolkit，EOF不应该与char类型值做比较，’c’应该是整型

string fgetstring(FILE* f){  string res;  for (;;)  {    char c = (char) fgetc(f);    if (c == EOF)      RuntimeError("error reading .... 0: %s", strerror(errno));    if (c == 0)      break;    res.push_back(c);  }  return res;}

EOF声明方式是

#define EOF（-1）

他是整型的，fgetc也是返回int的值，它可以返回0-255或-1，读取255时，就是变成-1，因为0XFF么，所以是错误的。
例如在Windows 1251代码页中，俄语字母表的最后一个字母具有0XFF代码，因此读到这里就结束了。
修复

for (;;){  int c = fgetc(f);  if (c == EOF)    RuntimeError("error reading .... 0: %s", strerror(errno));  if (c == 0)    break;  res.push_back(static_cast<char>(c));}

建议
这里没啥建议，就是不要太快的去转换类型。

22. 不要使用 #pragma warning(default:X)

来自ToroiseGIT项目，#pragma warning（default：X）使用不正确，应该使用#pragma warning（push / pop）

#pragma warning(disable:4996)LONG result = regKey.QueryValue(buf, _T(""), &buf_size);#pragma warning(default:4996)

说明
假定用disable关闭警告，用default恢复进行告警，但事实不是这样，因为如果使用编译器开关/Wall，这种状态下，default是不会重新显示此警告的，因为默认关闭。
修复

#pragma warning(push)#pragma warning(disable:4996)LONG result = regKey.QueryValue(buf, _T(""), &buf_size);#pragma warning(pop)

建议
用这个括起来，会比较好，具体参考
Pragma警告指令
还有一个好文章， 你想在VC++中屏蔽警告。

23. 让程序自己算长度

来自OpenSSL库，说strncmp的第三个参数和第二个参数所表达的意思不一样。

if (!strncmp(vstart, "ASCII", 5))  arg->format = ASN1_GEN_FORMAT_ASCII;else if (!strncmp(vstart, "UTF8", 4))  arg->format = ASN1_GEN_FORMAT_UTF8;else if (!strncmp(vstart, "HEX", 3))  arg->format = ASN1_GEN_FORMAT_HEX;else if (!strncmp(vstart, "BITLIST", 3))  arg->format = ASN1_GEN_FORMAT_BITLIST;else  ....

说明
作者还是强调不要复制粘贴，以免出错，这里就不说了。
修改

else if（！strncmp（vstart，“BITLIST”，strlen（“BITLIST”）））

但是作者又说也不好，因为谁知道编译器会不会优化这个strlen。。。然后又说使用宏。。。最后建议，自己写一个函数，来整。。。呵呵了了。。。

24. 不要覆盖虚函数

来自MFC

class CWnd : public CCmdTarget {  ....  virtual void WinHelp(DWORD_PTR dwData,                       UINT nCmd = HELP_CONTEXT);  ....};class CFrameWnd : public CWnd {  ....};class CFrameWndEx : public CFrameWnd {  ....  virtual void WinHelp(DWORD dwData,                       UINT nCmd = HELP_CONTEXT);  ....};

说明
当覆盖一个虚函数时，很容易产生错误，他不会以任何方式与基类中的函数相关联。可能会有如下错误：

• 另一种类型进行重写
• 重写的函数参数表变了
• 覆盖函数在const下是不同的
• 基类函数不是虚函数。
  修复

class CFrameWndEx : public CFrameWnd {  ....  virtual void WinHelp(DWORD_PTR dwData,                       UINT nCmd = HELP_CONTEXT) override;  ....};

建议
Override 说该函数需要覆盖
Final 说该函数不需要重写

不要把this和nullptr做比较

来自CoreCLR，因为在较新的编译器中，this永远不会为NULL

bool FieldSeqNode::IsFirstElemFieldSeq(){  if (this == nullptr)    return false;  return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;}

建议
作者也没有建议什么，只是说 不要用而已，但目前有规范说这样使用是不对的，但仍然有很多应用都这么用了，对于新的编译器来说，只是发出了警告，希望今后慢慢修改。

26. 阴险的VARIANT_BOOL

来自NAME项目，VARIANT_BOOL的真值被定义为-1.

virtual HRESULT __stdcall  put_HandleKeyboard (VARIANT_BOOL pVal) = 0;....pController->put_HandleKeyboard(true);

说明
当老子离开某家软件开发公司时，恨不得 #define true false, #define false true，然后在一键替换所有的自然值。是不是。。。同样，这个项目令人难受，他的真值是-1，来看看定义：

ypedef short VARIANT_BOOL;#define VARIANT_TRUE（（VARIANT_BOOL）-1）#define VARIANT_FALSE（（VARIANT_BOOL）0）

虽然非0都表示真值，但这种误解仍然存在很多问题。
修复及建议

pController-> put_HandleKeyboard（VARIANT_TRUE）;

当你试图用某些框架时，一定要谨慎，至少分清里面的宏都是啥啥的。当使用HRESULT类型时，不要忘记：

#define S_OK（（HRESULT）0L）#define S_FALSE（（HRESULT）1L）

27. 狡诈的BSTR类型

来自VirtualBox项目，wchar_t*类型字符串错误的转换成BSTR类型，应该考虑使用SysAllocString函数。

....HRESULT EventClassID(BSTR bstrEventClassID);....hr = pIEventSubscription->put_EventClassID(                    L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}");

下面是BSTR类型声明

typedef wchar_t OLECHAR;typedef OLECHAR * BSTR;

作者表示这两种类型所存储的字符结构是不一样的，BSTR是有长度前缀的，并且不包括终止空字符。如果
BSTR MyBstr = L”I am a happy BSTR”;
BSTR MyBstr = SysAllocString(L”I am a happy BSTR”);
在调试时，运行这两个进行内存比较是不一样的，所以应该区分这种用法。
检查wchar_t*传递给BSTR的静态代码分析
BSTR转std::string std::wstring，反之不行。
BSTR和CString转换
修复就不说了，用函数即可。
建议
如果你看到一个未知类型，先看源代码声明，在去找文档，弄清楚再使用。

28. 用简单函数，不要用宏

来自ReactOS项目，逻辑不对，缺少代码。

#define stat64_to_stat(buf64, buf)   \    buf->st_dev   = (buf64)->st_dev;   \    buf->st_ino   = (buf64)->st_ino;   \    buf->st_mode  = (buf64)->st_mode;  \    buf->st_nlink = (buf64)->st_nlink; \    buf->st_uid   = (buf64)->st_uid;   \    buf->st_gid   = (buf64)->st_gid;   \    buf->st_rdev  = (buf64)->st_rdev;  \    buf->st_size  = (_off_t)(buf64)->st_size;  \    buf->st_atime = (time_t)(buf64)->st_atime; \    buf->st_mtime = (time_t)(buf64)->st_mtime; \    buf->st_ctime = (time_t)(buf64)->st_ctime; \int CDECL _tstat(const _TCHAR* path, struct _stat * buf){  int ret;  struct __stat64 buf64;  ret = _tstat64(path, &buf64);  if (!ret)    stat64_to_stat(&buf64, buf);  return ret;}

说明
这代码有点长，如果宏展开应该是

if (!ret)  buf->st_dev   = (&buf64)->st_dev;buf->st_ino   = (&buf64)->st_ino;buf->st_mode  = (&buf64)->st_mode;

这显然是错的，这样写宏不仅冗长，还会造成问题
修复

#define stat64_to_stat(buf64, buf)   \  do { \    buf->st_dev   = (buf64)->st_dev;   \    buf->st_ino   = (buf64)->st_ino;   \    buf->st_mode  = (buf64)->st_mode;  \    buf->st_nlink = (buf64)->st_nlink; \    buf->st_uid   = (buf64)->st_uid;   \    buf->st_gid   = (buf64)->st_gid;   \    buf->st_rdev  = (buf64)->st_rdev;  \    buf->st_size  = (_off_t)(buf64)->st_size;  \    buf->st_atime = (time_t)(buf64)->st_atime; \    buf->st_mtime = (time_t)(buf64)->st_mtime; \    buf->st_ctime = (time_t)(buf64)->st_ctime; \  } while (0)

建议
我表示译者也是dowhile用，但。。。其实并不好。
宏的特点是，难以调试，容易出错，难以理解，我对宏有偏见。
函数多好，代码简单，可靠，安全。如果说缺点，就是优化吧。。。
在作者认为，代码应该这么写：

static void stat64_to_stat(const struct __stat64 *buf64,                           struct _stat *buf){  buf->st_dev   = buf64->st_dev;  buf->st_ino   = buf64->st_ino;  buf->st_mode  = buf64->st_mode;  buf->st_nlink = buf64->st_nlink;  buf->st_uid   = buf64->st_uid;  buf->st_gid   = buf64->st_gid;  buf->st_rdev  = buf64->st_rdev;  buf->st_size  = (_off_t)buf64->st_size;  buf->st_atime = (time_t)buf64->st_atime;  buf->st_mtime = (time_t)buf64->st_mtime;  buf->st_ctime = (time_t)buf64->st_ctime;}

嗯好吧，作者意思就是不要宏，这可不是译者的意思。