Refactoring Catalog 3: Introduce Parameter Object, Preserve Whole Object, Combine Functions into Class/Transform, Split Phase, Extract/Inline Class and Hide Delegate

Version note: The code in this post was built and run with C# 14 and .NET 10 (SDK 10.0.404); the tests use xUnit.net v3 4.0.3 and Verify.XunitV3 32.0.0.

Guess First: Do the Two Parsers Give the Same Result?

This third post of the catalog block covers the refactorings that group responsibilities: Introduce Parameter Object, Preserve Whole Object, Hide Delegate, Remove Middle Man, Combine Functions into Class, Combine Functions into Transform, Split Phase, Extract Class, Inline Class and Substitute Algorithm. The names are as on refactoring.com/catalog; the explanations and examples are ours. We apply all of them in the Ordering module of Bookstore, the online bookstore used as the shared example across this site's series: the CSV import that institutional customers use to upload bulk orders, the invoice calculation and the sales report.

The import splits the line, separates the fields, validates each field and writes error messages, all in a single loop. The price field arrives in Turkish format ("1.149,90"), and a small hand-written class converts it:

public static class TurkishDecimal
{
    // "1.234,50" -> 1234.50
    public static bool TryParse(string text, out decimal value)
    {
        string normalized = text.Replace(".", "").Replace(",", ".");
        return decimal.TryParse(normalized, NumberStyles.Number, CultureInfo.InvariantCulture, out value);
    }
}

Someone on the team wants to replace it with .NET's own culture support: decimal.TryParse(text, NumberStyles.Number, CultureInfo.GetCultureInfo("tr-TR"), out value). By its catalog name this is a Substitute Algorithm: putting a clearer algorithm that gives the same result in place of the old one. Think of 19 different inputs with a comma as decimal separator, a dot as thousands separator, a leading plus or minus sign, surrounding spaces. On how many do the two algorithms agree?

Introduce Parameter Object and Preserve Whole Object

I started with characterization tests again: the import result in a Verify snapshot, the invoice, the report and the shipping fee with a few Assert calls. The snapshot has two odd lines, approved as today's behavior: the price "59.90" is read as 5990, and "1,234.50" as 1.23450. We'll come back to them at the end of the post.

Both methods of the sales report take a date range: Revenue(orders, DateOnly from, DateOnly to) and OrderCount(orders, from, to). These two parameters that always travel together are a typical case of Data Clumps from the Clean Code series post "A Code Smell Catalog". Introduce Parameter Object gathers them into a type: create the new type, add a parameter of that type to the methods, either with the parallel change from the third post or in one step if all callers are inside the solution, move the callers, delete the old parameters. The real win comes in the second step: taking the range check away from the callers and moving it into the type.

public sealed record DateRange(DateOnly From, DateOnly To)
{
    public bool Contains(DateOnly date) => date >= From && date <= To;
}

public static decimal Revenue(IEnumerable<Order> orders, DateRange period) =>
    orders.Where(o => period.Contains(o.PlacedOn)).Sum(o => o.Total);

In this step I also wanted to add validation that rejects a From later than To, but in the old code a reversed range silently returned zero; throwing an exception would have been a behavior change. So the validation didn't go into this commit; it's work to be decided as a separate fix: commit. The PriceRequest we saw in the third post was an example of the same refactoring.

Preserve Whole Object is a close relative. The shipping fee was called as ShippingQuote.FeeFor(order.Total, order.ShippingAddress.City): the caller pulls two values out of the order and sends them. Giving the method the order itself saves us from changing the signature again tomorrow, when the shipping rule starts looking at another field of the order, such as weight or whether it's an e-book:

-    public static decimal FeeFor(decimal orderTotal, string city)
+    public static decimal FeeFor(Order order)
-        if (orderTotal >= 300m)
+        if (order.Total >= 300m)
-        return MetroCities.Contains(city) ? 49.90m : 64.90m;
+        return MetroCities.Contains(order.ShippingAddress.City) ? 49.90m : 64.90m;

It has a cost too: ShippingQuote now depends on Order. If the shipping calculation lived in a module independent of Ordering, or were also called with values from elsewhere, taking the two values separately would be the right choice. The inverse of Preserve Whole Object is not a separate refactoring named in the catalog; opening the parameter back into values is a Change Function Declaration.

