糟糕的编码实践
1.98/5 (24投票s)
糟糕的编码实践
引言
在现实世界中，获胜者是优秀者；而在编程世界里，优秀的编程才能获胜。开发者在查看他人或自己的代码时，常常会摇头叹气，甚至爆出一些不文明的脏话，这并非不常见。这很多时候，甚至可以说是所有时候，都是由于糟糕的编程实践所致。
虽然程序员普遍认为标准非常重要，但在遵循哪些标准上存在分歧。（不）幸运的是，编程并非一门科学，而至今仍然是一门艺术。我相信，在可预见的未来，如果不是永远的话，它都将保持艺术的地位。等到那一天到来时，我们所能做的最好的事情就是遵循一些良好的编程实践。
我认为，列出好的编程实践（这已经被计算机学者、项目负责人和质量保证小组不厌其烦地讨论过了）毫无意义，我只想列出我曾经遇到或听说过的糟糕的编码实践。这里的讨论大多局限于 C 和 C++ 编程。虽然其中一些概念可以应用于其他语言，但本文档中的讨论主要集中在 C 和 C++。
并非所有程序员都能在各种场景下进行编程。因此，我想维护一个场景列表以及这些场景下的糟糕编程实践。俗话说，“傻瓜从自己的经验中学习，聪明人从他人的经验中学习”。我曾愚蠢地实践了一些糟糕的编程实践，并从我的错误中吸取了教训。
1. 哎呀！！！这不是面向对象！
我曾多次屈服于此。以过程式的方式编写程序的诱惑有时太强大，令人难以抗拒，我因此暴露了类的内部细节，要么将成员变量设为 public，要么更糟，将它们分组为结构体而不是类。
在我继续深入之前，让我明确一点：我并非反对过程式编程。这是我们开始学习编程的方式，也是将业务逻辑想象成代码的最简单方式之一。而且，它要简单得多！承认自己是过程式程序员并不丢人。
但是，由于某个固执的项目负责人、坚决的项目经理或顶层那个该死的疯狂的梦想家，我们被迫使用面向对象编程语言编写程序。此时，世界大战就此开始。
面向对象不仅仅是一组类。它远比类这个简单的课本定义（数据和相关函数的集合）要广阔得多。如果我们只遵循这个定义，那么任何软件包中的所有函数都可以被视为相关的，并且都可以成为单个类的成员函数。这或许可行。经过一些额外的努力，它确实也可行。但恕我直言，这并非面向对象编程。
例如，请看下面的程序。这是典型的
面向对象过程式混乱的垃圾。
示例 1A
#include <stdio.h>
class CSingleClass
{
int i,j;
public:
void Execute()
{
int k=0;
scanf("%d%d",&i,&j);
}
};
void main()
{
CSingleClass o;
o.Execute();
}
从语法上讲，上面的程序是面向对象的。但这仅仅是因为它无法在 C 编译器中编译。从语义上讲，这个程序离所谓的面向对象编程相去甚远。上面的例子相当简单，我希望您还在跟着我。现在，让我们看一个稍微棘手一些的例子，我曾见过比我更有经验的程序员屈服于过程式编程的诱惑。
示例 1B
假设我有一个处理矩形的类，如下所示。开发几个月后，需要支持平行四边形。有时会诱使您只需为现有的矩形类添加一个枚举或标志，然后也将其用于平行四边形。
typedef struct sPoint
{
double x; double y;
} sPoint:
class CRectangle
{
sPoint m_BottomLeft;
sPoint m_TopRight;
public:
void Area();
};
通过少量修改，此类可用于平行四边形：
class CRectangle
{
sPoint m_BottomLeft;
sPoint m_TopRight;
double m_dInclusiveAngle;
bool m_bIsParallelogram;
public:
void Area()
{
if (m_bIsParallelogram)
{.. .. ..}
else
{.. .. ..}
}
}
这确实有效。它为您节省了很多时间，因为您无需编写设置/获取点的代码等。函数越多，您节省的编码时间就越多。它甚至可能让您的项目负责人称赞您快速完成了工作。但是，恕我直言，这既不是面向对象的，也不是过程式的。这是一种烹饪不当的意大利面条。您为避免代码重复节省的每分钟，维护这些代码所需的时间都远远超过了。
通过仔细分析设计，我相信通过最少的代码重复，可以完全避免这种意大利面条代码。
注意：我不是概率论学生。因此，我承认使用“完全”一词的傲慢。
2. 枚举还是 #defines？
我们都曾在编程中需要使用某些数组。很多时候，我们也知道我们感兴趣的对象是哪个索引。我们很容易争辩说：“这个索引不是用户可以修改的。为什么我不能硬编码这个数字？”
我见过的最好的例子是使用 Grid 或 Table 进行 UI 编程。假设我们正在一个表格中收集用户信息，如下所示：
| 用户名（姓，名） | 用户 ID | 出生日期 | 出生城市 | 婚姻状况 |
| 汤姆·克鲁斯 | tcruise | 1962 年 7 月 3 日 | 纽约州锡拉丘兹 | 已婚 |
那么我们可以安全地使用：
示例 2A
Table[RowNumber][0] = strUserName;
Table[RowNumber][1] = nUserId;
Table[RowNumber][2] = DOB;
Table[RowNumber][3] = strCity;
Table[RowNumber][4] = bIsSingle?”Single”:”Married”;
这肯定会奏效，似乎没有理由不这样做。但原因是，总会有一个愚蠢的用户希望“婚姻状况”排在“出生城市”之前。他甚至愿意为此付费。现在想象一下为这一个用户的安装进行此更改。想象一下将 3 替换为 2，反之亦然的开发工作。噩梦还在继续测试。一个“1”错误地替换为“2”或未替换的“3”将导致灾难，让您无法入睡。
当一个字段是下拉列表时（例如，婚姻状况可以是单身或已婚），这会变得更加棘手。那时，选择的顺序对用户来说可能并不重要。但然后您的项目经理会产生一个新颖的想法，提供“单身并寻觅”、“已婚但有兴趣”等选项。瞧！上述噩梦已经变成现实。
要尽量减少此类情况麻烦的一种安全方法是使用 #define 或 enum 来表示列号或下拉框条目等。考虑相同的情况，但现在使用 #defines。
示例 2B
#define COL_USER_NAME 0
#define COL_USER_ID 1
#define COL_DOB 2
#define COL_CITY 3
#define COL_MARITAL_STATUS 4
Table[RowNumber][COL_USER_NAME] = strUserName;
Table[RowNumber][COL_USER_ID] = nUserId;
Table[RowNumber][COL_DOB] = DOB;
Table[RowNumber][COL_CITY] = strCity;
Table[RowNumber][COL_MARITAL_STATUS] = bIsSingle?”Single”:”Married”;
现在，更改列的顺序仅限于更正 `#define` 中的列号。其余的将自动处理。
推论
随着时间的推移，我发现自己倾向于使用 `Enum` 而不是 `#define`。枚举具有独特的优势，它们会自动维护自己的编号。
使用 `#define` 时，重新排序意味着我们不仅需要记住它们的出现顺序，还需要确保编号得到维护。即，如果用户要求 DOB 作为第一个字段，ID 作为最后一个字段，则示例 2B 中显示的 `#define` 将变为：
示例 2C
#define COL_USER_NAME 1
#define COL_USER_ID 4
#define COL_DOB 0
#define COL_CITY 3
#define COL_MARITAL_STATUS 2
如果列数很多，比如 20 列，那么在修改代码时，维护所有数字会令人困惑。您可能需要进行 3-4 次调试会话才能获得正确的顺序。`Enum` 提供了一种更好的方法。
考虑使用 Enums 重写示例 2B：
示例 2D
enum{
COL_USER_NAME = 0,
COL_USER_ID,
COL_DOB,
COL_CITY,
COL_MARITAL_STATUS
};
Table[RowNumber][COL_USER_NAME] = strUserName;
Table[RowNumber][COL_USER_ID] = nUserId;
Table[RowNumber][COL_DOB] = DOB;
Table[RowNumber][COL_CITY] = strCity;
Table[RowNumber][COL_MARITAL_STATUS] = bIsSingle?”Single”:”Married”;
通过声明为 enum，COL_USER_ID 和其他项会自动由编译器分配一个值。该值可用作数组的索引。
现在，更改列的顺序就像吃蛋糕一样简单。更改 Enum 中的相对位置即可完成。
示例 2E
enum{
COL_DOB,
COL_USER_NAME,
COL_MARITAL_STATUS,
COL_CITY,
COL_USER_ID
};
Table[RowNumber][COL_USER_NAME] = strUserName;
Table[RowNumber][COL_USER_ID] = nUserId;
Table[RowNumber][COL_DOB] = DOB;
Table[RowNumber][COL_CITY] = strCity;
Table[RowNumber][COL_MARITAL_STATUS] = bIsSingle?”Single”:”Married”;
注意：将“单身”、“已婚”等字符串硬编码到代码中是一个明显的坏习惯。我在这里只为清晰起见才这样做。
3. 宏还是内联函数？
为什么 C/C++ 程序员乐于编写宏？当然，宏提供了编写可重用代码的便捷方式。但它们也有改变上下文的副作用。滥用宏可能导致意想不到的后果。请看下面的例子。宏会检查返回值并在各种情况下抛出异常。
示例 3A
#define CheckValueForError \
{ \
if(i+j-k!=0) \
{ \
value = value/(i+j-k); \
} \
if(value>1) \
throw exception; \
} \
你能重用这个宏吗？当然可以！但重用有多容易？这是一个有争议的问题。该宏的先决条件是变量 i、j、k 和 value 在宏调用的上下文中已声明，并且它们具有适当的值。此外，宏会更改这些变量的值。如果由于任何原因变量名冲突，则无法使用此宏。宏执行完成时，您无法确定宏是否完成了它应该做的事情。
这些问题的解决方案是使用内联函数。上面的同一个宏可以重写为一个函数，如下所示：
示例 3B
bool inline CheckValueForError(int i, int j, int k, float value)
{
bool bFlag = true;
if (i + j - k != 0)
{
value = value / (i + j - k);
bFlag = true;
}
if (value > 1)
throw exception;
return bFlag;
}
虽然您需要显式提供需要检查的变量作为输入参数，但这实际上是有帮助的。宏的所有上述弊端都将随着 `inline` 函数而消失。说函数有堆栈维护的开销在此处无效，因为这是一个 `inline` 函数。
那么，宏是伪装的恶魔吗？不是。它们不是。它们有自己的用途。如果您希望在调用上下文中执行更改，并且此更改需要在代码的多个地方执行，那么请使用宏。例如，您想要一段交换两个变量值的代码。对于许多不同的数据类型，需要一遍又一遍地编写相同的算法，而且您无法使用函数重载，或者函数重载是过度的。那么宏是唯一的出路。宏也适用于实现“单一入口，单一出口”编程的干净而精简的实现。稍后我将在我的论述中进一步讨论这一点。
如果您仔细观察，我在上面稍微欺骗了您以提出我的观点。我给出的糟糕宏的例子没有任何输入参数。当我提到了宏作为好方法的说法时，我指的是带输入参数的宏。在使用带输入参数的宏时，我之前归因于内联函数的优势也可以应用于宏。尽管如此，内联函数仍然有一个显著的优势：它们可以在调试器中单步跟踪！
当调用宏时，预处理器会处理宏，宏代码会在源程序中展开。展开后的源程序写入一个名为中间文件（Intermediate file）的文件中，而这个中间文件将被编译器处理。在存储在 PDB 文件中的调试信息中，中间文件中的所有这些代码行都链接到源 C/C++ 文件中编写宏的单行。因此，您会看到调试器将宏调用视为一个语句。
这种行为既有帮助也有麻烦。当宏调用工作正常，并且您不需要检查宏内部的代码流程时，这是好的。但如果您正在调试某个问题，并怀疑宏本身存在 bug，那么很难查看宏内部。
就像一切事物一样，这句话也有反驳的理由。当然可以指示编译器保存中间文件并在调试器中使用它，这样您就可以看到展开后的代码。但问题是，中间文件包含的代码是按照编译器容易理解的方式编写的。它包含您使用的 SDK、您或您的同事编写的各种宏以及您使用的各种库的展开代码。在最好的情况下，这些代码可以很容易地理解。有时，此中间文件可能非常晦涩，以至于您可能会发现很难找到涉及业务逻辑的代码。
4. 单一入口，多重出口
单一入口，单一出口是结构化编程（请勿将结构化编程与过程式编程混淆）的一个重要原则。
单一入口，单一出口编程表示函数/过程应只有一个入口点和一个出口点。在所有代码流的情况下，即成功、失败、错误或异常，控制必须从一个点进入过程，然后从另一个点退出。不应有其他入口或出口。
我很难想象在现代语言中如何编写一个具有多个入口点的过程。但编写具有多个出口点的代码是一种非常普遍的做法。例如，考虑下面的程序，它查找平方根：
示例 4A
bool SquareRoot(double dValue, double& dSquareRoot)
{
if (dValue < 0)
{
return false;
}
dSquareRoot = pow(dValue, 0.5);
return true;
}
类似的编码非常常见。这是一个单一入口，多重出口程序。现在，让我们考虑这个函数的一个单一入口，单一出口的对应版本。
示例 4B
bool SquareRoot(double dValue, double dSquareRoot)
{
bool bRetVal = false;
if (dValue < 0)
{
dSquareRoot = NAN;
bRetVal = false;
}
else
{
dSquareRoot = pow(dValue, 0.5);
bRetVal = true;
}
return bRetVal;
}
虽然这是一个专门选择的简单紧凑的例子，但在现实生活中，编程很少如此简单。当过程需要在嵌套循环中退出时，该循环深度达 3 个 `for()` 循环。那么在各处都有多个 return 语句是好是坏？请看下面，计算 `tan(arcsine())` 对于一个数字数组。
示例 4C
bool TanArcSine(double* pValues, int nLength, double** ppResult)
{
if (nLength < 0)
return false;
*ppResult = new double[nLength];
for (int inx=0; inx <= nLength; inx++)
{
if ((-1 <= pValues[inx]) && (pValues[inx] <= 1))
{
*ppResult[inx] = asin(pValues[inx]);
if (fabs(*ppResult[inx]) != M_PI / 2)
{
*ppResult[inx] = tan(*ppResult[inx]);
}
else
{
return false;
}
}
else
{
return false;
}
}
return true;
}
考虑使用单一入口，单一出口机制编写的相同程序。
示例 4D
bool TanArcSine(double* pValues, int nLength, double** ppResult)
{
bool bRetVal = false;
if (nLength < 0)
bRetVal = false;
else
{
*ppResult = new double[nLength];
for (int inx=0; inx < nLength; inx++)
{
if ((-1 <= pValues[inx]) && (pValues[inx] <= 1))
{
*ppResult[inx] = asin(pValues[inx]);
if (fabs(*ppResult[inx]) != M_PI / 2)
{
*ppResult[inx] = tan(*ppResult[inx]);
}
else
{
bRetVal = false;
break;
}
}
else
{
bRetVal = false;
break;
}
}
bRetVal = true;
}
return bRetVal;
}
令人惊讶的是，许多程序员认为编写单一入口，多重出口程序更容易、更安全、更紧凑、更可靠等等。他们通常提供的论点是——“当不再需要时，为什么还要携带额外的负担？”看看示例 4A 和 4B 中的程序，人们倾向于同意他们的观点。而且他们这样做也不能被指责。当遇到错误条件时执行过程，确实没有任何增值。
但是，作为一个负责任的程序员，我们的责任是确保每当我们的函数失败时，它都能优雅地失败。没有人应该抱怨它失败。为此，我发现单一出口点概念非常有帮助。
正如您可以想象的那样，过程被编写来完成一些真正复杂任务，例如将数据写入数据库表，进入关键区域并在多线程/多进程系统中以线程安全的方式执行某些操作。现在，当我们从过程中返回时，我们必须执行诸如提交或回滚数据库、释放互斥锁以及退出关键任务等操作。可能还需要做一些事情，例如关闭文件等。
如果存在多个出口点，那么这些出口任务需要在所有这些出口点执行。在大多数情况下，这将意味着生硬的代码重复。正如您非常清楚的那样，代码重复是一种坏习惯。
相反，如果我们维护一个单一出口点，那么由于只有一个出口，所有出口任务都将集中在代码的一个位置。这直接反映在代码质量上。因此，通过一点额外的负担，我们可以避免潜在的代码重复，并使我们的代码保持顶级质量。
我在这里不会如此傲慢地仅凭我自己的想法就称单一入口，多重出口为糟糕的编码实践。陪审团仍在审理此案。在我看来，单一入口，单一出口是一种好的实践，我所有剩余的讨论都基于这种编程风格。
无论幸运与否，C 和 C++ 都没有内置支持单一出口编程。如果您想采用这种编程方式，那么您必须制定精心准备的标准并严格遵守它们。不用说，这非常主观，并且因品味而异。C# 在 `try catch finally` 块中对此提供了一定程度的支持。但除此之外，您就只能靠自己了。
5. 资源泄露
最容易犯的错误之一是在遇到错误条件时忘记释放分配的内存。在示例 6A 和 6B 中，每当执行 return false 语句时，分配给 ppResult 的内存都没有被释放。作为一个负责任的程序员，过程有责任释放所有未使用的资源。我们不能期望调用者检查返回值并释放所有内存。如果 ppResult 恰好是局部变量，那么调用者就无法释放该内存。
Java、C# 和 .Net 对 C++ 的扩展引入了垃圾收集器来处理这种情况。但随后，可能还有其他资源，如打开的文件、网络套接字或信号量等，必须释放。没有内置支持来处理这些类型的资源。
大多数时候，程序员会释放资源。但他们只在成功的情况下释放。当采用单一入口，多重出口时，有时某些资源将不会在错误情况下被释放。这可能会堵塞资源，使系统瘫痪。如果您采用单一入口，单一出口的编码风格，那么解决此问题将变得容易一些。在出口点，您需要编写释放资源的代码，并且在所有情况下，即成功、失败、异常或错误，您都可以确保您的资源已被释放。
考虑以下示例
示例 5A
bool TanArcSine(double* pValues, int nLength, double** ppResult)
{
bool bRetVal = false;
if (nLength < 0)
bRetVal = false;
else
{
*ppResult = new double[nLength];
for (int inx=0; inx < nLength; inx++)
{
if ((-1 <= pValues[inx]) && (pValues[inx] <= 1))
{
*ppResult[inx] = asin(pValues[inx]);
if (fabs(*ppResult[inx]) != M_PI / 2)
{
*ppResult[inx] = tan(*ppResult[inx]);
}
else
{
bRetVal = false;
break;
}
}
else
{
bRetVal = false;
break;
}
}
bRetVal = true;
}
if (false == bRetVal)
{
if(*pResult)
delete *pResult;
*pResult = NULL;
}
return bRetVal;
}
世界上没有免费的午餐。在这种情况下，代码质量是以额外的负担为代价的。在我看来，额外的负担是值得的。
6. 声名狼藉的 goto
有多少人喜欢 `goto`？有多少人认为 `goto` 是坏的？有多少人*相信* `goto` 是坏的？当您看到一个人使用 `goto` 编写代码时，您会怎么说？有了 `break`、`continue` 和 `return` 语句，几乎总是可以不使用 goto 来编写代码。但是，goto 对于实现单一入口，单一出口编程风格非常容易和简单。
考虑以下示例
示例 6A
bool TanArcSine(double* pValues, int nLength, double** ppResult)
{
bool bRetVal = false;
if (nLength < 0)
goto error_exit;
*ppResult = new double[nLength];
for (int inx=0; inx < nLength; inx++)
{
if ((1 < 1))
goto error_exit;
*ppResult[inx] = asin(pValues[inx]);
if (fabs(*ppResult[inx]) == M_PI / 2)
goto error_exit;
*ppResult[inx] = tan(*ppResult[inx]);
}
bRetVal = true;
goto success_exit;
error_exit:
bRetVal = false;
if(*ppResult) delete *ppResult;
*ppResult = NULL;
success_exit:
return bRetVal;
}
这段代码肯定比示例 7 中的代码更容易理解，不是吗？唯一的 `goto` 语句是用于 `success_exit` 或 `error_exit`。不允许使用任何其他 `goto` 语句。我知道许多人使用代码质量检查工具来捕获所有其他 `goto` 用法。从而以最少的额外变量开销和保持编程的便捷性来实现单一入口，单一出口。在这些情况下，非常受控地使用 `goto` 也许是一种可行的权衡。所以下次您的质量保证团队因为您使用 `goto` 而对您大喊大叫时，问问他们更喜欢什么。是看起来干净的代码，所有资源都已释放，只有两个 `goto` 语句作为开销；还是一个陷入 `if then else` 语句的代码，难以编写、理解和维护，但没有 `goto`。
如果看到 `goto` 语句让您流泪，那么试试这个。与示例 6A 相同的程序，但稍微重写以使用宏。这是我发现宏不仅方便，而且必不可少的地方。
示例 6B
#define IF_TRUE_EXIT(expr) \
{ \
if(expr) \
goto error_exit; \
} \
#define SUCCESS_EXIT goto success_exit;
bool TanArcSine(double* pValues, int nLength, double** ppResult)
{
bool bRetVal = false;
IF_TRUE_EXIT(nLength < 0);
*ppResult = new double[nLength];
for (int inx=0; inx < nLength; inx++)
{
IF_TRUE_EXIT((1 < -1));
*ppResult[inx] = asin(pValues[inx]);
IF_TRUE_EXIT(fabs(*ppResult[inx]) == M_PI / 2);
*ppResult[inx] = tan(*ppResult[inx]);
}
bRetVal = true;
SUCCESS_EXIT;
error_exit:
bRetVal = false;
if(*ppResult) delete *ppResult;
*ppResult = NULL;
success_exit:
return bRetVal;
}
7. 返回值
我作为事后想法添加了这一部分。这一部分的目的仅仅是提醒人们检查函数调用的返回值。即使您确定函数不可能失败，最好也要检查返回值。在编码方面，您越谨慎越好。
假设您调用的函数失败了。但是该函数忘记释放为输出指针分配的内存。（可能是因为被调用函数不是单一入口，单一出口）。现在您不检查返回值，而是继续访问输出参数。这可能导致内存访问冲突或溢出等。这是一个完美的黑客攻击。听起来遥不可及？再想想。
结论
糟糕的编码实践可以就此结束吗？不。我不这么认为。我的大脑没有足够的空间来学习所有的编码实践。我的大脑中绝对没有足够的计算能力来判断它们所有并将其分类为好或坏。我为我在 C、C++ 编码方面的经验感到自豪，我开始列出我发现的糟糕实践。其中大多数糟糕实践曾经由我遵循。我尽最大努力现在不再犯这些错误，但这是我一直未能企及的完美。我想在学习到新东西时不断更新这份文档。