بازبینی کد (Code Review)

مشکل: «به نظر خوب است»

در فروشگاه اینترنتی ما، یک برنامه‌نویس قابلیت «کد تخفیف» را اضافه کرده است. یک Pull Request با ۹۰۰ خط تغییر می‌فرستد.

دو بازبین آن را می‌بینند:

1. بازبین اول پنج دقیقه نگاه می‌کند و می‌نویسد «به نظر خوب است». تأیید.
2. بازبین دوم بیست نظر می‌نویسد: فاصله اضافه، ترتیب using ها، اسم یک متغیر.

یک هفته بعد، یک کد تخفیف ده درصدی هزار بار استفاده می‌شود. سقف استفاده هیچ وقت چک نمی‌شد. هیچ کدام از دو بازبین این را ندید.

هر دو بازبین وقت گذاشتند. ولی هیچ کدام به سؤال اصلی نگاه نکرد: آیا این کد درست کار می‌کند؟

هدف Code Review چیست؟

بازبینی کد چند هدف دارد. به ترتیب اهمیت:

1. پیدا کردن مشکل قبل از کاربر. باگ منطقی، مشکل امنیتی، حالت‌هایی که فراموش شده‌اند.
2. سالم نگه داشتن طراحی. کد در جای درست است؟ یک سال بعد هم قابل تغییر است؟
3. پخش دانش در تیم. حالا دست‌کم دو نفر این بخش را می‌شناسند.
4. یک سبک مشترک. کد همه تیم شبیه هم باشد.

ظاهر کد را به ابزار بسپار

بحث درباره فاصله و ترتیب خط‌ها، وقت آدم‌ها را هدر می‌دهد و فقط حال همه را بد می‌کند. ماشین این کار را بهتر و بی‌طرف انجام می‌دهد:

1. فایل editorconfig. قانون‌های سبک کد را یک جا برای همه تیم می‌نویسد.
2. دستور dotnet format. کد را بر اساس همان قانون‌ها مرتب می‌کند. در CI می‌شود فقط چک کرد که کد مرتب است یا نه.
3. تحلیل‌گرهای کد (Analyzers). هشدارهای کامپایلر و قانون‌های کیفیت را در زمان ساخت نشان می‌دهند. می‌شود هشدارهای مهم را خطا کرد تا ساخت شکست بخورد.
4. تست‌ها در CI. اگر تست‌ها شکست خورده‌اند، آدم هنوز وقتش را نگذارد.

<!-- Directory.Build.props: same rules for every project -->
<Project>
  <PropertyGroup>
    <Nullable>enable</Nullable>
    <AnalysisLevel>latest-recommended</AnalysisLevel>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  </PropertyGroup>
</Project>

حالا بازبین آزاد است تا روی چیزهایی فکر کند که ماشین نمی‌فهمد.

یک بازبینی واقعی

این بخش اصلی همان Pull Request کد تخفیف است. قبل از خواندن ادامه، خودت نگاه کن. چند مشکل می‌بینی؟

public async Task<decimal> ApplyDiscount(Guid orderId, string code)
{
    var order = await _db.Orders.FindAsync(orderId);
    var discount = await _db.Discounts.FirstAsync(d => d.Code == code);

    order.Total = order.Total - order.Total * discount.Percent / 100;
    discount.UsedCount++;

    await _db.SaveChangesAsync();
    return order.Total;
}

یک بازبین خوب از پایه هرم شروع می‌کند:

1. درستی. تاریخ انقضا و سقف استفاده چک نمی‌شود. همان باگ هزار بار استفاده.
2. درستی. اگر کاربر دو بار دکمه را بزند، تخفیف دو بار کم می‌شود.
3. درستی. اگر کد تخفیف وجود نداشته باشد، متد FirstAsync خطا می‌دهد و کاربر خطای 500 می‌بیند، نه پیام «کد نامعتبر است».
4. امنیت. هیچ چکی نیست که این سفارش مال همین کاربر باشد. هر کسی با شناسه سفارش، روی سفارش دیگران تخفیف می‌گذارد.
5. همزمانی. اگر دو مشتری همزمان آخرین ظرفیت کد را استفاده کنند، هر دو موفق می‌شوند. افزایش UsedCount بدون کنترل همزمانی است.
6. طراحی. قانون تخفیف داخل سرویس است و جمع سفارش از بیرون تغییر می‌کند. قانون باید داخل خود سفارش باشد.
7. تست. هیچ تستی برای انقضا، سقف و دو بار اعمال نیست.
8. جزئی. اسم متد پسوند Async ندارد و CancellationToken نمی‌گیرد.

