Refactoring Catalog 7: Dealing with Inheritance — Pull Up/Push Down, Extract Superclass, Replace Type Code with Subclasses and Replace Subclass with 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, and the database sample uses Entity Framework Core 10.0.15 (SQL Server provider).

Guess First: How Many Books Are on the Wishlist?

The seventh post of the catalog block, and the last post of the series, covers eleven refactorings that work with inheritance: Pull Up Method, Pull Up Field, Pull Up Constructor Body, Push Down Method, Push Down Field, Extract Superclass, Collapse Hierarchy, Replace Type Code with Subclasses, Remove Subclass, Replace Subclass with Delegate and Replace Superclass with Delegate. The names are as on refactoring.com/catalog; the explanations and examples are ours. The scene is again Bookstore, the online bookstore used as the shared example across this site's series: the book classes in the Catalog module and the wishlist in Ordering. To avoid keeping the same book twice, the wishlist derives from List<string>:

// A customer's wishlist of ISBNs; the same book is kept only once.
public class Wishlist : List<string>
{
    public new void Add(string isbn)
    {
        if (!Contains(isbn))
            base.Add(isbn);
    }
}

The code adds books from three different places: the "add to wishlist" button on the page, an operation that moves every book in the cart to the list, and a shared helper method that takes an ICollection<string>:

Wishlist wishlist = [];
wishlist.Add("9780441013593");
wishlist.Add("9780441013593");
wishlist.AddRange(["9780441013593"]);
ICollection<string> collection = wishlist;
collection.Add("9780441013593");

Console.WriteLine(wishlist.Count);

The same ISBN was added four times. What gets printed? And if the list came from an API as JSON with the same ISBN twice in it, how many books would the deserialized Wishlist hold?

Extract Superclass and the Pull Ups

On the book side there's another piece of old code. Printed books were sold first; the e-book team started from a copy of PrintedBook, and the two classes repeat the same fields, the same constructor check and the same price calculation:

public class PrintedBook
{
    public PrintedBook(string isbn, string title, decimal price, int weightGrams)
    {
        if (string.IsNullOrWhiteSpace(title))
            throw new ArgumentException("A book needs a title.", nameof(title));
        Isbn = isbn;
        Title = title.Trim();
        Price = price;
        WeightGrams = weightGrams;
    }

    public string Isbn { get; }
    public string Title { get; }
    public decimal Price { get; }
    public DateOnly? ReleaseDate { get; init; }
    public int WeightGrams { get; }

    public decimal VatRate => 0.10m;

    public decimal PriceWithVat() => Math.Round(Price * (1 + VatRate), 2, MidpointRounding.AwayFromZero);

    public decimal ShippingWeightKg() => WeightGrams / 1000m;

    public string Delivery(DateOnly today) =>
        ReleaseDate > today ? $"Ships on {ReleaseDate:yyyy-MM-dd}"
        : WeightGrams > 1000 ? "Cargo (heavy), 3 days"
        : "Cargo, 2 days";
}

public class EBook
{
    // Constructor, Isbn, Title, Price, ReleaseDate, PriceWithVat and ShippingWeightKg are identical.
    public string FileFormat { get; }

    // The cargo calculator asks every book for its weight.
    public int WeightGrams => 0;

    public decimal VatRate => 0.20m;

    public string Delivery(DateOnly today) =>
        ReleaseDate > today ? $"Available on {ReleaseDate:yyyy-MM-dd}"
        : $"Download ({FileFormat})";
}

The safety net is a Verify snapshot holding the price, weight and delivery text for six different books, plus a test that rejects an empty title. Extract Superclass moves the shared part of two classes that do similar jobs into a new base class; it answers the Duplicated Code and Alternative Classes with Different Interfaces smells from the Clean Code series' "Code Smell Catalog" post. The first commit only created an empty abstract class Book and derived both classes from it. Then came three separate Pull Ups, each its own commit:

Pull Up Field: Isbn, Title, Price and ReleaseDate moved to the base class. C# needed a small intermediate step: a subclass constructor can't assign a get-only property in the base class. For one commit the properties were { get; protected init; }; an init accessor can also be called from a derived class's constructor.

