iT邦幫忙

2026 iThome 鐵人賽

DAY 7
0
Software Development

諸神也搖頭的 Legacy Code: 30天 .NET 工程師生存之道系列 第 7

Day 07 -「龍牙戰士」客戶已經火大了,怎麼寫測試? (上)

  • 分享至 

  • xImage
  •  

https://ithelp.ithome.com.tw/upload/images/20260806/20182564QEKpaW239K.png

卡德摩斯是腓尼基的王子,他的妹妹歐羅巴被化身成白牛的宙斯帶走後,父親便叫他去把妹妹找回來,而且找不到就不准回家。

於是卡德摩斯找了很久,始終沒有歐羅巴的消息,最後實在不知道該往哪裡走,只好跑去問德爾菲神諭

沒想到神直接叫他別找了:

「去找一頭從來沒有拉過車、耕過田的母牛,然後跟著牠走,牠在哪裡停下來,你就在那裡蓋一座城」。

卡德摩斯雖然搞不懂這和尋找妹妹有什麼關係,但還是乖乖照著神諭的話去做...

他找到母牛一路跟著牠走,最後來到一片平原,母牛走到一半停了下來,直接趴在地上休息,卡德摩斯心想:「看來就是這裡了。」

他準備把母牛獻給諸神,於是派幾名同伴去附近的泉水取水,結果等了很久一個人都沒有回來。

卡德摩斯只好自己去看看,這才發現泉水旁住著一條巨大的龍蛇,他派來取水的同伴全都已經被牠殺死了...卡德摩斯一氣之下,拿起武器就衝了上去,雙方打了好一陣子,才終於把龍蛇殺死。

就在他看著地上的屍體,不知道接下來該怎麼辦時,雅典娜突然出現對他說:

「把牠的牙齒拔下來,種進土裡」。

這個要求聽起來很奇怪,但既然是女神親自交代,卡德摩斯也只能照做,於是他把龍牙一顆顆拔下來,再像播種一樣埋進土裡。

沒過多久,地面突然開始晃動。

最先冒出來的是長矛,接著是頭盔,然後是一名又一名全副武裝的戰士。

這些人不需要成長,也不用接受訓練,從土裡爬出來的那一刻就殺氣騰騰,隨即彼此就打了起來,最後只剩下五人。

活下來的人最後協助卡德摩斯建立底比斯,也被視為當地幾個古老家族的祖先。

希臘人稱他們為 Spartoi,意思是「被播種的人」。

PS: 這麼容易就放棄妹妹的嗎?!


時間緊迫

正當我們還在因為前兩天學會一連串技巧感到自豪時,PM 急急忙忙跑了過來:

「客戶早上跑第一批訂單時,有間門市原本要訂 50 箱飲料,結果多打一個 0,變成 500 箱,是因為今天來的實習生少一根筋,倉庫差點就出貨了,現在客戶已經先把自動處理停掉了。」

「下午還有幾千多張訂單等著處理,都是他打的,不可能叫人逐張檢查,一直停著的話今天所有門市的補貨都會延誤。」

PM 講完,回去想了一下...

「原本功能超過信用額度的訂單,本來就會進主管的待審清單,流程應該不用重做,現在只需要再加上同一商品超過 100 件,或整張訂單超過 10 萬元,也丟進原本的待審清單,一個小時內能不能先做出可以驗證的版本?」

於是我們自信的打開程式碼,看到幾百行的 ProcessOrder,看著螢幕思考一下,心想:

完了,測試要怎麼補?


兩個選項都不太想選

當下似乎只有兩個選擇:

  • A :直接多一個判斷塞進 ProcessOrder,反正三年來都這樣加的,多一段也不會死。
  • B :先把 ProcessOrder 補完整測試再說,不過有幾百行,等我完成客戶可能已經殺到公司了。

A 方案爽快、B 方案看起來完整,可是我們兩個都不想選,B 是讓 PM 直接去跟主管告狀,這時候學了什麼都沒用了,因為會被主管罵到下次再也不敢了,興致勃勃地學了一些東西準備要帶著專案一起變好了,結果卻被現實潑了冷水,這誰還想要學習啊,當然開始厭世啊!

