Refactoring Catalog 2: Encapsulate Variable/Record/Collection, Replace Primitive with Object, Replace Temp with Query and Split Variable

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.

Guess First: What Does This Order Print?

In the Ordering module of Bookstore, the online bookstore used as the shared example across this site's series, the order model was written years ago and has grown ever since by adding "one more field":

public static class OrderSettings
{
    public static int MaxQuantityPerLine = 10;
}

public class OrderItem
{
    public string Isbn = "";
    public string Title = "";
    public decimal UnitPrice;
    public int Quantity;
}

public class Address
{
    public string Street = "";
    public string City = "";
    public string PostalCode = "";
}

public class Customer
{
    public int Id;
    public string Name = "";
    public Address Address = new();
    public int LoyaltyPoints;
}

public class Order
{
    public List<OrderItem> Items { get; set; } = new();
    public decimal Total;
    public Customer Customer = new();
    public Address ShippingAddress = new();

    public void AddItem(OrderItem item)
    {
        if (item.Quantity > OrderSettings.MaxQuantityPerLine)
            throw new InvalidOperationException($"At most {OrderSettings.MaxQuantityPerLine} copies per line.");
        Items.Add(item);
        decimal lineTotal = item.UnitPrice * item.Quantity;
        Total += lineTotal;
    }

    public bool Contains(string isbn) => Items.Any(i => i.Isbn == isbn);

    public decimal DiscountedTotal(decimal rate)
    {
        decimal temp = Total * rate;
        temp = Total - temp;
        return Math.Round(temp, 2, MidpointRounding.AwayFromZero);
    }

    public string Summary()
    {
        List<string> lines = [];
        foreach (OrderItem item in Items)
        {
            decimal lineTotal = item.UnitPrice * item.Quantity;
            lines.Add($"{item.Quantity} x {item.Title} = {lineTotal}");
        }
        return string.Join(Environment.NewLine, lines);
    }
}

The "order again" feature copies the lines of an old order with Items.Add directly, not with AddItem. The book page sends the ISBN with hyphens, the search page without. When a customer moves, the profile page updates Address.City. The program below runs these three cases one after another:

Order order = new() { Customer = ada, ShippingAddress = ada.Address };

order.AddItem(new OrderItem { Isbn = "978-0-441-01359-3", Title = "Dune", UnitPrice = 12.45m, Quantity = 1 });
order.Items.Add(new OrderItem { Isbn = "9780261103344", Title = "The Hobbit", UnitPrice = 149.90m, Quantity = 2 });
ada.Address.City = "Izmir";

Console.WriteLine($"Total: {order.Total}");
Console.WriteLine($"Contains 9780441013593: {order.Contains("9780441013593")}");
Console.WriteLine($"Ship to: {order.ShippingAddress.City}");

What is the order total? Does the order contain Dune? Where does an order shipped to Ankara appear to have been shipped after the customer moves to Izmir?

The Answer: Three Surprises, One Root Cause

Total: 12.45
Contains 9780441013593: False
Ship to: Izmir

The total counts only Dune, because Total is a field and a line added to the list outside AddItem doesn't update it. Two spellings of the same ISBN are two different strings. The order's shipping address is the same object as the customer's address, so when the customer moves, the past order "moves" too. All three have the same root: the data is too open to protect its own rules. This post's refactorings are about exactly that problem: Encapsulate Variable, Encapsulate Record, Encapsulate Collection, Replace Temp with Query, Replace Derived Variable with Query, Split Variable, Replace Primitive with Object, Change Reference to Value and Change Value to Reference. The names are as on refactoring.com/catalog; the explanations and examples are ours.

I started with tests again and recorded all three surprises as today's behavior: Total_ignores_items_added_directly_to_the_list, Contains_compares_isbn_strings_as_written, Shipping_address_follows_the_customer_after_the_order. Why record wrong behavior? Because some steps will change it shortly, and we want that to be deliberate. The two-hats rule from the first post becomes very concrete here: a step that keeps behavior is a refactor: commit, a step that changes it is a fix: commit, and the two never share a commit.