Pull Up Constructor Body: the title check and the assignments moved into the base class's constructor, the subclasses call it with : base(isbn, title, price), and the properties became { get; } again. The mechanics: create a constructor in the base class (if there isn't one), move the shared lines from the subclasses into it, call the base from the subclasses, test. If the shared lines come after a subclass-specific assignment, use Slide Statements to move them to the top first.

Pull Up Method: PriceWithVat was identical in both classes; only the VatRate it used was different. The method moved to the base, VatRate became an abstract property and the subclasses override it. The bodies of Delivery differ, but its signature is the same; I pulled it up as abstract too, so it can be called through a variable of type Book.

public abstract class Book
{
    protected Book(string isbn, string title, decimal price)
    {
        if (string.IsNullOrWhiteSpace(title))
            throw new ArgumentException("A book needs a title.", nameof(title));
        Isbn = isbn;
        Title = title.Trim();
        Price = price;
    }

    public string Isbn { get; }
    public string Title { get; }
    public decimal Price { get; }
    public DateOnly? ReleaseDate { get; init; }

    public abstract decimal VatRate { get; }

    public decimal PriceWithVat() => Math.Round(Price * (1 + VatRate), 2, MidpointRounding.AwayFromZero);

    public abstract string Delivery(DateOnly today);
}

Push Down and Collapse Hierarchy: Going Back Is a Refactoring Too

In Visual Studio's "Pull member(s) up to new base class" or Rider's Extract Superclass dialog, one more member shows up that's identical in both classes: ShippingWeightKg. Because it looked the same, I pulled it up too; WeightGrams became abstract in the base, and the e-book overrode it by returning 0. The tests were green and the commit went in. But an e-book has no weight; that 0 is a workaround written back when the cargo calculator asked every book for its weight. The same text doesn't mean the same knowledge. A subclass that doesn't use a member of its base, or fills it with a meaningless value, is the Refused Bequest smell from the Clean Code series.

Push Down Method and Push Down Field move a member of the base class down to the subclass that really uses it; they're the inverses of the Pull Ups. The mechanics: copy the member into every subclass that needs it, delete it from the base, test, delete it from the subclasses that don't use it. After the deletion the compiler pointed at the only place asking an e-book for its weight, which was the characterization test itself:

tests/Bookstore.Catalog.Tests/BookTests.cs(31,70): error CS1061: 'EBook' does not contain a definition for 'ShippingWeightKg' and no accessible extension method 'ShippingWeightKg' accepting a first argument of type 'EBook' could be found (are you missing a using directive or an assembly reference?)

The test no longer asks for an e-book's weight. Since Verify doesn't write default values (0, null), the snapshot had no weight line for e-books anyway, and the file didn't change. Had production code asked an e-book for its weight, the same error would have pointed at it too, and the decision there would be a design decision: does that code really want every book's weight, or only the ones that go by cargo?

The old code had one more subclass: SignedPrintedBook. It derived from PrintedBook, and its only difference was a property holding the name of the person who signed it; it overrode no behavior. Collapse Hierarchy merges a base class and a subclass that aren't different enough into one class. The mechanics: decide which one stays, move the other's members to it with Pull Up or Push Down, point references at the remaining class, delete the empty class. PrintedBook stayed, and the signature is a property:

// Before
public class SignedPrintedBook : PrintedBook
{
    public SignedPrintedBook(string isbn, string title, decimal price, int weightGrams, string signedBy)
        : base(isbn, title, price, weightGrams)
    {
        SignedBy = signedBy;
    }

    public string SignedBy { get; }
}

// After (inside PrintedBook)
public string? SignedBy { get; init; }

// Usage
new PrintedBook("9780261103344", "The Hobbit", 349.90m, 700) { SignedBy = "Grace" };

Collapse Hierarchy is the inverse of Extract Superclass. It answers the Lazy Element and Speculative Generality smells from the Clean Code series: a subclass created because "signed books will have a different delivery one day", which never filled up.

Replace Subclass with Delegate and Remove Subclass