那看來是選 A 囉?當然不是啊!如果是的話那現在就可以馬上打開 Netflix 開始追劇了 XD

當然這也牽扯到 PM 的能力,有沒有辦法在第一線就幫我們擋著,啊沒有的話就...也沒關係,因為接下來的兩天就是要來看一些辦法也許可以幫助我們在這之中取得平衡。

https://ithelp.ithome.com.tw/upload/images/20260806/20182564Sn1u11wDVN.jpg
節錄自網路

魚與熊掌無法兼得的場景到處都是,而且只會越來越多,許多問題當下沒有唯一最好的解法,既要快又要安全,就得暫時放下「順手把舊的也修一遍」的想法,大家都想讓爛東西趕快變好,但這時候越急,就越容易壞事。

所以我的建議是:

不一定要先整修那間老房子。

如果老房子已經塞到沒有空間,再往裡面擴建只會變成違建,也許可以在旁邊蓋一間小工具房:

水電獨立
設備完整
規格清楚

最後再從老房子開一扇門接過去,老房子還是那間老房子,但新功能在新空間裡跑,而且那裡可以鋪上感測器、裝上監視器,也就是說,可以被測試

這就是今天要學的,先不急著重構舊程式碼,但讓新加入的程式碼有測試。


開門第一招

Sprout 的字面意思是「萌芽」,把新邏輯當作一顆種子,從舊 code 旁邊長出來,自己長成一個獨立的 method。

所以今天要學的招式就是 新生方法(Sprout Method)

而這招可以幫我們:

沒辦法立刻為舊 method 寫測試,但至少能讓新加進去的那段邏輯有測試。

我自己覺得最大的價值在於「策略性後退」,承認現在沒有資源大改舊 method,但同時也避免讓新邏輯也跟著一起爛掉。


一樣先觀察

今天要處理的就是這個 ProcessOrder,當我們初次打開程式碼的時候,會發現裡面密密麻麻的,所以一樣我們還是得先做「觀察」,於是我們在茫茫程式碼海中找到了有關信用審查的程式碼,那看來就是要在附近做處理了:

public class OrderProcessor
{
    public void ProcessOrder(Order order)
    {
        // ... 前面還有數百行舊邏輯,包含訂單金額計算 ...

        var reviewReasons = new List<OrderReviewReason>();

        if (new CustomerDAL().ExceedsCreditLimit(
                order.Customer.Id,
                order.FinalAmount))
        {
            reviewReasons.Add(OrderReviewReason.CreditLimitExceeded);
        }

        if (RequestReviewWhenNeeded(order, reviewReasons))
        {
            return;
        }

        // ... 後面還有數百行舊邏輯 ...
    }
}

這個 method 前後還有數百行程式碼,礙於篇幅就不全部貼出來了,程式碼範例中可以看到完整內容,因為當前我們沒有時間把整支 method 的縫都鑿開,更不可能先把流程從頭測一遍,所以我們要挑重要的看,也同時要確保它們對於前後流程的影響程度。

經過我們大概定位後,我們發現有個 reviewReasons 會收集送審原因,RequestReviewWhenNeeded 則負責把訂單交給既有的主管審核流程。

而在收集送審原因的流程中,目前只會去看信用額度,透過 GetAvailableCredit 取得客戶目前還能使用的額度,且尚未結清的訂單也會在裡面直接扣掉,因此這裡會去比較本次訂單金額:

private bool ExceedsCreditLimit(int customerId, decimal orderAmount)
{
    var availableCredit = GetAvailableCredit(customerId);
    return orderAmount > availableCredit;
}

private decimal GetAvailableCredit(int customerId)
{
    var creditLimit = GetCreditLimit(customerId);
    var unpaidOrderAmount = GetUnpaidOrderAmount(customerId);

    return creditLimit - unpaidOrderAmount;
}

接著,負責送審的 RequestReviewWhenNeeded 中會先整理原因,再只留下尚未核准的原因:

private bool RequestReviewWhenNeeded(
    Order order,
    IEnumerable<OrderReviewReason> reviewReasons)
{
    var reasons = reviewReasons
        .Distinct()
        .ToList();

    if (reasons.Count == 0)
    {
        return false;
    }

    var reviewDal = new OrderReviewDAL();
    var approvedReasons = reviewDal.GetApprovedReasons(
        order.Id,
        order.Version);

    var reasonsToReview = reasons
        .Except(approvedReasons)
        .ToList();

    if (reasonsToReview.Count == 0)
    {
        return false;
    }

    order.Status = OrderStatus.PendingReview;
    reviewDal.CreateRequest(order, reasonsToReview);
    return true;
}

GetApprovedReasons 會用訂單 ID 與版本查詢,訂單沒有變更、而且原因已經核准時,會回傳 false 讓它繼續走完 ProcessOrder,若訂單版本已變更,就沒有相符的核准紀錄,會建立新的送審申請,CreateRequest 則沿用既有交易,一次保存訂單狀態、版本與送審原因。

還挺複雜的對吧?不過經過我們的梳理過後,還是可以發現 PendingReview 狀態、核准人與核准版本,以及核准後重新排回處理佇列,都是原本就有的功能,而重新處理時 RequestReviewWhenNeeded 會認得同一版本訂單已核准的原因,不會把訂單再次攔下,如果訂單內容在核准後被修改,舊流程則會讓那次核准失效,這並不影響我們新增新的機制,所以我們確定 RequestReviewWhenNeeded 是不用來動了。


向外擴張

因為新規則需要的商品、數量與單價都已經在 order.Items 裡,所以我們不必再查 DB,但如果直接把分組、加總和兩段判斷直接寫進 ProcessOrder 的話,要測一個邊界值仍然得走過數百行流程,不是完全不想測,而是沒辦法為了這次改動先測完整支巨大的 method

所以既然我們已經知道預計的呼叫地點了,就先寫成註解,再透過新生方法把異常判斷往外長出去:


if (new CustomerDAL().ExceedsCreditLimit(
    order.Customer.Id,
    order.FinalAmount))
{
    reviewReasons.Add(OrderReviewReason.CreditLimitExceeded);
}

// reviewReasons.AddRange(FindAbnormalOrderReasons(order));

if (RequestReviewWhenNeeded(order, reviewReasons))
{
    return;
}

這裡有個值得注意的地方,FindAbnormalOrderReasons 除了負責判斷規則,也必須保證回傳值永遠不是 null,沒有任何異常時則回傳空集合,這是新生方法與舊流程之間的介面契約,因為 AddRange 收到 null 時會直接拋出例外,反而讓正常訂單無法繼續處理。

而我們為什麼不直接選用另一種作法把「原因加入清單」的動作一起包進去?這樣不就可以一起測了嗎?

AddAbnormalOrderReasons(order, reviewReasons);

當然沒有問題,但我會想把 AddRange 留在外面的原因,是因為 reviewReasons 原本就屬於 ProcessOrder 舊流程中的區域變數,而這次新生方法其實只需要回答我們:

這張訂單有哪些新的異常原因?

因此它只讀取 order,並把答案回傳,至於答案要加進哪一個清單、接下來要不要送審,仍由原本擁有 reviewReasons 的流程決定。

FindAbnormalOrderReasons 是新生方法,AddRange 則是把新生方法的結果接回舊流程的最小接合點。

如果把清單傳進新生方法,那它就會直接修改呼叫端擁有的物件,這就是副作用,副作用不一定是錯,但會讓方法同時負責「判斷原因」與「修改外部狀態」,這對於我們來說不見得是一件好事,再者,其實要不要寫在一起都能夠測,不要的話等等在寫測試的時候確保輸出的資料不會導致 AddRange 拋錯就好,同理寫在一起也需要確保這件事。


開始囉!

先把剛才還在註解裡的方法加進 OrderProcessor,先決定好介面,內容則暫時拋出 NotImplementedException

protected IReadonlyList<OrderReviewReason> FindAbnormalOrderReasons(
    Order order)
{
    throw new NotImplementedException();
}

