Clean Code in Practice: Cleaning Up a Legacy OrderService Step by Step

Version note: The code in this post was built and run with C# 14 and .NET 10.

Guess First: What Should the First Commit Be?

The legacy OrderService.cs we've looked at piece by piece throughout the series is in front of you: 609 lines, ProcessData alone is 127 lines with a Cognitive Complexity of 92, and there are no tests. The product team wants new coupon types and nobody dares to touch the file. You have one day. What is your first commit?

• Fix the names first, because d, tmp2, k make reading impossible.
• Split ProcessData into small methods first, because the biggest risk is there.
• Write tests first, but there's a weekend discount that depends on DateTime.Now, so the results change with the day.

In this post I cleaned up the file from start to finish: 16 commits; from the third commit, where the tests were added, all tests ran and passed at every commit. The outputs below come from those runs. You've probably guessed the answer is the third option, but the interesting part is the question "what do you do when you can't write the test?"

Strategy: Safety Net First, Then Cleanup

The order follows the plan: first a safety net of characterization tests, then naming, splitting functions, error handling and splitting classes. Every step is a separate commit, and two rules never bent:

A refactoring commit doesn't change behavior. The tests keep asserting the same values and the same messages; only the shape of the API (Application Programming Interface) being called may change.

A behavior change is a separate fix: commit and deliberately changes the expected value in the test. That way, someone looking at git log can see in one line which commit changed the product's behavior. The commit messages follow the Conventional Commits format; we covered the format itself in the Git series post "Teamwork with Git: Branching Strategies, Rulesets and Conventional Commits".

The resulting history:

39cbf8e chore: import legacy OrderService
9873de4 refactor: inject TimeProvider instead of DateTime.Now (seam for tests)
adb9544 test: add characterization tests for OrderService
b1be16c refactor: remove dead and unused code
217d695 fix: compare customer type and coupon codes with invariant culture
12140ca fix: parse imported prices with invariant culture
147bf65 refactor: rename to intention-revealing names and drop redundant comments
7810c9d refactor: replace magic numbers and type strings with enums and constants
9eb89ff test: pin that rejected carts consume an order id
e1060a5 refactor: flatten PlaceOrder with guard clauses and extract methods
3c3d015 fix: quote uses the same pricing as placed orders
120231a fix: let unexpected payment exceptions propagate
9bc2cd0 fix: GetOrders returns an empty list instead of null
372968a refactor!: return Result<T> instead of error codes and LastError
4de6003 refactor: split OrderService into focused classes, one type per file
276eb4b fix: rejected carts no longer consume an order id

Step 0: Seam and Characterization Tests

The term characterization test comes from Michael Feathers' book Working Effectively with Legacy Code (2004): a test that pins down not what the code should do but what it does right now. You don't decide whether it's correct; you put the current behavior in writing, bugs included.

The first obstacle here is time. The weekend discount looks at DateTime.Now; if today is Saturday the test sees one result, if it's Wednesday another. To test untested code you first need a small change, and that change itself is made without tests. Feathers calls this place a seam: a point where you can change behavior from outside without editing the code there. I kept the seam as small and mechanical as possible:

@@ -18,8 +18,15 @@ namespace Bookstore.Legacy
         StringBuilder log = new StringBuilder();
         public bool debug = false;
 
-        public OrderService()
+        private readonly TimeProvider clock;
+
+        public OrderService() : this(TimeProvider.System)
+        {
+        }
+
+        public OrderService(TimeProvider clock)
         {
+            this.clock = clock;
             if (Db.Books.Count == 0) Db.Seed();
         }
 
@@ -97,7 +104,7 @@ namespace Bookstore.Legacy
                             tmp2 += tmp * 0.05m;
                         }
                         if (o.Items.Count > 5) tmp2 += 15;
-                        if (DateTime.Now.DayOfWeek == DayOfWeek.Saturday || DateTime.Now.DayOfWeek == DayOfWeek.Sunday)
+                        if (clock.GetLocalNow().DateTime.DayOfWeek == DayOfWeek.Saturday || clock.GetLocalNow().DateTime.DayOfWeek == DayOfWeek.Sunday)
                             tmp2 += tmp * 0.02m;
                         // shipping
                         decimal k = KargoUcretiHesapla(tmp - tmp2, t, c.Address.City.Name);

