关于编程、重构等 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））;
or
RtlSecureZeroMemory（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();
  ....
}
or
this->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;
}

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