而為了讓測試能夠呼叫這個 Method,所以我們打算來做個 FakeOrderProcessor,不過可以看到 OrderProcessor 需要透過建構式注入多個既有依賴,但我們現在並不會使用這些依賴,還記得 Day05 為了忽略 IServiceProvider 時用過 Mock.Of() 來做為 Dummy 嗎?現在一樣可以派上用場,並且新增公開 method 替 FindAbnormalOrderReasons 做轉接,程式碼如下:

public sealed class FakeOrderProcessor() :
    OrderProcessor(
        Mock.Of<IInventoryService>(),
        Mock.Of<IShippingService>(),
        Mock.Of<INotificationService>(),
        Mock.Of<IClock>()
    )
{
    public IReadonlyList<OrderReviewReason> FindAbnormalOrderReasonsForTest(Order order)
        => FindAbnormalOrderReasons(order);
}

幹嘛不用空建構子就好

技術上當然可以,但那等於告訴其他人,沒有這些服務也能建立一個有效的 OrderProcessor,實際上這些依賴仍是其他流程正常運作的必要條件,空建構子只會留下尚未初始化的欄位,今天測試剛好只呼叫不使用它們的新生方法,所以當然沒有問題,不過之後只要有人誤用它,就可能會遇到 NullReferenceException,因此這裡選擇保留正式建構子的契約,再用 Moq 提供測試不關心的協作者。


超過件數

準備好測試入口後,就可以先來寫第一個測試:

同商品的數量加總是否超過 100 件:

public class FindAbnormalOrderReasonsTests
{
    private readonly FakeOrderProcessor _sut = new();

    [Fact]
    public void 同商品分成多列且合計超過100件_回傳數量異常()
    {
        var order = new Order
        {
            Items =
            [
                new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 60 },
                new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 50 }
            ]
        };

        var reasons = _sut.FindAbnormalOrderReasonsForTest(order);

        Assert.Equal(
            [OrderReviewReason.ItemQuantityTooHigh],
            reasons);
    }
}

接著在 Enum OrderReviewReason 補上 ItemQuantityTooHigh 通過編譯:

public enum OrderReviewReason
{
    CreditLimitExceeded,
    ItemQuantityTooHigh
}

此刻應該要因為 NotImplementedException 亮紅燈,確認紅燈原因正確後,再補上讓測試通過的實作:

protected IReadOnlyList<OrderReviewReason> FindAbnormalOrderReasons(
    Order order)
{
    var hasExcessiveItemQuantity = order.Items
        .GroupBy(item => item.ProductId)
        .Any(group => group.Sum(item => item.Quantity) > 100);

    if (hasExcessiveItemQuantity)
    {
        return new[] { OrderReviewReason.ItemQuantityTooHigh };
    }

    return Array.Empty<OrderReviewReason>();
}

重新執行第一個測試後,它會綠燈,接下來小重構一下:

protected <OrderReviewReason> FindAbnormalOrderReasons(Order order)
{
    if (HasExcessiveItemQuantity(order))
        return [OrderReviewReason.ItemQuantityTooHigh];

    return [];
}

private bool HasExcessiveItemQuantity(Order order)
{
    var hasExcessiveItemQuantity = order.Items
        .GroupBy(item => item.ProductId)
        .Any(group => group.Sum(item => item.Quantity) > 100);
    return hasExcessiveItemQuantity;
}

因為 GroupBy、Sum、Any 這些操作是屬於很底層的實作細節,把它們留在 FindAbnormalOrderReasons 裡,主方法交代「有哪些規則」和「每條規則做什麼」。


超過金額

接著繼續往下實作「超過金額」的部分:

[Fact]
public void 訂單總額超過10萬元_回傳金額異常()
{
    var order = new Order
    {
        Items = [
            new OrderItem { ProductId = 101, UnitPrice = 100_001m, Quantity = 1 }
        ]
    };

    var reasons = _sut.FindAbnormalOrderReasonsForTest(order);

    Assert.Equal(
        [OrderReviewReason.OrderAmountTooHigh],
        reasons);
}

接著一樣在 OrderReviewReason 補上原因:

public enum OrderReviewReason
{
    CreditLimitExceeded,
    ItemQuantityTooHigh,
    OrderAmountTooHigh
}