The hierarchy is clean now: the shared part is in the base, the differences are in the subclasses. So why one more step? Three limits of inheritance are real in Bookstore. First, an object's class can't change. Continuing to sell a book whose printed edition has run out as an e-book or as print-on-demand under the same catalog record means throwing away the existing object and creating a new subclass object. Second, inheritance works along a single axis. If a second axis arrives next to the format (say, an institutional edition sold to schools), every combination needs a subclass. Third, subclasses are tightly bound to the internals of the base class; the SOLID and Design Principles series' "Composition over Inheritance: Fragile Base Class, Delegation and Pluggable Behavior" post covers the price of that coupling in detail.

Replace Subclass with Delegate moves the subclasses' differing behavior into a separate delegate object, and the base class holds that object. The mechanics: create an empty delegate type built from the subclass-specific data; add a field holding the delegate to the base class and have the subclasses fill it when they're created; move the subclass methods to the delegate one by one (Move Function) and forward to the delegate from the base class; delete the subclasses once they're empty. If you create subclasses with new in many places, applying Replace Constructor with Factory Function from the previous post first makes the job easier. I moved VatRate and Delivery in one commit; a commit per method would also have worked.

public interface IBookFormat
{
    decimal VatRate { get; }
    string Delivery(DateOnly? releaseDate, DateOnly today);
}

public sealed record PrintedFormat(int WeightGrams) : IBookFormat
{
    public string? SignedBy { get; init; }

    public decimal VatRate => 0.10m;

    public decimal ShippingWeightKg() => WeightGrams / 1000m;

    public string Delivery(DateOnly? releaseDate, DateOnly today) =>
        releaseDate > today ? $"Ships on {releaseDate:yyyy-MM-dd}"
        : WeightGrams > 1000 ? "Cargo (heavy), 3 days"
        : "Cargo, 2 days";
}

public sealed record DigitalFormat(string FileFormat) : IBookFormat
{
    public decimal VatRate => 0.20m;

    public string Delivery(DateOnly? releaseDate, DateOnly today) =>
        releaseDate > today ? $"Available on {releaseDate:yyyy-MM-dd}"
        : $"Download ({FileFormat})";
}

public sealed class Book
{
    public Book(string isbn, string title, decimal price, IBookFormat format)
    {
        if (string.IsNullOrWhiteSpace(title))
            throw new ArgumentException("A book needs a title.", nameof(title));
        Isbn = isbn;
        Title = title.Trim();
        Price = price;
        Format = format;
    }

    public string Isbn { get; }
    public string Title { get; }
    public decimal Price { get; }
    public DateOnly? ReleaseDate { get; init; }
    public IBookFormat Format { get; }

    public decimal VatRate => Format.VatRate;

    public decimal PriceWithVat() => Math.Round(Price * (1 + VatRate), 2, MidpointRounding.AwayFromZero);

    public string Delivery(DateOnly today) => Format.Delivery(ReleaseDate, today);
}

The last step is a separate refactoring: Remove Subclass. After the move to the delegate, PrintedBook and EBook held only a constructor and a few properties forwarding to the delegate. Remove Subclass replaces subclasses that don't do enough with a field in the base class; here that field is Format. The mechanics: change the places that create the subclasses to create the base class (and add a field if a value distinguishes the subclass), turn type checks (is PrintedBook) into checks on the field, delete the subclasses. Usage became new Book("9780441013593", "Dune", 120m, new PrintedFormat(450)), and not a single line of the snapshot changed.

The formats are records and immutable. The Format property is get-only for now; when a need like print-on-demand arrives, making it changeable is a one-method job, while writing it today would break YAGNI. I built the delegate hierarchy with an interface too, because there's no shared behavior, only a shared question; in Fowler's description the delegates can use inheritance among themselves as well.

Had this hierarchy been mapped to a database, the work wouldn't end with the code. Entity Framework Core's default inheritance strategy is TPH: the whole hierarchy in one table, the row's type in a discriminator column. For a simplified model of the old hierarchy I looked at the table EF Core generates without connecting to a database (samples/tph.cs, GenerateCreateScript()):