Hide Delegate and Remove Middle Man

These two refactorings are inverses of each other, and I applied both in the same class. When grouping by customer, the reports wrote o.Customer.Id: a chain that knows the Customer inside the Order, the Message Chains smell. Hide Delegate hides the chain with a member on its owner: public int CustomerId => Customer.Id;. The report now knows only Order. It's also a step towards the idea in the DDD series post "Aggregate Design: Consistency Boundaries, Small Aggregates, and Referencing by Id": if one day Order holds only a CustomerId, the callers are already prepared.

The same class had the opposite problem too. Over time Order had collected properties that only forwarded to Customer, such as CustomerName, CustomerLoyaltyPoints and CustomerAddress. The only place using them was the loyalty report, and that report cared about the customer, not the order. Order is a Middle Man here: it does no work, only forwards. Remove Middle Man removes the forwarding members and lets the caller talk to the real object:

public static class LoyaltyReport
{
    public static string Line(Customer customer) =>
        $"{customer.Name} ({customer.Id}): {customer.LoyaltyPoints} points, {customer.Address.City}";
}

The question that decides which one is right: should the caller know what is behind the intermediate object? A report counting orders per customer works with orders and doesn't need to know the customer's internals. The loyalty report is about the customer anyway. Applying the Law of Demeter mechanically to every chain turns every class into a middle man full of dozens of forwarding members; the way the SOLID and Design Principles series post "Coupling and Cohesion" presents it treats it as a measure of coupling, not a rule.

Combine Functions into Class or Transform?

The invoice calculation was four static functions, all taking the same three arguments:

public static class InvoiceFunctions
{
    public static decimal BaseAmount(Order order) => order.Total;

    public static decimal DiscountAmount(Order order, decimal discountRate) =>
        Math.Round(order.Total * discountRate, 2, MidpointRounding.AwayFromZero);

    public static decimal VatAmount(Order order, decimal discountRate, decimal vatRate) =>
        Math.Round((order.Total - DiscountAmount(order, discountRate)) * vatRate, 2, MidpointRounding.AwayFromZero);

    public static decimal InvoiceTotal(Order order, decimal discountRate, decimal vatRate) =>
        BaseAmount(order) - DiscountAmount(order, discountRate) + VatAmount(order, discountRate, vatRate);
}

For a group of functions working on the same data, the catalog has two options. Combine Functions into Class makes the shared data the state of a class and turns the functions into its members. With a primary constructor it's very short:

public sealed class InvoiceCalculator(Order order, decimal discountRate, decimal vatRate)
{
    public decimal BaseAmount => order.Total;

    public decimal Discount => Round(BaseAmount * discountRate);

    public decimal Vat => Round((BaseAmount - Discount) * vatRate);

    public decimal Total => BaseAmount - Discount + Vat;

    private static decimal Round(decimal amount) => Math.Round(amount, 2, MidpointRounding.AwayFromZero);
}

Combine Functions into Transform writes a single function that takes the source data and returns new data that also contains the derived values; its natural counterpart in C# is an immutable record. To compare the two I wrote a small file-based app, samples/transform.cs in the repo: it first computes the invoice both ways, then adds a book to the order, and finally copies the transform's result with with and zeroes the discount.

InvoiceData data = InvoiceTransform.Enrich(order, discountRate: 0.10m, vatRate: 0.10m);
InvoiceCalculator calculator = new(order, discountRate: 0.10m, vatRate: 0.10m);

order.AddItem(new OrderItem(Isbn.Parse("9780441013593"), "Dune", 12.45m, 1));

Console.WriteLine($"transform : {data.Total}");
Console.WriteLine($"calculator: {calculator.Total}");

InvoiceData noDiscount = data with { Discount = 0m };
Console.WriteLine($"with      : discount {noDiscount.Discount}, total {noDiscount.Total}");

public sealed record InvoiceData(decimal BaseAmount, decimal Discount, decimal Vat, decimal Total);

$ dotnet run transform.cs
transform : 296.80
calculator: 309.12
with      : discount 0, total 296.80