The parameterless constructor stays, so nothing changes for callers in production; TimeProvider.System gives the same local time as DateTime.Now. The tests pass a FakeTimeProvider from the Microsoft.Extensions.TimeProvider.Testing package. We introduced TimeProvider itself in the C# Async series post "C# Async and .NET 8: TimeProvider, ConfigureAwaitOptions, Parallel.ForAsync and CancelAsync".

Then the tests. The method Feathers suggests is simple: deliberately put a wrong value in the assert, run the test, copy the real value from the failure message. I did the same job with a small console program on a fixed clock that printed the real values; it ended up as 33 tests. A few of them:

[Fact]
public void Vip_customer_on_a_weekday_gets_five_percent_and_free_shipping()
{
    int id = Service.ProcessData(1, Cart((Dune, 1), (BriefHistory, 1)), null, false, true, 1);

    Order order = Service.GetOrder(id);
    Assert.Equal(1000, id);
    Assert.Equal(33.75m, order.Discount);
    Assert.Equal(0m, order.Shipping);
    Assert.Equal(705.38m, order.Total);
    Assert.Equal(1, order.Status);
    Assert.Equal("TX0001", order.PaymentRef);
}

[Theory]
[InlineData(99, Emma, 1, null, -7, "Customer not found")]
[InlineData(1, "123", 1, null, -2, "Book not found: 123")]
[InlineData(1, Emma, 4, null, -3, "Not enough stock: Emma")]
[InlineData(2, Emma, 1, "FREE", -4, "Invalid coupon")]
[InlineData(3, Emma, 1, null, -5, "Payment failed: Do not honor")]
public void Failures_return_negative_codes_and_set_last_error(
    int customerId, string isbn, int qty, string? coupon, int expectedCode, string expectedError)
{
    int result = Service.ProcessData(customerId, Cart((isbn, qty)), coupon, false, false, 1);

    Assert.Equal(expectedCode, result);
    Assert.Equal(expectedError, OrderService.LastError);
}

[Fact]
public void Under_turkish_culture_the_vip_discount_is_lost()
{
    CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("tr-TR");

    int id = Service.ProcessData(1, Cart((Dune, 1), (BriefHistory, 1)), null, false, false, 1);

    Assert.Equal(0m, Service.GetOrder(id).Discount);
    Assert.Equal(742.50m, Service.GetOrder(id).Total);
}

Look at the last test. Losing the VIP (Very Important Person) discount under the Turkish culture is a bug, but the characterization test pins it too, because the goal is to know the current behavior. The test's name says so openly.

Because of static state, the test base had to reset the whole world before every test and switch off parallel execution:

[assembly: CollectionBehavior(DisableTestParallelization = true)]

protected LegacyTestBase()
{
    CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("en-US");
    Db.Reset();
    Mailer.Sent.Clear();
    OrderService.LastError = null;
    OrderService.cnt = 0;
    Clock = new FakeTimeProvider(Wednesday);
    Service = new OrderService(Clock);
}

For measuring coverage, the coverlet collector that comes with the xUnit template is enough:

dotnet test --collect:"XPlat Code Coverage"
Passed!  - Failed:     0, Passed:    33, Skipped:     0, Total:    33, Duration: 30 ms - Bookstore.Tests.dll (net10.0)
<coverage line-rate="0.7961" branch-rate="0.7037"

The 33 tests execute 79.6 percent of the lines and 70.4 percent of the branches. The methods that never run are IsValid, CheckStock, TryCalc, CalcPoints, UpdateStatus, OldCalcDiscount, SendMail, GetFromCache and ClearAll. This list became the input for the next step: code that has no callers in the solution and isn't protected by tests isn't refactored, it's deleted. The first cleanup commit deleted 112 lines from OrderService.cs; in the tests, only the line resetting the now nonexistent cnt counter went away.

A small side note: I first wrote the invoice's date line as "Date: 11/4/2026 10:00:00 AM" and the test went red, even though the two strings looked identical on screen. The ICU (International Components for Unicode) data .NET uses on macOS puts a narrow no-break space (U+202F) instead of a regular space before the AM (ante meridiem) marker in the en-US time format. I produced the expected value by formatting the same DateTime.

