代码审查实战：6个常见案例教你避坑与提升代码质量

作者：微信公众号：【架构师老卢】
3-1 7:54
42

我从事代码审查已有相当长的一段时间，并总结出了一些常见模式，这些模式帮助我更好地进行代码审查。根据我的经验，我整理了一份在审查任何拉取请求时关注的重点清单。

在本文中，我们将一起进行代码审查，并学习审查时需要注意的事项。我们还将讨论如何以专业和尊重的方式进行代码审查。

审查 1：发现常见错误

在审查代码时，我首先关注的是识别开发者常忽略的常见错误。让我们通过以下代码来发现一些常见错误和反模式：

public class OrderProcessor
{
    private readonly List<Order> _orders;

    public OrderProcessor(List<Order> orders)
    {
        _orders = orders;
    }

    public void ProcessOrders()
    {
        foreach (var order in _orders)
        {
            if (order.orderStatus == "Paid")
            {
                order.orderStatus = "Shipped";
                Console.WriteLine($"Order {order.order_id} is now Shipped.");
            }
            else if (order.orderStatus == "Shipped")
            {
                order.orderStatus = "Completed";
                Console.WriteLine($"Order {order.order_id} is now Completed.");
            }
        }
    }
}

public class Order
{
    public int order_id { get; set; }
    public string orderStatus { get; set; }
    public decimal TotalAmount{ get; set; }
}

你发现代码中的问题了吗？

是的！这段代码存在多个问题：

  1. 缺乏异常处理：ProcessOrders 方法没有 try-catch 块，如果出错会导致应用程序崩溃且无日志记录。
  2. 硬编码状态值：直接使用字符串（如 "Paid" 和 "Shipped"）进行状态比较，容易出错。
  3. 命名不一致：Order 类的属性命名混乱（如 order_id 是蛇形命名，orderStatus 是驼峰命名，TotalAmount 是帕斯卡命名）。

修复后的代码：

using System;
using System.Collections.Generic;

public class OrderProcessor
{
    private readonly List<Order> _orders;

    public OrderProcessor(List<Order> orders)
    {
        _orders = orders;
    }

    public void ProcessOrders()
    {
        foreach (var order in _orders)
        {
            try
            {
                // 使用枚举进行状态比较
                if (order.Status == OrderStatus.Paid)
                {
                    order.Status = OrderStatus.Shipped;
                    Console.WriteLine($"Order {order.Id} is now Shipped.");
                }
                else if (order.Status == OrderStatus.Shipped)
                {
                    order.Status = OrderStatus.Completed;
                    Console.WriteLine($"Order {order.Id} is now Completed.");
                }
                else
                {
                    Console.WriteLine($"Order {order.Id} has an unknown status: {order.Status}");
                }
            }
            catch (Exception ex)
            {
                // 正确处理并记录异常
                Console.WriteLine($"Error processing order {order.Id}: {ex.Message}");
                throw ex;
            }
        }
    }
}

public class Order
{
    // 保持一致的命名规范（帕斯卡命名）
    public int Id { get; set; }
    public OrderStatus Status { get; set; }
    public decimal TotalAmount { get; set; }
}

// 使用枚举存储状态值
public enum OrderStatus
{
    Paid,
    Shipped,
    Completed
}

审查 2：最常见的遗漏

你能审查以下代码吗？🧐

public class OrderProcessor
{
    public void GetCustomerOrderDetails(Customer customer)
    {
        if (customer.Orders.Count > 0)
        {
            foreach (var order in customer.Orders)
            {
                Console.WriteLine($"Order ID: {order.Id}, Amount: {order.Amount}");
            }
        }
        else
        {
            Console.WriteLine("No orders found.");
        }
    }
}

你发现问题了吗？

没错！缺少空值检查。开发者常常为了快速交付而忽略基础的空值检查，这可能导致意外错误。

修复后的代码：

public class OrderProcessor
{
    public void GetCustomerOrderDetails(Customer customer)
    {
        // 空值检查以避免 NullReferenceException
        if (customer == null)
        {
            Console.WriteLine("Customer is null.");
            return;
        }

        if (customer.Orders == null)
        {
            Console.WriteLine("Customer has no orders.");
            return;
        }

        // 现在可以安全访问 Orders 列表
        if (customer.Orders.Count > 0)
        {
            foreach (var order in customer.Orders)
            {
                Console.WriteLine($"Order ID: {order.Id}, Amount: {order.Amount}");
            }
        }
        else
        {
            Console.WriteLine("No orders found.");
        }
    }
}