بعد از بازبینی، کد این شکل را پیدا می‌کند:

public async Task<IResult> ApplyDiscountAsync(
    Guid orderId, string code, string customerId, CancellationToken ct)
{
    var order = await db.Orders.SingleOrDefaultAsync(
        o => o.Id == orderId && o.CustomerId == customerId, ct);
    if (order is null)
        return Results.NotFound();

    var discount = await db.Discounts.SingleOrDefaultAsync(d => d.Code == code, ct);
    if (discount is null || !discount.CanBeUsed(time.GetUtcNow()))
        return Results.BadRequest("The discount code is not valid.");

    order.ApplyDiscount(discount);   // throws if the order already has a discount
    discount.MarkUsed();             // UsedCount is a concurrency token

    await db.SaveChangesAsync(ct);   // a second, parallel use fails here
    return Results.Ok(order.Total);
}

چطور نظر بنویسیم؟

نظر خوب سه چیز دارد: مشکل، دلیل و اهمیت. درباره کد حرف بزن، نه درباره آدم.

یک برچسب کوچک اول هر نظر، کار نویسنده را خیلی راحت می‌کند. بعضی تیم‌ها برای این کار قالبی به اسم Conventional Comments دارند:

1. مانع ادغام. باید قبل از ادغام درست شود.
2. پیشنهاد. فکر می‌کنم بهتر است، ولی تصمیم با نویسنده است.
3. سؤال. نفهمیدم. شاید مشکل نباشد.
4. جزئی (nit). خیلی کوچک. اگر نخواستی، درستش نکن.
5. تعریف. کار خوبی دیدی؟ بگو. بازبینی فقط پیدا کردن اشکال نیست.

کار نویسنده: بازبینی را آسان کن

بازبینی خوب از نویسنده شروع می‌شود.

1. تغییر کوچک. هر Pull Request یک ایده داشته باشد. تغییر اسم‌ها را جدا بفرست.
2. توضیح روشن. چه چیزی تغییر کرد؟ چرا؟ چطور تست شد؟ لینک کار مربوط را بگذار.
3. اول خودت مرور کن. قبل از فرستادن، تغییرها را یک بار مثل یک بازبین بخوان. خیلی از اشکال‌ها همین‌جا پیدا می‌شوند.
4. جاهای حساس را نشان بده. «لطفاً به منطق همزمانی دقت کن. مطمئن نیستم.»

چک‌لیست بازبین

1. رفتار. کد کاری را می‌کند که توضیح می‌گوید؟ حالت‌های مرزی چه؟ ورودی خالی، تکراری، خیلی بزرگ؟
2. خطا. اگر سرویس دیگر جواب ندهد چه می‌شود؟ خطا بی‌صدا خورده می‌شود؟
3. امنیت. ورودی کاربر چک می‌شود؟ کاربر فقط به داده خودش می‌رسد؟ Secret در کد هست؟
4. داده و همزمانی. دو درخواست همزمان چه می‌کنند؟ تراکنش درست است؟ Migration روی جدول بزرگ چه اثری دارد؟
5. کارایی. کوئری داخل حلقه هست؟ داده بیشتر از لازم خوانده می‌شود؟
6. طراحی. کد در جای درستی است؟ یک چیز تکراری ساخته شده که قبلاً وجود داشت؟
7. تست. رفتار مهم تست دارد؟ تست واقعاً شکست می‌خورد اگر کد غلط باشد؟
8. خوانایی. کسی که فردا این را می‌خواند، می‌فهمد؟ اسم‌ها به زبان کسب‌وکار هستند؟

اشتباه‌های رایج