Behavior Changes in Their Own Commits

Once the safety net was in place I fixed two known bugs, without mixing them into refactoring. The Turkish-I fix is a one-line difference:

@@ -95,7 +95,7 @@ namespace Bookstore.Legacy
                                 return -4;
                             }
                         }
-                        if (c.Type.ToUpper() == "VIP")
+                        if (c.Type.ToUpperInvariant() == "VIP")
                         {
                             tmp2 += tmp * 0.05m;
                         }

Running the tests, the net caught it as expected:

  Failed Bookstore.Tests.Characterization.ProcessDataTests.Under_turkish_culture_the_vip_discount_is_lost [< 1 ms]
Expected: 0
Actual:   33,750
Failed!  - Failed:     1, Passed:    32, Skipped:     0, Total:    33, Duration: 25 ms - Bookstore.Tests.dll (net10.0)

The expected value went from 0 to 33.75 and the test was renamed to Turkish_culture_keeps_the_vip_discount. In the same way, culture-dependent parsing of prices in the import (on a Turkish machine the price 520.50 was read as 52050) was fixed in a separate commit. The reason I did these two commits first: shortly after, I would turn string Type into an enum, and that refactoring would have silently removed the Turkish-I bug. The only record that the bug was fixed would have been lost inside a commit called "refactor".

Naming and Functions

The rename commit is purely mechanical: ProcessData → PlaceOrder, KargoUcretiHesapla → CalculateShipping, d → cart, tmp2 → discount, Qty → Quantity. Changelog comments and comments that repeat the code went in the same commit. In the tests only the names of the called methods changed; I checked with git diff, not a single expected value line changed. Then the magic numbers became the OrderStatus, ShippingMethod, BookFormat and CustomerTier enums and the PricingRules constants.

Before splitting functions I noticed a blind spot: ProcessData took the order number before checking the books, so a rejected cart also used up a number. No test saw this, and while restructuring the function I could have changed this behavior without noticing. I first added a test commit that pins it, then flattened PlaceOrder with guard clauses:

public int PlaceOrder(int customerId, Dictionary<string, int> cart, string couponCode, bool giftWrap, bool sendConfirmation, ShippingMethod shippingMethod)
{
    LastError = null;
    Customer customer = Db.Customers.FirstOrDefault(c => c.Id == customerId);
    if (customer == null) return Fail(-7, "Customer not found");
    if (cart == null || cart.Count == 0) return Fail(-1, "Empty cart");
    if (customer.IsBlocked) return Fail(-6, "Customer blocked");

    Order order = StartOrder(customer);
    foreach (var (isbn, quantity) in cart)
    {
        Book book = Db.Books.FirstOrDefault(b => b.Isbn == isbn);
        if (book == null) return Fail(-2, "Book not found: " + isbn);
        if (book.Stock < quantity) return Fail(-3, "Not enough stock: " + book.Title);
        order.Items.Add(new OrderItem { Book = book, Quantity = quantity, Price = UnitPrice(book) });
    }
    if (!IsValidCoupon(couponCode)) return Fail(-4, "Invalid coupon");

    decimal subtotal = order.Items.Sum(item => item.Price * item.Quantity);
    order.Discount = CalculateDiscount(subtotal, customer, couponCode, order.Items.Count);
    order.Shipping = CalculateShipping(subtotal - order.Discount, shippingMethod, customer.Address.City.Name)
        + (giftWrap ? PricingRules.GiftWrapFee : 0);
    order.Total = Math.Round((subtotal - order.Discount) * (1 + PricingRules.TaxRate) + order.Shipping, 2);

    int paymentError = ChargeCustomer(order);
    if (paymentError != 0) return paymentError;

    CompleteOrder(order);
    if (sendConfirmation) SendConfirmation(order);
    return order.Id;
}

The four-level if pyramid is gone, every guard is one line. Cognitive Complexity dropped from 88 to 10. There are still return codes and a static LastError; error handling hasn't had its turn yet. One thing is deliberately the same: StartOrder still takes the number before validation, because the test says it does.

Once pricing was gathered into a single Price method, GetQuote started using it too, and the price on the cart page became equal to the price at checkout. That's a behavior change as well, so it's a separate fix: commit: the test expects 705.38 instead of 731.50.