审查 3：反模式

以下代码有什么问题？🤔

public class OrderService
{
    private readonly PaymentService _paymentService;

    public OrderService()
    {
        _paymentService = new PaymentService();
    }

    public void ProcessOrder(Order order)
    {
        _paymentService.ProcessPayment(order);
    }
}

正确！代码存在紧耦合。

OrderService 与 PaymentService 紧耦合，难以独立测试且无法轻松替换为模拟实现。

修复后的代码：

public class OrderService
{
    private readonly IPaymentService _paymentService;

    // 使用依赖注入解耦并提高可测试性
    public OrderService(IPaymentService paymentService)
    {
        _paymentService = paymentService;
    }

    public void ProcessOrder(Order order)
    {
        _paymentService.ProcessPayment(order);
    }
}

审查 4：魔法数字 ✨

准备好审查下一个代码了吗？

某 PR 中，你的同事编写了一个计算税款的方法并请你审查：

public double CalculateTax(double income)
{
    if (income <= 50000)
        return income * 0.10; // 收入 ≤ 50,000 的税率为 10%

    if (income <= 100000)
        return income * 0.15; // 收入 50,001–100,000 的税率为 15%

    if (income <= 200000)
        return income * 0.20; // 收入 100,001–200,000 的税率为 20%

    return income * 0.30; // 收入 > 200,000 的税率为 30%
}

发现问题了吗？

是的！代码中充斥着魔法数字（硬编码数值）。虽然注释让代码更清晰，但这仍然是糟糕的实现。

修复方案（将数值移至配置文件）：

public double CalculateTax(double income, IConfiguration configuration)
{
    double[] incomeSlabs = configuration.GetSection("TaxSettings:IncomeSlabs").Get<double[]>();
    double[] taxRates = configuration.GetSection("TaxSettings:TaxRates").Get<double[]>();

    for (int i = 0; i < incomeSlabs.Length; i++)
    {
        if (income <= incomeSlabs[i])
        {
            return income * taxRates[i];
        }
    }

    return income * taxRates.Last(); // 默认税率（收入超过所有区间）
}

对应的配置文件：

{
  "TaxSettings": {
    "IncomeSlabs": [50000, 100000, 200000],  // 收入区间阈值
    "TaxRates": [0.10, 0.15, 0.20, 0.30]     // 对应税率
  }
}

审查 5：DRY 原则

另一个待审查的代码：

public class DiscountCalculator
{
    public double CalculateDiscount(double amount, double discountPercentage)
    {
        double discount = amount * discountPercentage;
        double discountedPrice = amount - discount;
        return discountedPrice;
    }

    public double ApplyDiscount(double amount, double discountPercentage)
    {
        double discount = amount * discountPercentage;
        double discountedPrice = amount - discount;
        return discountedPrice;
    }
}

发现问题了吗？

是的！重复代码。相同的逻辑在多个地方重复，违反了 DRY（不要重复自己）原则。

修复后的代码：

public class DiscountCalculator
{
    public double CalculateDiscount(double amount, double discountPercentage)
    {
        return CalculateDiscountAmount(amount, discountPercentage);
    }

    public double ApplyDiscount(double amount, double discountPercentage)
    {
        double discount = CalculateDiscountAmount(amount, discountPercentage);
        return amount - discount;
    }

    private double CalculateDiscountAmount(double amount, double discountPercentage)
    {
        return amount * discountPercentage;
    }
}

审查 6：YAGNI 原则

需求背景：开发者需要实现基于预定义折扣率的订单折扣逻辑。

提交的代码：

public class OrderProcessor
{
    public void ProcessOrder(Order order, double discount)
    {
        decimal discountAmount = CalculateDiscount(order, discount);
        decimal finalAmount = order.Amount - discountAmount;

        Console.WriteLine($"Discount Applied: {discountAmount:C}");

        if (!string.IsNullOrEmpty(order.CouponCode))
        {
            decimal couponDiscount = ApplyCoupon(order.CouponCodeDiscount);
            finalAmount -= couponDiscount;
            Console.WriteLine($"Coupon {order.CouponCode} applied.");
        }
    }

    private decimal CalculateDiscount(Order order, double discount)
    {
        return order.Amount * discount;
    }

    private decimal ApplyCoupon(double couponCodeDiscount)
    {
        return order.Amount * couponCodeDiscount;
    }
}