Encapsulate Variable: Global Data and Public Fields

Encapsulate Variable puts data that anyone can read and write, the Global Data and Mutable Data smells, behind a single access point. The goal is not to get rid of the variable but to see and control who touches it and how. .NET's own analyzers recognize this smell; after turning four rules into warnings in .editorconfig, the legacy code said this (lines shortened):

$ dotnet build --no-incremental
src/Bookstore.Ordering/Order.cs(5,23): warning CA2211: Non-constant fields should not be visible
src/Bookstore.Ordering/Order.cs(10,19): warning CA1051: Do not declare visible instance fields
src/Bookstore.Ordering/Order.cs(33,28): warning CA1002: Change 'List<OrderItem>' in 'Order.Items' to use 'Collection<T>', 'ReadOnlyCollection<T>' or 'KeyedCollection<K,V>'
src/Bookstore.Ordering/Order.cs(33,28): warning CA2227: Change 'Items' to be read-only by removing the property setter

CA1051 alone showed up fourteen times. The first target is CA2211: MaxQuantityPerLine is a public static field and everyone, test code included, can change it. The mechanics: write a property that wraps the field, turn every read and write into the property, make the field private, test. Once the property is in place, the spot for validation is obvious too:

public static class OrderSettings
{
    private static int s_maxQuantityPerLine = 10;

    public static int MaxQuantityPerLine
    {
        get => s_maxQuantityPerLine;
        set => s_maxQuantityPerLine = value >= 1
            ? value
            : throw new ArgumentOutOfRangeException(nameof(value), value, "The limit must be at least 1.");
    }
}

I applied the same step to the public fields in Customer and Order: Id and Name became init, Address and LoyaltyPoints became set. A warning: turning a field into a property breaks almost no caller in source code, except fields passed with ref and out, but it does break compiled code. Fields and properties are accessed differently in IL; an assembly that uses the same package and hasn't been recompiled gets a MissingFieldException. If you publish a package, this is a breaking change.

Moving static global data behind a property is an intermediate step; the real target is often to pass it through the constructor with IOptions<T>. That is the parameterize-constructor step we saw in the fifth post.

Encapsulate Record: From Data Class to record

OrderItem is a Data Class made only of public fields. Encapsulate Record puts the fields of such a data structure behind members with access control. In C# the shortest way is a record:

public sealed record OrderItem(Isbn Isbn, string Title, decimal UnitPrice, int Quantity)
{
    public decimal LineTotal => UnitPrice * Quantity;
}

This is the final form; in the first commit Isbn was still a string and there was no LineTotal. The mechanics: define the record, turn every place that creates the object from an object initializer into a constructor call (in the tests I did that with one regex replacement), build. Since the properties of a positional record are init, a line can't be changed after it's created. The C# page for Visual Studio Code lists a refactoring called "Convert class to record" for this conversion.

Moving from class to record has an effect beyond encapsulation: equality. A record considers two instances equal when their fields are equal; a class looks at the reference. If there is code that uses Items.Contains(item), Remove(item) or a HashSet, its behavior changes. Our code had none; if yours does, this step is also a Change Reference to Value and needs a test.

Encapsulate Collection: A Read View, Not a List

The goal of Encapsulate Collection is that the class owning the collection always knows what gets added to it. The mechanics: move the collection into a private field, expose a read-only view, offer methods for changes (we already had AddItem), move callers that change it directly to the method. Once the property type became IReadOnlyList<OrderItem>, the compiler immediately pointed at the place that changed the list directly:

tests/Bookstore.Ordering.Tests/OrderCharacterizationTests.cs(42,21): error CS1061: 'IReadOnlyList<OrderItem>' does not contain a definition for 'Add' and no accessible extension method 'Add' accepting a first argument of type 'IReadOnlyList<OrderItem>' could be found (are you missing a using directive or an assembly reference?)

There is a common mistake here. In my first attempt the property returned the inner list as is: public IReadOnlyList<OrderItem> Items => _items;. The type is read-only, but the object itself is still a List. To test that, I turned the characterization test into a cast-and-add, and the test passed; the total stayed at 12.45 again. The door wasn't closed, only its handle was removed. When the property returned _items.AsReadOnly(), the same test turned red:

failed Bookstore.Ordering.Tests.OrderCharacterizationTests.Total_ignores_items_added_directly_to_the_list (0ms)
  Xunit.MicrosoftTestingPlatform.XunitException: System.InvalidCastException : Unable to cast object of type 'System.Collections.ObjectModel.ReadOnlyCollection`1[Bookstore.Ordering.OrderItem]' to type 'System.Collections.Generic.List`1[Bookstore.Ordering.OrderItem]'.

This time a red test is good news: the way to change it from outside really is closed. I rewrote the test to describe the new behavior; its final form checks that the cast throws and the total doesn't change:

private readonly List<OrderItem> _items = [];

public IReadOnlyList<OrderItem> Items => _items.AsReadOnly();

[Fact]
public void Items_cannot_be_changed_by_casting_to_List()
{
    Order order = new();
    order.AddItem(new OrderItem(Isbn.Parse("978-0-441-01359-3"), "Dune", 12.45m, 1));

    Assert.Throws<InvalidCastException>(() => ((List<OrderItem>)order.Items).Add(
        new OrderItem(Isbn.Parse("9780261103344"), "The Hobbit", 149.90m, 2)));
    Assert.Equal(12.45m, order.Total);
}

AsReadOnly() creates a small wrapper object on each call; if that's a problem in a loop that runs very often, keeping the wrapper in a field is enough. In Entity Framework Core this structure has to be mapped through the _items backing field; how is covered in the DDD series post "Persisting Aggregates with EF Core 10: Backing Fields, Owned/Complex Types, and Concurrency".

Replace Temp with Query, Replace Derived Variable with Query and Split Variable

item.UnitPrice * item.Quantity was computed into a temporary variable in two methods. Replace Temp with Query turns such a variable into a query, a method or a property: make sure the variable is assigned once, extract the right-hand side into a property, replace the variable's uses with the property, delete the variable. The result is OrderItem.LineTotal; now the only place that knows the line amount is the line itself. If the variable only makes sense in that method, Extract Variable is enough; if the same calculation repeats in more than one place, a query is better.

Replace Derived Variable with Query is this post's main win. Total was a value derived from other data but stored separately; it had to be updated by hand on every change, and the moment someone forgot, it showed a wrong number. Turn it into a query and there is nothing left to forget:

public decimal Total => _items.Sum(i => i.LineTotal);

public decimal DiscountedTotal(decimal rate)
{
    decimal discount = Total * rate;
    decimal net = Total - discount;
    return Math.Round(net, 2, MidpointRounding.AwayFromZero);
}

Computing the total on every read is not a measurable cost for a cart or an order. For a collection with millions of items, or on a frequently called path, measure and decide. On the EF Core side there is a warning: Total is no longer a column in the database, so a query like Where(o => o.Total > 500) can't be translated to SQL. We'll see that trap with real output in the thirteenth post, together with Move Function.

The same code has a Split Variable too. In the old DiscountedTotal, temp held first the discount, then the net amount. A variable that carries more than one meaning is impossible to name; Split Variable opens a separate variable for each meaning: make the first assignment to a new variable with the right name, bind the following uses to it, repeat for the second assignment. Loop counters and accumulators like sum += ... are the exception; they update one meaning step by step. Reassigning a parameter, as in rate = rate / 100, is a variant of the same smell.

Replace Primitive with Object: Isbn

The ISBN travels as a string, its check digit is never validated, and two spellings of the same number are two different values. That is the Primitive Obsession described in the Clean Code series post "Objects or Data Structures?". Replace Primitive with Object replaces the primitive with a small type that knows its own rules. The Percentage in the fourth post was an example of the same refactoring.

I split this step into two commits on purpose. In the first, Isbn only wrapped the string, with no normalization or validation; the tests stayed the same and green, so it was a pure refactoring. In the second, I gave the type its real job:

public sealed record Isbn
{
    private Isbn(string value) => Value = value;

    public string Value { get; }