Error Handling and Splitting Classes

Two more small fix: commits came before the error handling commit: an unexpected exception from the payment SDK (Software Development Kit) is no longer swallowed and turned into -99, and GetOrders returns an empty list instead of null. Then the return codes and LastError were replaced with the Result<T> and typed Error records from the error handling post. This commit is marked refactor!:, because the public API breaks and the two flag arguments of CancelOrder go away: cancelling now always means refund and notify. In the tests, the negative numbers became error types while the messages stayed exactly the same: "Book not found: 123" is still asserted.

The last big step is splitting the class. OrderService went from a single 462-line class to a class that takes five dependencies through its constructor and whose file is 115 lines:

public sealed class OrderService(
    BookstoreStore store,
    OrderPricing pricing,
    IPaymentGateway payments,
    IOrderNotifier notifier,
    TimeProvider clock)
{
    public Result<Order> PlaceOrder(PlaceOrderRequest request)
    {
        Customer? customer = store.FindCustomer(request.CustomerId);
        if (customer is null) return new CustomerNotFound(request.CustomerId);
        if (request.Cart.Count == 0) return new EmptyCart();
        if (customer.IsBlocked) return new CustomerBlocked(customer.Id);

        Result<IReadOnlyList<OrderItem>> items = PriceItems(request.Cart);
        if (!items.IsSuccess) return items.Error!;
        if (!OrderPricing.IsKnownCoupon(request.CouponCode)) return new InvalidCoupon(request.CouponCode!);

        Order order = new()
        {
            Id = store.NextOrderId(),
            Customer = customer,
            Date = clock.GetLocalNow().DateTime,
            Items = items.Value,
            Price = pricing.Price(items.Value, customer, request.CouponCode, request.ShippingMethod, request.GiftWrap),
        };
        return Charge(order, request.SendConfirmation);
    }

    // ... GetQuote, CancelOrder, ShipOrder and PriceItems live here

    private Result<Order> Charge(Order order, bool sendConfirmation)
    {
        PaymentOutcome outcome = payments.Charge(order.Customer.CardToken, order.Total);
        if (outcome is PaymentOutcome.Declined declined)
        {
            order.Status = OrderStatus.PaymentFailed;
            store.Add(order);
            return new PaymentDeclined(declined.Reason);
        }

        order.PaymentRef = ((PaymentOutcome.Approved)outcome).TransactionId;
        foreach (OrderItem item in order.Items) item.Book.Stock -= item.Quantity;
        order.Status = OrderStatus.Paid;
        store.Add(order);
        order.Customer.EarnPointsFor(order);
        if (sendConfirmation) notifier.OrderPlaced(order);
        return order;
    }
}

Pricing moved to OrderPricing, the payment SDK behind IPaymentGateway as in the boundaries post (the interface was async there; since the legacy code is synchronous I kept it synchronous in this step), e-mail behind IOrderNotifier, the invoice to InvoiceFormatter, the report to SalesReport, customer registration to CustomerRegistry, the CSV (Comma-Separated Values) import to BookCsvImporter. The static Db and Mailer became instances, and the test base now looks like a composition root:

protected LegacyTestBase()
{
    CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("en-US");
    Clock = new FakeTimeProvider(Wednesday);
    Store = BookstoreStore.CreateSeeded();
    Mailer = new Mailer();
    Service = new OrderService(
        Store,
        new OrderPricing(Clock),
        new VendorPaymentGateway(new PaymentClient("merchant-001", 30)),
        new EmailOrderNotifier(Mailer),
        Clock);
    Customers = new CustomerRegistry(Store);
    Importer = new BookCsvImporter(Store);
    Invoices = new InvoiceFormatter(Clock);
    Reports = new SalesReport(Store);
}

The DisableTestParallelization line was deleted; the tests now run in parallel because they no longer touch each other's static state. That was the most concrete cost of the Global Data smell. #nullable disable is gone too; it stays only in Vendor/PaymentClient.cs, because that file simulates third-party code and we don't change it. The last commit fixed the order number behavior I pinned at the start: the number is now taken after the cart is validated.

Before and After: Metrics