public class Order
{
    public int Id { get; set; }
    public decimal Amount { get; set; }
    public double CouponCodeDiscount { get; set; }
}

发现问题了吗？

ApplyCoupon() 方法是多余的！开发者添加了优惠券处理逻辑，但需求中并未提及此功能。这违反了 YAGNI（你不需要它）原则。

修复后的代码：

public class OrderProcessor
{
    public void ProcessOrder(Order order, decimal discount)
    {
        decimal discountAmount = CalculateDiscount(order, discount);
        decimal finalAmount = order.Amount - discountAmount;

        Console.WriteLine($"Discount Applied: {discountAmount:C}, Final Amount: {finalAmount:C}");
    }

    private decimal CalculateDiscount(Order order, decimal discount)
    {
        return order.Amount * discount;
    }
}

public class Order
{
    public int Id { get; set; }
    public decimal Amount { get; set; }
}

代码审查的注意事项

在代码审查中，保持专业和尊重至关重要。以下是我总结的指导原则：

  1. 保持尊重：目标是改进代码，而非批评开发者。
  2. 提供建设性反馈：不要说“这不对”，而是说“如果这样做会更好，因为…”。
  3. 多提问：如果不理解某些代码，直接询问“为什么这样实现？”。
  4. 清晰具体：避免模糊反馈（如“这看起来不好”），明确指出问题。
  5. 保持平衡：既要指出问题，也要肯定代码中的优点。
  6. 开放心态：接受不同的实现方式，保持协作态度。
  7. 保持耐心：耐心解释基础问题，帮助他人成长。
  8. 避免吹毛求疵：不要过度关注对功能影响小的细节。
  9. 避免个人偏见：忽略代码风格差异（如空格 vs 制表符）。
  10. 尊重时间：提供简洁有效的反馈，确保开发者能按时完成任务。
相关留言评论
昵称：
邮箱：
阅读排行
深入WPF数据绑定：Converter...
2156
C#异步定时器：精准定期执行...
2196
WPF应用中融入ASP.NET Core...
1630
在C# .NET 8中使用MediatR实...
1340
延迟等待的艺术：C#中Task.D...
1166
WPF中轻松生成动态图表：实...
1227
C#屏幕截图：轻松捕获活动窗...
1170
.NET Core 中集成Dapper高级...
1043
串口通信利器：RJCP.DLL.Ser...
1069
.NET 性能分析：.NET 8 中的...
942
C#中轻松实现二维码和条形码...
950
C# 中的并发性：使用 Blocki...
905
轻松上传：C#中HTTP POST mu...
1132
C#串口通讯实战：基础功能及...
1002
解决WPF界面卡死等待问题：...
2080
ASP.NET 后端开发：全面的路...
767
还在到处加异常日志记录？试...
879
精准数字处理：C#中四舍五入...
1097
WPF中轻松操控GIF动画：WpfA...
862
.NET —ToList 和 ToArray ...
712
打造真实感十足的速度表盘：...
826
.net中OpenCVSharp+Zxing读...
824
实现WPF多语言支持：简单灵...
819
C#中Task.Run怎么用？能解决...
804
在 .NET8 中通过 RabbitMQ实...
665
.NET 9中ToList 和 ToArray ...
633
深入了解SOAPCore：在.NET 6...
754
掌握ImageSharp：图像处理的...
743
Microsoft.Extensions.Loggi...
648
MediatR让进程内通信如此简...
1831
轻松实现.NET应用自动更新：...
713
解密C# Socket通信：克服粘...
706
解密C#中允许HttpClient使用...
594
FluentFTP实战：轻松操控FTP...
703
.NET 中的 UUID 和 ULID：最...
567
C# 中的弱事件 — 如何避免...
564
深入理解WPF中的Dispatcher...
678
C#文件编码检测：轻松解析文...
565
掌握System.Text.Json中的多...
666
WPF神器揭秘：一键截图，轻...
538
WPF魔法：轻松实现依赖注入...
1764
ASP .NET Core 8+ 中的全局...
511
使用OpenXML在.NET 6中导出E...
623
ASP.NET Core .NET 8 中间件...
488
WPF开发新境界：MVVM设计模...
610
C# Socket通信：灵活传输DTO...
592
WPF大展示专业指南：轻松实...
695
.NET配置文件大揭秘：轻松读...
571
深入FileSystemWatcher：C#...
682
实用指南：C#中优雅更新GUI...
550