    public static Isbn Parse(string text) =>
        TryParse(text, out Isbn? isbn) ? isbn : throw new FormatException($"'{text}' is not a valid ISBN-13.");

    public static bool TryParse(string? text, [NotNullWhen(true)] out Isbn? isbn)
    {
        isbn = null;
        string digits = (text ?? "").Replace("-", "").Replace(" ", "");
        if (digits.Length != 13 || !digits.All(char.IsAsciiDigit))
            return false;

        // ISBN-13 check digit: sum weighted by 1 and 3
        int sum = 0;
        for (int i = 0; i < 12; i++)
            sum += (digits[i] - '0') * (i % 2 == 0 ? 1 : 3);
        if ((10 - sum % 10) % 10 != digits[12] - '0')
            return false;

        isbn = new Isbn(digits);
        return true;
    }

    public override string ToString() => Value;
}

When I ran the tests, the one test I expected turned red:

failed Bookstore.Ordering.Tests.OrderCharacterizationTests.Contains_compares_isbn_strings_as_written (0ms)
  Assert.False() Failure
  Expected: False
  Actual:   True

Behavior changed because we wanted it to. I updated the test's name and expectation to the new behavior, added a separate test project for Isbn with hyphenated, spaced, wrong-check-digit and short inputs, and committed it as fix:. This split helps a lot in review: in refactor: commits the reviewer asks "is the behavior the same", in the fix: commit "is the new behavior right".

The reason I made the type a record is equality. I checked with a small file-based app what would happen if the same type were written as an ordinary class; it's samples/equality.cs in the repo:

IsbnClass a = new("9780441013593");
IsbnClass b = new("9780441013593");
IsbnRecord c = new("9780441013593");
IsbnRecord d = new("9780441013593");

Console.WriteLine($"class : a == b -> {a == b}, Equals -> {a.Equals(b)}");
Console.WriteLine($"record: c == d -> {c == d}, Equals -> {d.Equals(c)}");

HashSet<IsbnClass> seen = [a];
Console.WriteLine($"class in HashSet: {seen.Contains(b)}");

public sealed class IsbnClass(string value)
{
    public string Value { get; } = value;
}

public sealed record IsbnRecord(string Value);

$ dotnet run equality.cs
class : a == b -> False, Equals -> False
record: c == d -> True, Equals -> True
class in HashSet: False

With the class version, Contains would no longer find any ISBN; a worse result than the old string-based code. readonly record struct is an option too, but default(Isbn) produces a value that skipped validation and has a null Value; that's why I chose a sealed record class here.

Change Reference to Value and Change Value to Reference

The third surprise was about sharing an object. Address is a reference: the customer and the order hold the same object, and when one changes it, the other changes too. Change Reference to Value turns such an object into an immutable value: make its fields init, rewrite every place that wants to change the value so that it creates a new value, define equality by fields. In C#, record does all of that. When Address became a positional record, the compiler found the line that changed the address in place:

tests/Bookstore.Ordering.Tests/OrderCharacterizationTests.cs(63,9): error CS8852: Init-only property or indexer 'Address.City' can only be assigned in an object initializer, or on 'this' or 'base' in an instance constructor or an 'init' accessor.

Changing the address now creates a new value, and the order keeps its old value:

ada.Address = ada.Address with { City = "Izmir" };

Assert.Equal("Ankara", order.ShippingAddress.City);
Assert.Equal("Izmir", ada.Address.City);

This step is not a pure refactoring either: the past order's address no longer changes. An address changing on a shipped order was a bug, the decision was made with the Product Owner, and the commit was again a fix:. When you apply Change Reference to Value, first look for code that relies on "change it in one place, see it everywhere"; if there is any, either that usage is a bug, as in our case, or the object is not a value but a truly shared entity.

The example of the second case is Customer. The bulk order import created a new Customer object for each row; loyalty points given to two orders of the same customer stayed in two separate copies, and the customer's total was 20 nowhere. Change Value to Reference does the opposite: one object for everything with the same identity. A repository keeps objects by identity and returns the same instance on every request:

public sealed class CustomerRepository
{
    private readonly Dictionary<int, Customer> _byId = [];