CREATE TABLE [Books] (
    [Id] int NOT NULL IDENTITY,
    [Isbn] nvarchar(13) NOT NULL,
    [Title] nvarchar(max) NOT NULL,
    [Price] decimal(10,2) NOT NULL,
    [Discriminator] nvarchar(13) NOT NULL,
    [FileFormat] nvarchar(max) NULL,
    [WeightGrams] int NULL,
    CONSTRAINT [PK_Books] PRIMARY KEY ([Id])
);

The Discriminator column holds the class name (PrintedBook, EBook). Once the subclasses are gone, this column loses its meaning: you need a new column to store which format the delegate is, convert the existing values into it, and delete the old column only after every reader has moved to the new one. That's the same expand-contract sequence as in the seventh post. With TPT, where every subclass has its own table, the job is even bigger, because the tables have to be merged. The difference between the two strategies is in the EF Core series' "Inheritance Mapping in EF Core: TPH, TPT and When to Choose Which" post. I didn't write the mapping for the new delegate-based model for this post.

Replace Superclass with Delegate: The Wishlist

The problem with Wishlist : List<string> is the opposite of Refused Bequest: the subclass has to accept all of its inheritance. In .NET 10, List<string> has 32 distinctly named public instance methods (excluding those from object and the property accessors); I counted them with reflection. Insert, AddRange, Sort, RemoveAll and all the others are now part of the wishlist's API too, and none of them knows about the duplicate check. Replace Superclass with Delegate turns the base class into a private field and forwards only the operations that are really needed. The mechanics: create a field of the base class's type in the subclass, write a method forwarding to that field for each base method callers use, remove the inheritance, test.

public class Wishlist : IReadOnlyCollection<string>
{
    private readonly List<string> _isbns = [];

    public int Count => _isbns.Count;

    public void Add(string isbn)
    {
        if (!_isbns.Contains(isbn))
            _isbns.Add(isbn);
    }

    public void AddRange(IEnumerable<string> isbns) => _isbns.AddRange(isbns);

    public bool Contains(string isbn) => _isbns.Contains(isbn);