這個測試會失敗,因為目前只實作數量規則,來完成吧:

protected IReadonlyList<OrderReviewReason> FindAbnormalOrderReasons(Order order)
{
    if (HasExcessiveItemQuantity(order))
        return [OrderReviewReason.ItemQuantityTooHigh];

    var totalAmount = order.Items.Sum(
        item => item.UnitPrice * item.Quantity);

    if (totalAmount > 100_000m)
        return [OrderReviewReason.OrderAmountTooHigh];

    return [];
}

重新執行後,確保測試都綠燈後,就可以再做一次小重構。

protected IReadonlyList<OrderReviewReason> FindAbnormalOrderReasons(Order order)
{
    if (HasExcessiveItemQuantity(order))
        return [OrderReviewReason.ItemQuantityTooHigh];

    if (HasExcessiveOrderAmount(order))
        return [OrderReviewReason.OrderAmountTooHigh];

    return [];
}

private bool HasExcessiveOrderAmount(Order order)
{
    var totalAmount = order.Items.Sum(
        item => item.UnitPrice * item.Quantity) > 100_000m;
    return totalAmount;
}

很好,這樣在 FindAbnormalOrderReasons 中要做的就很明確了,數量過量就加數量原因、金額過量就加金額原因,整支方法讀起來就是一份「規則清單」。


整理測試

寫到這裡會發現前兩支測試長得幾乎一樣,只有「訂單明細」和「期望原因」不同,這種「同一套流程、只有資料不同」的情況,很自然就會想抽個共用出來,就像昨天 Checkout 那樣把所有測資攤成一個方法的參數列,而且這樣如果寫到一半覺得設計不好想要花心的時候,那測試的改動就會比較小。

不過昨天的測資都很單純,會員等級、地址、是否 VIP,一格一格填進去剛剛好,但這次不一樣,輸入是一組訂單明細,期望原因也可能有好幾個,兩邊天生都是「數量不定」的陣列。

我會希望輸入的商品能直接用 params 來做,這樣測試讀起來也最簡潔,可是 C# 一個方法只能有一個 params、而且必須放在最後,所以如果硬要照昨天那樣把輸入和期望塞進同一個簽名,兩種陣列混在同一列參數裡,呼叫端會變得又長又難讀。

所以這次不抽成單一方法了,而是拆成兩個各自乾淨的小工具,一個負責「拿這些商品去跑判斷」,一個負責「驗證」:

public class FindAbnormalOrderReasonsTests
{
    private readonly FakeOrderProcessor _sut = new();
    private IEnumerable<OrderReviewReason> _reasons = [];

    [Fact]
    public void 同商品分成多列且合計超過100件_回傳數量異常()
    {
        FindReasonsFor(
            new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 60 },
            new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 50 }
        );

        AssertReasons(OrderReviewReason.ItemQuantityTooHigh);
    }


    [Fact]
    public void 訂單總額超過10萬元_回傳金額異常()
    {
        FindReasonsFor(
            new OrderItem { ProductId = 101, UnitPrice = 100_001m, Quantity = 1 }
        );

        AssertReasons(OrderReviewReason.OrderAmountTooHigh);
    }

    private void FindReasonsFor(params OrderItem[] items)
    {
        var order = new Order()
        {
            Items = items.ToList()
        };
        _reasons = _sut.FindAbnormalOrderReasonsForTest(order);
    }


    private void AssertReasons(params OrderReviewReason[] reasons)
        => Assert.Equal(reasons.ToList(), _reasons);
}

FindReasonsFor 讓每支測試把商品像清單一樣直接列出來,AssertReasons 則驗證期望原因。


同時違規

到目前為止 FindAbnormalOrderReasons 只要一命中違規就直接 return,一次只回得了一個原因,但一張訂單可能同時「數量超標」又「金額超標」,主管得一次看到全部原因,所以要加一支「同時違規」的測試:

[Fact]
public void 同時違規()
{
    FindReasonsFor(
        new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 60 },
        new OrderItem { ProductId = 101, UnitPrice = 1m, Quantity = 50 },
        new OrderItem { ProductId = 101, UnitPrice = 100_001m, Quantity = 1 }
    );

    AssertReasons(
        OrderReviewReason.ItemQuantityTooHigh,
        OrderReviewReason.OrderAmountTooHigh
    );
}

因為現在的實作命中數量規則就直接 return,金額規則就不會被檢查到,把「命中就回傳」改成「命中就收集」就好了:

protected IReadonlyList<OrderReviewReason> FindAbnormalOrderReasons(Order order)
{
    var reasons = new List<OrderReviewReason>();

    if (HasExcessiveItemQuantity(order))
        reasons.Add(OrderReviewReason.ItemQuantityTooHigh);

    if (HasExcessiveOrderAmount(order))
        reasons.Add(OrderReviewReason.OrderAmountTooHigh);

    return reasons;
}

測試都綠燈後,才回到 ProcessOrder 拿掉註解:

reviewReasons.AddRange(FindAbnormalOrderReasons(order));

if (RequestReviewWhenNeeded(order, reviewReasons))
{
    return;
}

到這裡,新規則已經完成並接回舊流程。

但是這裡要小心,尤其是在每個接合處的地方,因為我們只證明了新規則本身正確,主流程我們選擇先放過他,所以不管用什麼方法,後續的驗證都一定要做好。


我們得知道一個真相

Sprout Method 不是魔法,把 FindAbnormalOrderReasons 萌生出去之後,ProcessOrder 依然有幾百行保持原樣沒有測試保護,所以今天做的事情,不是一次還清技術債,而是先阻止債務繼續擴大:

我們沒有讓舊程式立刻變好,但至少沒有讓新需求跟著在一起變爛。

這聽起來像是退而求其次,卻往往是緊急狀況中最負責任的選擇,重視客戶不是只顧著快,把所有風險留給下一次事故,而重視工程品質,也不是每次都要求先把整間老房子翻修完成,讓客戶在門外等,真正困難的是在有限時間裡找出平衡,哪些風險這次一定要守住,哪些改善可以延後,以及延後之後要怎麼找得回來。

如果當下時間真的只剩不多,那就先守住新規則、完成必要驗證,把服務恢復起來,等火滅了再把尚未完成的整合測試或重構留下可追蹤的工作,而不是「之後再補」,因為「之後再補」你我都知道,通常都不會再補 XD

在這兩天會學到的技術還是很吃經驗的,在觀察的階段尤其關鍵,要改動的地方是不是對的,是不是最快、當下又最能兼顧品質的,只能多做做看,不過不用擔心啦!反正在以前還不會這些之前,不是都是想都不想直接下手的嗎?所以其實已經比以前的做法還要可靠了啦!

成熟的工程判斷,不是保證每次都能把程式整理到完美,而是清楚知道這次保護了什麼、沒有保護什麼,以及交付之後還欠了什麼,我想這才是我們得接受的現實。


總結

今天學到的做法並不複雜,從難以測試的舊程式旁邊,長出一個邊界清楚也可以獨立測試的新方法,再用最小的接合點把結果接回原流程。

特別適合下面這三種條件同時出現的時候:

  • 時間緊迫,需求必須盡快交付。
  • 舊 method 太大、依賴太多,短時間內無法安全補齊測試。
  • 新需求可以整理成一段輸入、輸出與責任都清楚的邏輯。

承認現在改不動舊的,沒有想像中那麼丟臉,不要假裝問題已經全部解決就好。

Sprout Method 讓我們在現實不允許一步到位時,仍然替下一次改動保留一條比較安全的路。

明天我們繼續看:Legacy Code 緊急自救篇下集

Reference


上一篇
Day 06 -「普羅米修斯之火」-五步驟 SOP
下一篇
Day 08 -「赫菲斯托斯的網」客戶已經火大了,怎麼寫測試? (下)
系列文
諸神也搖頭的 Legacy Code: 30天 .NET 工程師生存之道12
圖片
  熱門推薦
圖片
{{ item.channelVendor }} | {{ item.webinarstarted }} |
{{ formatDate(item.duration) }}
直播中

尚未有邦友留言

立即登入留言