    public Customer Get(int id, string name)
    {
        if (!_byId.TryGetValue(id, out Customer? customer))
        {
            customer = new Customer { Id = id, Name = name };
            _byId.Add(id, customer);
        }
        return customer;
    }
}

In the test, two orders share the customer, checked with Assert.Same, and the points go up to 20. In a real application EF Core's change tracker usually does this: entities loaded with the same key inside the same DbContext are a single instance. When you load with AsNoTracking(), every row is a separate copy, which means you applied Change Reference to Value without noticing.

All the Steps and IDE Support

All the steps of the post are in these commits; the two commits starting with fix: change behavior on purpose, the others don't:

$ git log --oneline
a61879a refactor: customers come from CustomerRepository (value to reference)
c10b1c7 fix: Address is an immutable value; orders keep the address they were shipped to
597b198 refactor: encapsulate Customer and Order fields as properties
09404cf fix: Isbn.Parse validates the check digit and normalizes hyphens
e7dab67 refactor: replace string ISBN with Isbn type (text kept as written)
003a834 refactor: split variable temp into discount and net
98e2415 refactor: replace derived Total field with a query
63446aa refactor: replace temp lineTotal with OrderItem.LineTotal
1b265be refactor: encapsulate Order.Items (read-only view, AddItem only)
ebd9792 refactor: encapsulate OrderItem as a record
2580684 refactor: encapsulate OrderSettings.MaxQuantityPerLine
cee55b2 build: enable CA1002, CA1051, CA2211, CA2227 as warnings
7202088 test: characterize Order (legacy, incl. surprising behavior)

The IDE column is based on the Microsoft Learn and JetBrains docs; I didn't run the IDEs for this post. The analyzer column comes from the real dotnet build output above. In Visual Studio, Encapsulate field opens with Ctrl+R, Ctrl+E and offers two options: switch the usages to the property, or keep using the field.

The summary of the table: the only thing the IDE does automatically in this family is turning a field into a property. The rest involves design decisions, such as which operations to offer, what equality is based on, whether the object is shared, and only tests make those safe.

Common Pitfalls

Doing Encapsulate Collection only at the type level. Returning IReadOnlyList doesn't close the door; if you return the inner List, it can be changed with a cast. Return AsReadOnly(), a copy or an immutable collection, and pin that with a test.

Forgetting equality on a value object. With an Isbn written as an ordinary class, ==, Contains and HashSet look at the reference; as the output above shows, the same ISBN can't be found. Use a record or write Equals and GetHashCode together.

Putting a refactoring and a fix in the same commit. Two steps in this post changed behavior. Had I put the normalization in the commit that introduced Isbn, the answer to the review question "why does this commit change a test" would have been lost in the diff.

Missing the code that relies on sharing when turning a shared object into a value. Before Change Reference to Value, find every place where the object is changed in place; CS8852 errors are the easy way to get the compiler to produce that list.

Forgetting that turning a field into a property breaks binary compatibility. Inside your own solution it's fine; if you publish a NuGet package, it's a major version change.

A Small Experiment

★☆☆ Add CA1002, CA1051, CA2211 and CA2227 as warning to .editorconfig in your own project and run dotnet build --no-incremental. Which rule showed up most? Which one would you take your first Encapsulate step with?

★★☆ Find a collection property on an entity. Test with a cast whether it returns the inner list; if it does, apply Encapsulate Collection and offer add/remove methods.

★★★ Pick an identifier or code that travels as a string in your code: an order number, a coupon code, a tax number. Apply Replace Primitive with Object in two commits: first only the wrapping type, then normalization and validation. How many tests changed in the second commit, and was each one a bug?

The data now protects its own rules, but responsibilities still gather in the wrong places: the invoice calculation lives in three places, and the CSV import parses, validates and saves in a single loop. In the next post we move on to the refactorings that group responsibilities: Introduce Parameter Object, Preserve Whole Object, Combine Functions into Class and Transform, Split Phase, Extract and Inline Class, Hide Delegate, Remove Middle Man and Substitute Algorithm.