There are two differences. The transform's result is a snapshot: it goes stale when the order changes. The class computes on every read. And with doesn't recompute derived fields; we got an invoice whose discount was zeroed but whose total stayed the same, inconsistent with itself. The rule is simple: if the source data can change later, use a class. If the data flows once through a pipeline and doesn't change, such as a report row or an export record, a transform is simpler; even then, produce derived fields by calling the transform again, not with with. In Bookstore an order can change until it's invoiced, so I chose InvoiceCalculator.

Split Phase and Extract Class

Back to the import. The single loop interleaved two different jobs: turning text into structure, meaning lines, fields and row numbers, and applying business rules to that structure: customer number, ISBN, quantity limit, price. Split Phase divides such code into two consecutive phases with only an intermediate data structure passing between them. It's the typical answer to the Divergent Change smell from the Clean Code series: when the CSV format changes, say a new column or quoted fields, only the first phase changes; when a business rule changes, only the second.

The mechanics: extract the code of the second phase into its own method; find the data the two phases share and put it into an intermediate data structure, here CsvRow(int Number, string[] Fields); extract the first phase into its own method too and make it return that structure; let the second phase take only that structure. In the first commit the two phases were two private methods of the same class, and the snapshot didn't change. The second commit is an Extract Class: each phase moved into its own class, and the import turned into a thin composer.

public class OrderCsvImporter
{
    public ImportResult Import(string csv) => OrderRowValidator.Validate(OrderCsvParser.Parse(csv));
}

public static class OrderCsvParser
{
    public static IReadOnlyList<CsvRow> Parse(string csv)
    {
        List<CsvRow> rows = [];
        int row = 0;
        foreach (string raw in csv.Split('\n'))
        {
            row++;
            if (row == 1 || raw.Trim() == "")
                continue;
            rows.Add(new CsvRow(row, raw.Trim().Split(';')));
        }
        return rows;
    }
}

Extract Class applies when a class changes for two different reasons or some of its fields are always used together, the Large Class and Data Clumps smells: open the new class, move the related fields and methods one by one with Move Field and Move Function, the topic of the next post, and test after each move. The win came right away: the parser can now be tested independently of business rules. The test that checks it skips the header and blank lines and keeps the row number is four lines.

Substitute Algorithm: Check Equivalence First

The mechanics of Substitute Algorithm are short: put the old algorithm behind a single function, write the new one, run both with the same inputs and compare the results, delete the old one if they match. The hard part is making "the same inputs" thorough enough. So I wrote a separate test: it feeds 19 inputs to both algorithms and compares both the success flag and the value, including scale, as a string.

[Theory]
[InlineData("12,45")]
[InlineData("1.149,90")]
[InlineData("59.90")]
// ... 19 inputs in total
[InlineData("1,234.50")]
[InlineData("0,1.5")]
public void New_algorithm_gives_the_same_result(string text)
{
    bool oldOk = TurkishDecimal.TryParse(text, out decimal oldValue);
    bool newOk = decimal.TryParse(text, NumberStyles.Number, Turkish, out decimal newValue);

    Assert.Equal((oldOk, oldValue.ToString(CultureInfo.InvariantCulture)), (newOk, newValue.ToString(CultureInfo.InvariantCulture)));
}

The Answer: 17 Inputs Agree, Two Differ, and the Old One Was Wrong

failed Bookstore.Ordering.Tests.PriceParsingEquivalenceTests.New_algorithm_gives_the_same_result(text: "0,1.5") (1ms)
  Assert.Equal() Failure: Values differ
  Expected: Tuple (True, "0.15")
  Actual:   Tuple (False, "0")
failed Bookstore.Ordering.Tests.PriceParsingEquivalenceTests.New_algorithm_gives_the_same_result(text: "1,234.50") (0ms)
  Assert.Equal() Failure: Values differ
  Expected: Tuple (True, "1.23450")
  Actual:   Tuple (False, "0")

On seventeen inputs the two algorithms agree exactly, including "12,", ",5", "+7", inputs with surrounding spaces and with thousands separators. Both read "59.90" as 5990, because in Turkish format the dot is the thousands separator. The two inputs that differ are interesting: the old code silently turns "1,234.50", written in English format, into 1.2345 TL, while the new code rejects it. In a file uploaded by a school whose Excel runs in English, a line worth a thousand TL would drop to about one lira twenty-three kuruş.