I checked out every commit in a separate worktree and re-ran the tests and the same metrics build (the SonarAnalyzer rules S3776 (Cognitive Complexity) and S138 (method length) with lowered thresholds, plus CA1502):

All metrics were taken with the same measurement, and the vendor SDK file is included on both sides. Total Cognitive Complexity went from 182 to 59, the highest cyclomatic complexity from 27 to 7. Line coverage rose from 79.6 percent after the characterization tests to 94.6 percent; most of that comes not from new tests but from deleting code that never ran. One number went up, though: the total line count grew from 609 to 678, and the single file became 28 files. Remember the fourth rule from the simple design post: the goal isn't to reduce the number of elements but to reduce what the reader has to keep in mind at once. Someone changing a pricing rule now reads the 54-line OrderPricing, not 609 lines.

The most important column of the table is the middle one: no row has a red test. The biggest metric jump (from 88 to 13 at e1060a5) came without a single expected value in the tests changing.

Smells Left on Purpose

The cleanup didn't finish, it was stopped. These are the ones left on purpose, and why:

• ShipOrder(orderId, bool express) still takes a flag argument. The right fix is for the order to store its shipping method, but that's a data model change that needs a migration; separate work.
• OrderService.ShipOrder writes the label straight to disk with File.WriteAllText. The label text moved to ShippingLabel; until a printer integration arrives, a file is enough.
• Cancelling a VIP order doesn't take back the 50 bonus points. This may be a bug or a business decision; a test pins it, and the comment in Customer.ReturnPointsFor is waiting for the business team's answer.
• The rates in PricingRules and the "Istanbul" and "Turkey" strings are fixed in the code. Moving them to configuration before anyone asks to manage campaigns from a panel would be the speculative generality from the simple design post.
• Order.Status is changed directly by the service. Moving the transitions into a state machine is the topic of the Solution Patterns series post "When the Status Column Is Updated from Everywhere".
• Money is still a bare decimal. The Money value object is in the Solution Patterns series post "Money and Rounding"; it was postponed here because the rounding rules didn't change.

Knowing where to stop the cleanup is a decision too. Every remaining smell is noted with its likelihood of change and its cost; the code can be picked up again in the same order at the next change.

Common Pitfalls

Starting to refactor without tests. While flattening ProcessData it was very easy to change the order number behavior, and no compiler warning would have said so.

Silently fixing a bug in a refactoring commit. The enum conversion would have removed the Turkish-I bug on its own. If a behavior change doesn't come in a separate commit with a changed test, a year later nobody knows what changed and why.

Mistaking characterization tests for "correct behavior" tests. These tests record today's behavior. Name a test that pins a bug so that it says so, then change it deliberately.

Doing it all at once. 16 commits may look excessive, but when you hunt for a regression with git bisect, every commit doing one thing makes all the difference; we covered the tool itself in the Git series post "Git bisect and git worktree: Finding the Bad Commit and Two Branches at Once".

Making metrics the goal. To lower Cognitive Complexity, you could split BookCsvImporter.Import (Cognitive 10, now the highest) into five methods. But it's a readable, flat if/else if chain; splitting it would be an easy way to produce the entangled methods from the Ousterhout debate.

A Small Experiment

★☆☆ Put LEGACY_ORDERSERVICE.cs into a project, add the seam and write the three characterization tests from this post yourself. Use Feathers' method: first assert a wrong value, then take the real value from the failure message.

★★☆ Remove the flag from ShipOrder, one of the smells left on purpose: add a ShippingMethod to Order and read from there whether the label is express. Which characterization test's expectation has to change, and should this commit be refactor: or fix:?

★★★ Assume you want to change the VIP bonus point behavior in CancelOrder. First find the test that pins the current behavior, then prepare a fix: commit that encodes the new behavior and reference the business decision in the commit message.

This post ends the Clean Code series. Over fourteen posts we cleaned up the same file, from naming to error handling, from classes to test code, from the smell catalog to the critique of the rules. Asking which cost each rule lowers, instead of memorizing the rules, is the series' real recommendation. In the upcoming SOLID (Single Responsibility, Open/Closed, Liskov Substitution, Interface Segregation and Dependency Inversion principles) and Design Principles series, we'll take the coupling and cohesion questions that stayed at class level here up to the level of principles.