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

郑子铭

发布于 2025-04-11 14:12:33

22400

代码可运行

文章被收录于专栏：DotNet NB && CloudNativeDotNet NB && CloudNative

运行总次数：0

代码可运行

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

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

审查 1：发现常见错误

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

public classOrderProcessor
{
    privatereadonlyList<Order> _orders;

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

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

publicclassOrder
{
    publicint order_id {get;set;}
    publicstring orderStatus {get;set;}
    publicdecimal TotalAmount{get;set;}
}

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

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

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

修复后的代码：

using System;
usingSystem.Collections.Generic;

publicclassOrderProcessor
{
    privatereadonlyList<Order> _orders;

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

    publicvoidProcessOrders()
    {
        foreach(var order in _orders)
        {
            try
            {
                // 使用枚举进行状态比较
                if(order.Status == OrderStatus.Paid)
                {
                    order.Status = OrderStatus.Shipped;
                    Console.WriteLine($"Order {order.Id} is now Shipped.");
                }
                elseif(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;
            }
        }
    }
}

publicclassOrder
{
    // 保持一致的命名规范（帕斯卡命名）
    publicint Id {get;set;}
    publicOrderStatus Status {get;set;}
    publicdecimal TotalAmount {get;set;}
}

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

审查 2：最常见的遗漏

你能审查以下代码吗？🧐

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

你发现问题了吗？

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

修复后的代码：

public classOrderProcessor
{
    publicvoidGetCustomerOrderDetails(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 >)
        {
            foreach(var order in customer.Orders)
            {
                Console.WriteLine($"Order ID: {order.Id}, Amount: {order.Amount}");
            }
        }
        else
        {
            Console.WriteLine("No orders found.");
        }
    }
}

审查 3：反模式

以下代码有什么问题？🤔

public classOrderService
{
    privatereadonlyPaymentService _paymentService;

    publicOrderService()
    {
        _paymentService =newPaymentService();
    }

    publicvoidProcessOrder(Order order)
    {
        _paymentService.ProcessPayment(order);
    }
}

正确！代码存在紧耦合。

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

修复后的代码：

public classOrderService
{
    privatereadonlyIPaymentService _paymentService;

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

    publicvoidProcessOrder(Order order)
    {
        _paymentService.ProcessPayment(order);
    }
}

审查 4：魔法数字 ✨

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

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

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

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

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

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

发现问题了吗？

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

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

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

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

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

对应的配置文件：

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

审查 5：DRY 原则

另一个待审查的代码：

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

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

发现问题了吗？

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

修复后的代码：

public classDiscountCalculator
{
    publicdoubleCalculateDiscount(double amount,double discountPercentage)
    {
        returnCalculateDiscountAmount(amount, discountPercentage);
    }

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

    privatedoubleCalculateDiscountAmount(double amount,double discountPercentage)
    {
        return amount * discountPercentage;
    }
}

审查 6：YAGNI 原则

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

提交的代码：

public classOrderProcessor
{
    publicvoidProcessOrder(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.");
        }
    }

    privatedecimalCalculateDiscount(Order order,double discount)
    {
        return order.Amount * discount;
    }

    privatedecimalApplyCoupon(double couponCodeDiscount)
    {
        return order.Amount * couponCodeDiscount;
    }
}

publicclassOrder
{
    publicint Id {get;set;}
    publicdecimal Amount {get;set;}
    publicdouble CouponCodeDiscount {get;set;}
}

发现问题了吗？

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

修复后的代码：

public classOrderProcessor
{
    publicvoidProcessOrder(Order order,decimal discount)
    {
        decimal discountAmount =CalculateDiscount(order, discount);
        decimal finalAmount = order.Amount - discountAmount;

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

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

publicclassOrder
{
    publicint Id {get;set;}
    publicdecimal Amount {get;set;}
}