    public IEnumerator<string> GetEnumerator() => _isbns.GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

In the first commit AddRange still adds straight to the list as before; a refactoring doesn't change behavior. Thanks to Add and IEnumerable, a collection expression (Wishlist wishlist = [];) still works. The compiler, on the other hand, showed the door that closed:

tests/Bookstore.Ordering.Tests/WishlistTests.cs(27,42): error CS0266: Cannot implicitly convert type 'Bookstore.Ordering.Wishlist' to 'System.Collections.Generic.ICollection<string>'. An explicit conversion exists (are you missing a cast?)

The Answer: Three Books, Two from JSON

The opening code printed 3. The two Add calls added one book, but AddRange and ICollection<string>.Add never saw the hidden method. The new keyword isn't an override but method hiding: which method runs is decided at compile time from the variable's type. List<string>'s own methods, calls through an interface and a serializer all call the base class's Add. In a small experiment I deserialized ["9780441013593","9780441013593"] into a Wishlist with System.Text.Json; the result held two books. The characterization test pinned this surprising behavior on purpose with Assert.Equal(3, ...).

After the move to the delegate, the ICollection door closed with a compile error and that line came out of the test. AddRange not checking for duplicates is now a plainly visible bug; I fixed it in a separate fix: commit by having it call Add. That split is the two hats rule from the first post itself: change the structure while keeping behavior first, then make the fix that became easy as a separate step. A note on the JSON side too: Wishlist is no longer a collection; if it's read from an API, it should be built from a DTO or get its own converter. I didn't do that in this post.

Replace Type Code with Subclasses: The Payment Method

In the conditionals post we moved the switches that decided based on the payment method into IPaymentOption classes, but left the PaymentMethod enum in Cart. Every call first went from enum to object with PaymentOptions.For(cart.Payment), and CheckoutPolicy still had comparisons like cart.Payment == PaymentMethod.BankTransfer. Replace Type Code with Subclasses replaces a type code that drives behavior (an enum, string or int) with subclasses; it answers the Repeated Switches and Primitive Obsession smells. The mechanics: put the type code behind a property, create a subclass for each value and a factory that returns the right one, move the checks that use the type code into the subclasses or turn them into type checks, remove the type code field.

The subclasses and the factory already existed. When I changed the field's type in Cart to IPaymentOption, the compiler listed the four remaining places:

src/Bookstore.Ordering/CheckoutPolicy.cs(44,85): error CS0029: Cannot implicitly convert type 'Bookstore.Ordering.PaymentMethod' to 'Bookstore.Ordering.IPaymentOption'
src/Bookstore.Ordering/CheckoutPolicy.cs(62,38): error CS0019: Operator '==' cannot be applied to operands of type 'IPaymentOption' and 'PaymentMethod'
src/Bookstore.Ordering/CheckoutPolicy.cs(70,64): error CS1503: Argument 1: cannot convert from 'Bookstore.Ordering.IPaymentOption' to 'Bookstore.Ordering.PaymentMethod'
src/Bookstore.Ordering/CheckoutPolicy.cs(74,69): error CS1503: Argument 1: cannot convert from 'Bookstore.Ordering.IPaymentOption' to 'Bookstore.Ordering.PaymentMethod'

public sealed record Cart(Customer Customer, IReadOnlyList<CartLine> Lines, IPaymentOption Payment)
{
    public decimal Subtotal => Lines.Sum(l => l.UnitPrice * l.Quantity);
}

private static bool IsEligibleForDiscount(Cart cart) =>
    cart is not ({ Customer.IsGuest: true } or { Lines.Count: 0 } or { Payment: CashOnDeliveryPayment });

// CheckoutError
if (cart.Customer.IsGuest && cart.Payment is BankTransferPayment)
    return "Guests cannot pay by bank transfer.";

public decimal PaymentFee(Cart cart) => cart.Payment.Fee(cart.Subtotal);

C#'s type patterns (is BankTransferPayment, { Payment: CashOnDeliveryPayment }) took the place of the enum comparisons directly. The enum didn't disappear, though: it's the value stored in the database and the API, and PaymentOptions.For is the single place at the system's boundary that turns the enum into an object. Fowler's name says "subclasses"; here the common type is an interface, because the payment methods share no behavior. If the payment method can change for a cart (the customer drops the card on the payment screen and picks a bank transfer), the subclasses should derive not from Cart itself but from the object it carries; that's exactly what we did here.

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. The compiler lines come from the real output above.

$ git log --oneline
225c4c6 chore: samples/tph.cs (how EF Core stores the old hierarchy)
c1f508a refactor: replace type code Cart.Payment (PaymentMethod) with IPaymentOption classes
1c3339f fix: Wishlist.AddRange keeps a book once
74f0f26 refactor: replace superclass List<string> with a private list in Wishlist
dac1460 refactor: remove subclasses PrintedBook and EBook (Book holds its format)
3c6985e refactor: replace subclass behavior with delegate IBookFormat (PrintedFormat, DigitalFormat)
74b4c48 refactor: collapse hierarchy (SignedPrintedBook into PrintedBook.SignedBy)
20b35ca refactor: push down WeightGrams and ShippingWeightKg to PrintedBook
8e0f742 refactor: pull up ShippingWeightKg (identical in both classes)
6a49bf3 refactor: pull up method PriceWithVat (VatRate and Delivery abstract)
e502546 refactor: pull up constructor body (title check and assignments)
8144118 refactor: pull up fields Isbn, Title, Price, ReleaseDate
e233dca refactor: extract superclass Book (empty)
1318a32 test: characterize books and wishlist (legacy)

The pull up and push down commits sit side by side, and I left them that way on purpose: showing why a step was undone is more useful to whoever reads the code later than a cleaned-up history.

66 Refactorings, Seven Families: Where Is the Catalog?

With this post, all 66 names on refactoring.com/catalog have appeared at least once in this series on Bookstore code, with the tests run before and after. By family:

• Basic refactorings (post 10, 9): Extract/Inline Function, Extract/Inline Variable, Change Function Declaration, Rename Variable, Rename Field, Replace Magic Literal, Replace Inline Code with Function Call.
• Encapsulation and data (post 11, 9): Encapsulate Variable/Record/Collection, Replace Primitive with Object, Replace Temp with Query, Replace Derived Variable with Query, Split Variable, Change Reference to Value, Change Value to Reference.
• Grouping responsibilities (post 12, 10): Introduce Parameter Object, Preserve Whole Object, Combine Functions into Class/Transform, Split Phase, Extract/Inline Class, Hide Delegate, Remove Middle Man, Substitute Algorithm.
• Moving things (post 13, 8): Move Function, Move Field, Move Statements into Function, Move Statements to Callers, Slide Statements, Split Loop, Replace Loop with Pipeline, Remove Dead Code.
• Conditionals (post 14, 7): Decompose Conditional, Consolidate Conditional Expression, Replace Nested Conditional with Guard Clauses, Replace Control Flag with Break, Replace Conditional with Polymorphism, Introduce Special Case, Introduce Assertion.
• APIs (post 15, 12): Separate Query from Modifier, Parameterize Function, Remove Flag Argument, Replace Parameter with Query, Replace Query with Parameter, Remove Setting Method, Replace Constructor with Factory Function, Replace Function with Command, Replace Command with Function, Return Modified Value, Replace Error Code with Exception, Replace Exception with Precheck.
• Inheritance (this post, 11): Pull Up Method/Field/Constructor Body, Push Down Method/Field, Extract Superclass, Collapse Hierarchy, Replace Type Code with Subclasses, Remove Subclass, Replace Subclass with Delegate, Replace Superclass with Delegate.

You don't need to memorize the catalog. What you need is two or three candidates that come to mind when you see a smell, and knowing that each of them has an inverse: Pull Up has Push Down, Extract has Inline. A step in the wrong direction can be undone, as long as the tests run at every step.

Common Pitfalls

Producing a "BaseService" dumping ground with Extract Superclass. Collecting everything that looks shared across a few classes into a base class eventually produces a class no subclass uses in full and everyone is afraid to change. The Design Patterns series' "Anti-Patterns: God Object, Service Locator, Singleton Abuse, Anemic Domain Model and Patternitis" post shows where that road ends. Does the shared code really express the same knowledge, or is it the same by coincidence today? If it's the latter, Extract Class or a small helper function is the better fit.

Taking the same text for the same knowledge. ShippingWeightKg was identical character for character in both classes, but in the e-book it was part of a workaround. Before a Pull Up, ask for each subclass: "what does this member mean here?"

Deriving from collection classes and hiding methods with new. Classes like List<T> and Dictionary<TKey, TValue> weren't designed for inheritance; their methods aren't virtual. A method hidden with new is skipped by every call made through an interface or the base type. If you want to add behavior, use a delegate or derive from Collection<T>, which is designed for it.

Changing a hierarchy that's mapped to a database only in code. With TPH the discriminator values, with TPT the tables are part of the data. Adding a subclass, deleting one or moving to a delegate means a migration and a data conversion; go step by step with expand-contract.

Applying Replace Subclass with Delegate to every hierarchy. For a single-axis hierarchy where an object's type doesn't change and the base class is under your control, inheritance is still the shortest path. Move to a delegate when you actually hit one of inheritance's limits.

A Small Experiment

★☆☆ Search your codebase for classes derived from List<T>, Dictionary<TKey, TValue> or HashSet<T>. Which of them hide a method with new? For each, try to write a call that skips the hidden method.

★★☆ For a hierarchy, write down in a table which subclasses really use each member of the base class. Where you see Refused Bequest, apply Push Down and look at the callers the compiler points to.

★★★ Pick a hierarchy mapped as TPH with EF Core. Write the migration for removing one of the subclasses or moving to a delegate: the new column, the data conversion, moving the readers and deleting the old column. How many separate deployments does it spread across?

The series ends here. In the first nine posts we treated refactoring as a discipline: keeping behavior, building a safety net, moving in small steps, breaking dependencies, planning a big change with the Mikado Method and branch by abstraction, changing contracts without breaking them, modernizing Angular code, and measuring and defending technical debt. In the last seven posts we tried the everyday tools of that discipline, the catalog's 66 refactorings, one by one on Bookstore. The two work together: the catalog says what to do, the discipline says when and with what safety net. The next step is in your own codebase, in the next small, tested commit.