So this Substitute Algorithm can't be a pure refactoring: behavior changes on two inputs, and the new behavior is more correct than the old. As in the eleventh post, I made it a fix: commit: the new algorithm, the removal of the equivalence test and the snapshot update in the same commit. The snapshot diff shows the change exactly: the 1.23450 TL line disappeared and an error line took its place. The end of the diff:

<     Row 9: price 'abc' is not a number.
---
>     Row 9: price 'abc' is not a number.,
>     Row 10: price '1,234.50' is not a number.

A warning: CultureInfo.GetCultureInfo("tr-TR") relies on the operating system's culture data. If the application runs with InvariantGlobalization enabled, which is common in small container images, this line throws. I tried it with a separate small program, samples/invariant-culture.cs: CultureNotFoundException: Only the invariant culture is supported in globalization-invariant mode. In such an environment, building your own NumberFormatInfo with a comma decimal separator and a dot group separator is more robust.

After the Substitute Algorithm, the TurkishDecimal class shrank to a one-line method, and its only user was the validator. Inline Class is the inverse of Extract Class: it moves the members of a class that no longer does enough work into the class that uses it and deletes the class. The parsing moved into OrderRowValidator, and the culture into a private field of it.

IDE Support and All the Steps

The IDE column is based on the Microsoft Learn and JetBrains docs; I didn't run the IDEs for this post. Automation is weak in this family, because most steps involve a design decision.

$ git log --oneline
c27679e refactor: inline class TurkishDecimal into OrderRowValidator
3c57edf fix: parse prices with tr-TR culture; reject English-formatted prices
dc113bb refactor: extract OrderCsvParser and OrderRowValidator classes
1dc28fb refactor: split CSV import into parse and validate phases
2e8bbdf refactor: combine invoice functions into InvoiceCalculator
f280b1f refactor: remove middle man (customer profile forwarders on Order)
817055b refactor: hide delegate with Order.CustomerId
05da370 refactor: preserve whole object in ShippingQuote.FeeFor
f580083 refactor: move the range check into DateRange.Contains
3cc2758 refactor: introduce DateRange parameter object
342e26a test: characterize CSV import, invoice functions, reports and shipping quote

Common Pitfalls

Applying Hide Delegate to every chain. Adding a forwarding member to a for every a.B.C eventually produces classes that do no work and only forward. First ask whether the caller needs to know the object behind; the Law of Demeter is a warning sign, not a rule list.

Changing a transform's result with with. Derived fields are not updated along with the source; in the output above we saw an invoice with a zeroed discount and an unchanged total.

Doing Substitute Algorithm by trusting the name. "Turkish number parsing" is the name of both algorithms, yet they behave differently on two inputs. Write the equivalence test with edge values: empty, signed, separator in the wrong place, another language's format.

Skipping the intermediate structure in Split Phase. If the code is split into two methods but the first calls the second, or both change a shared field, it's still one phase. The only link between the phases should be the intermediate data structure.

Coupling layers with Preserve Whole Object. Giving an upper layer's entity to a calculation function in a lower layer makes that function impossible to reuse.

A Small Experiment

★☆☆ Find two or three parameters that always travel together in your code: a date range, a page number and size, an amount and its currency. Apply Introduce Parameter Object, then move a check on those parameters into the new type.

★★☆ Split an import or export code path in two with Split Phase. How hard was it to find the intermediate data structure? Can you test the first phase independently of business rules?

★★★ Pick a place where you're thinking of replacing hand-written parsing, formatting or calculation code with .NET's own API. Before replacing it, write an equivalence test with 20 edge values. How many inputs differed, and for each difference, was the old or the new one right?

Responsibilities now gather in the right classes, but some methods and fields still work with another class's data, and a single loop in the stock report does three different jobs. In the next post we move on to the refactorings that move something from one place to another: Move Function, Move Field, Move Statements into Function and Move Statements to Callers, Slide Statements, Split Loop, Replace Loop with Pipeline and Remove Dead Code.