代码度量、代码坏味和重构实践
4.40/5 (14投票s)
在本文中，我将介绍我们的团队如何使用度量来识别代码坏味，并应用重构来纠正这些代码坏味。这个例子很简单，但它准确地展示了极限编程的一些实践如何帮助我们的团队维护代码。
引言
我们的团队使用极限编程实践来管理一个大型零售连锁店的关键任务系统的开发。我们没有采用所有实践，但使用了其中的大部分。
这是我们使用的实践列表
- 测试驱动开发 (VSTS 测试工具)
- 集体代码所有权
- 编码规范 (FxCop, 代码度量)
- 持续集成 (每日构建，但我们很快就会实现CI环境)
- 规划游戏 (迭代规划和每日站会)
- 客户测试 (Fit, VSTS Web Tests)
- 小版本发布 (每两周发布一次UAT)
每天早上的站会，团队负责人将报告夜间集成构建的结果。
此报告包含以下度量
- 单元测试通过、失败、忽略
- Fit测试通过、失败、忽略、异常
- 测试覆盖率 (应高于80%)
- 圈复杂度 (应低于10)
- 代码指令数 (应低于100)
- FxCop规则验证
每天，度量都会与前一天进行比较，项目经理会跟踪这些度量来了解项目的整体健康状况。
背景
发现此示例代码的组件是一个面向服务应用程序中的业务流程Web服务。该组件的平均圈复杂度为8，并且在几周内一直保持不变。然后，该组件的复杂度突然跳升到17。参见图1显示的代码度量。
什么是圈复杂度？
以下精彩介绍由Samudra Gupta在Java Boutique上完成。该度量由Thomas McCabe引入，用于衡量方法的结构复杂度。
假设您有一个实现类，它变得非常庞大或过于复杂而无法维护。或者，您有一个类充当整个业务层的控制类，但其中嵌入了过多的业务逻辑。或者，您又被交接了一个包含过多代码重复的类。这些情况被称为“复杂性”。
学习如何使用这三个度量可以帮助您指导正确的重构步骤
- 圈复杂度
- 类的响应
- 类的加权方法数
圈复杂度 (CC) = 决策点数 + 1
决策点是条件语句，如if/else, while等。考虑以下示例
public void isValidSearchCriteria( SearchCriteria s )
{
if( s != null )
{
return true;
}
else
{
return false;
}
}
在上面的示例中，CC=2+1=3 (一个if + 一个else + 1)。
圈复杂度对代码的可测试性和可维护性有巨大影响。如示例所示，如果要测试isValidSearchCriteria()方法，您需要编写三个测试来验证它：一个用于方法本身，两个用于方法内的决策点。显然，如果CC值增加，并且任何方法内的决策点数量增加，这意味着测试该方法需要更多的精力。
下表总结了CC值在代码测试和维护方面的影响
|
CC 值 |
风险 |
|
1-10 |
低风险程序 |
|
11-20 |
中等风险 |
|
21-50 |
高风险 |
|
>50 |
最复杂且高度不稳定的方法 |
识别代码问题
现在，让我们回到具有中等风险复杂性的方法。此时，我想提及一些您在审视可疑方法时应该考虑的事情
- 代码意图和清晰度 – 易于理解和弄清楚方法应该做什么。
- 代码坏味 – 识别面向对象代码中常见的设计问题。
这是该方法
/// <summary>
/// Validate UI Request and throw a business exception if there is any error
/// </summary>
/// <param name="request">The business process request.</param>
/// <param name="customerIDFieldLength">Length of the Customer field.</param>
/// <param name="productFieldLength">Length of the product field.</param>
/// <returns>Returns true if the Request was validated, else false.</returns>
private bool ValidateRequest( CustomerInquiryRequest request,
int customerIDFieldLength, int productFieldLength )
{
if( request.CustomerProduct.ProductNumber != null &&
request.Customer.CustomerID != null )
{
if( request.CustomerProduct.ProductNumber != string.Empty &&
request.Customer.CustomerID != string.Empty ){
// Both were populated
throw new BusinessException(
HandledErrors.InvalidBothParameterMessage );
}
else if( request.Customer.CustomerID == string.Empty &&
request.CustomerProduct.ProductNumber == string.Empty ) {
// if objects were instantiated but not populated
throw new BusinessException(
HandledErrors.CustomerEmptyMessage );
}
else if( request.Customer.CustomerID != string.Empty ){
// Note: ProductNumber was equal
// to string.empty Check Customer ID length
if( request.Customer.CustomerID.Length >
customerIDFieldLength ){
throw new BusinessException(
HandledErrors.CustomerInvalidLengthMessage );
}
}else{
// Note: CustomerID was equal
// to string.empty check Product Length
if( request.CustomerProduct.ProductNumber.Length >
productFieldLength ){
throw new BusinessException(
HandledErrors.ProductInvalidLengthMessage );
}
}
}else if( request.CustomerProduct.ProductNumber == null &&
request.Customer.CustomerID == null ){
// Both were null
throw new BusinessException( HandledErrors.CustomerEmptyMessage );
}else if( request.CustomerProduct.ProductNumber == null ){
// ProductNumber was null and CustomerID was not null
if( request.Customer.CustomerID.Length >
customerIDFieldLength ){
throw new BusinessException(
HandledErrors.CustomerInvalidLengthMessage );
}
}else{ // ProductNumber not null and CustomerID was null
// Check Product Length
if( request.CustomerProduct.ProductNumber.Length >
productFieldLength ) {
throw new BusinessException(
HandledErrors.ProductInvalidLengthMessage );
}
}
// Set objects below with formatted data i.e PadLeft
if( request.Customer.CustomerID != null ){
request.Customer.CustomerID = request.Customer.CustomerID.PadLeft(
customerIDFieldLength,
Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
}
if( request.CustomerProduct.ProductNumber != null ){
request.CustomerProduct.ProductNumber =
request.CustomerProduct.ProductNumber.PadLeft(
productFieldLength,
Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
}
return true;
}
当我查看这个方法时，首先发现的是以下代码坏味
- 长方法 – 方法越长，越难看清它在做什么。目前，我个人倾向于让方法不超过十行代码。如果超过十行，我宁愿重构并分解该方法。
- 重复代码.
- 注释 – 应该只用于澄清“为什么”，而不是“是什么”。注释会很快变得冗长并降低代码的清晰度。
- 死代码 – 未被使用的变量、参数、方法、代码片段、类等。
我也对该方法在做什么感到非常困惑。这里进行了什么验证？代码意图不明确。经过几分钟的仔细研究，我才弄清楚从UI传递到业务流程Web服务的请求有两个参数需要检查。
建立了以下规则
- 如果两者都为null或空字符串，则抛出业务异常。
- 如果两者都被填充，则抛出业务异常。
- 如果仅填充了CustomerID，则检查参数的最大长度是否被违反。
- 如果仅填充了ProductNumber，则检查参数的最大长度是否被违反。
- 我发现的最后一件事是，该方法返回一个布尔值，但不可能返回false值，因此返回任何内容都是多余的。
解决已识别的问题
为了解决可疑方法的问题，我主要使用了提取方法作为重构，并应用了.NET 2.0中的一些新功能来减少对null和空字符串的检查。
这是重构后的代码
- 主 `ValidateRequest` 方法
/// <summary> /// Validates the customer inquiry request. /// </summary> /// <param name="request">The customer inquiry request.</param> /// <param name="customerFieldLength">Length of the customer field.</param> /// <param name="productFieldLength">Length of the product field.</param> private void ValidateRequest( CustomerInquiryRequest request, int customerFieldLength, int productFieldLength ) { // 1. Check both parameters are not null and not empty strings CheckCustomerInquiryNotNullOrEmpty( request ); // 2. Check both parameters aren't populated CheckCustomerInquiryNullOrEmpty( request ); // 3. Check CustomerID for field length and pad the parameter CheckCustomerIDValid( request, customerFieldLength ); // 4. Check ProductNumber for field length and pad the parameter CheckProductNumberValid( request, productFieldLength ); }
- 检查两个参数是否都不为null且不为空字符串的方法
/// <summary> /// Checks the customer inquiry not null or empty. /// </summary> /// <param name="request"> customer inquiry request.</param> /// <remarks>If both customer id and product number is not null, then /// we throw a customer empty message business exception.</remarks> private void CheckCustomerInquiryNotNullOrEmpty( CustomerInquiryRequest request ) { // Check both parameters are not null or empty string if( !string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) && !string.IsNullOrEmpty( request.Customer.CustomerID ) ) { // Both were populated throw new BusinessException( HandledErrors.InvalidBothParameterMessage ); } }
- 检查两个参数是否均未被填充的方法
/// <summary> /// Checks the customer inquiry null or empty. /// </summary> /// <param name="request">The customer inquiry request.</param> /// <remarks> /// If both customer id and product number is null, then we /// throw a customer empty message business exception.</remarks> private void CheckCustomerInquiryNullOrEmpty( CustomerInquiryRequest request ) { if( string.IsNullOrEmpty( request.Customer.CustomerID ) && string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) ) { // Both are null or empty string throw new BusinessException( HandledErrors.CustomerEmptyMessage ); } }
- 检查 `CustomerID` 字段长度并用零填充参数以达到正确长度的方法
/// <summary> /// Checks the customer ID valid. /// </summary> /// <param name="request">The customer inquiry request.</param> /// <param name="customerIDFieldLength">Length of /// the customer ID field.</param> /// <remarks>Due to the Product number being null, /// we are working with the customer id</remarks> private void CheckCustomerIDValid( CustomerInquiryRequest request, int customerIDFieldLength ) { if( string.IsNullOrEmpty( request.Customer.CustomerID ) ) { // Check Customer ID length if( request.Customer.CustomerID.Length > customerIDFieldLength ) { throw new BusinessException( HandledErrors.CustomerInvalidLengthMessage ); } // Pad the left of the customer id request.Customer.CustomerID = request.Customer.CustomerID.PadLeft( customerIDFieldLength, Convert.ToChar( "0", CultureInfo.CurrentCulture ) ); } }
- 检查 `ProductNumber` 字段长度并用零填充参数以达到正确长度的方法
/// <summary> /// Checks the product number valid. /// </summary> /// <param name="request">The customer inquiry request.</param> /// <param name="productFieldLength">Length of /// the product field.</param> /// <remarks>Due to the customer id being null, /// we are working with the product number</remarks> private void CheckProductNumberValid( CustomerInquiryRequest request, int productFieldLength ) { if( string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) ) { // Check Product Length if( request.CustomerProduct.ProductNumber.Length > productFieldLength ) { throw new BusinessException( HandledErrors.ProductInvalidLengthMessage ); } // Pad the left of the product number request.CustomerProduct.ProductNumber = request.CustomerProduct.ProductNumber.PadLeft( productFieldLength, Convert.ToChar( "0", CultureInfo.CurrentCulture ) ); } }
关注点
当你查看重构后的 `ValidateRequest` 方法时，你会注意到的第一件事是，仅通过阅读方法名称就可以轻松理解该方法试图实现的目标。编写自文档化且易于阅读的代码并不难，它只需要投入。
你会发现
- 其他人将更喜欢与你的代码一起工作
- 这会让你看起来更专业，因为你似乎关心其他人接触代码
- 这会帮助你写出更好的代码，因为它迫使你清楚地定义你在做什么
接下来要注意的是，所有方法都很小且易于理解。因此，项目中的每个人都可以轻松地隔离错误并扩展功能。
由于降低了复杂性，可测试性也得到了提高。重构后的代码现在可能有五个方法，而之前只有一个方法，但最高复杂度现在是4。这有助于我们更容易地隔离每个代码路径，并降低未测试代码的风险，这通常有助于提高组件的代码测试覆盖率。
结论
我认为极限编程实践的好处是显而易见的。也许，最有利的是项目成员之间持续的反馈和沟通。像Fit和测试驱动开发、重构和持续集成这样的其他实践对于保持代码最高质量至关重要。
也许，如果你看看这个具体的例子，如果我们没有每天在站会上通过代码度量报告来获得关于代码健康状况的反馈循环，那么上述这样的问题就会被忽略，技术债务就会累积，直到代码库变得庞大而脆弱，以至于没有人愿意再做任何改动。