Refactoring Catalog 6: Refactoring APIs — Separate Query from Modifier, Remove Flag Argument, Parameterize Function, Factory Functions and Commands

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: Who Breaks When You Add a Parameter with a Default Value?

The sixth post of the catalog block covers twelve refactorings that change the API a class exposes: 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 and Replace Exception with Precheck. The names are as on refactoring.com/catalog; the explanations and examples are ours. The scene is the new Notifications module of Bookstore, the online bookstore used as the shared example across this site's series. The method that sends shipping notifications looks like this:

public class Notifier(Inbox inbox, ISmsGateway sms, IReadOnlyDictionary<int, string> phones, IReadOnlySet<int> smsOptOut)
{
    // 0: done, -1: order without items, -2: no SMS could be sent
    public int SendNotification(Order order, bool viaSms, bool urgent)
    {
        if (order.Items.Count == 0)
            return -1;

        string text = new ShippedMessageBuilder(order).Build();
        inbox.Add(new Notification(order.CustomerId, "shipped", text, DateTimeOffset.UtcNow));
        if (!viaSms)
            return 0;
        if (smsOptOut.Contains(order.CustomerId))
            return -2;

        string phone;
        try
        {
            phone = phones[order.CustomerId];
        }
        catch (KeyNotFoundException)
        {
            return -2;
        }
        sms.Send(phone, urgent ? "URGENT: " + text : text);
        return 0;
    }
}

// Callers
notifier.SendNotification(order, true, false);
notifier.SendNotification(order, false, false);

This class ships as a NuGet package, and one of its consumers is the Institutional Orders team's own package, compiled against version 1.3.0 of Notifications. The product team wants e-mail too, and the shortest route looks like adding one more parameter:

public int SendNotification(Order order, bool viaSms, bool urgent, bool viaEmail = false)

Because the parameter has a default value, every existing call compiles unchanged. So you make it 1.4.0 and release it. The application upgrades Notifications to 1.4.0, while the Institutional Orders package isn't recompiled and stays as it was built against 1.3.0. What happens when the SendNotification(order, true, false) call in that package runs?

Separate Query from Modifier

First the notification inbox. The method that returned a user's unread notifications marked them as read at the same time:

public IReadOnlyList<Notification> GetAndMarkAsRead(int customerId)
{
    List<Notification> unread = _items.Where(n => n.CustomerId == customerId && !n.IsRead).ToList();
    foreach (Notification notification in unread)
        notification.IsRead = true;
    return unread;
}

A method that returns a value and also changes state makes every caller depend on order. If the badge at the top of the page calls this method to show the unread count, the list that comes right after is empty. Even a call typed into the debugger's watch window marks the notifications as read. Separate Query from Modifier splits the method into a query with no side effects and a modifier that returns nothing. It's the refactoring form of Bertrand Meyer's CQS principle; the principle itself is in the SOLID and Design Principles series' "Lesser-Known Principles" post. On the HTTP side, its counterpart is that GET requests are safe: a browser or proxy calling a GET ahead of time must not change data.

The mechanics: copy the method and name it as a query, delete the side effects from the copy; change each call to call the query first and then the modifier; remove the return value from the original method, which is now a modifier.

public IReadOnlyList<Notification> Unread(int customerId) =>
    _items.Where(n => n.CustomerId == customerId && !n.IsRead).ToList();

public void MarkAsRead(IEnumerable<Guid> ids)
{
    HashSet<Guid> toMark = [.. ids];
    foreach (Notification notification in _items.Where(n => toMark.Contains(n.Id)))
        notification.IsRead = true;
}

The modifier's parameter isn't the customer but the ids of the notifications that were shown. If a new notification arrives between the query and the modifier, a notification the user never saw must not count as read. This small question that the split brings up was never asked in the combined method.

Parameterize Function, Replace Query with Parameter and Its Inverse

The two methods that remind a customer who left items in the cart were nearly identical:

public void RemindAfterOneDay(int customerId, DateTimeOffset cartUpdatedAt)
{
    Notification notification = new(customerId, "reminder", "Your cart is waiting for you.", cartUpdatedAt.AddDays(1));
    ApplyQuietHours(notification, IsNight(notification.SendAt));
    inbox.Add(notification);
}

public void RemindAfterThreeDays(int customerId, DateTimeOffset cartUpdatedAt)
{
    Notification notification = new(customerId, "reminder", "Your cart is waiting for you.", cartUpdatedAt.AddDays(3));
    ApplyQuietHours(notification, IsNight(notification.SendAt));
    inbox.Add(notification);
}

private static bool IsNight(DateTimeOffset at) => at.Hour >= 22 || at.Hour < 8;

// Night-time reminders wait until 08:00.
private static void ApplyQuietHours(Notification notification, bool isNight)
{
    if (!isNight)
        return;
    DateTimeOffset day = notification.SendAt.Hour >= 22 ? notification.SendAt.AddDays(1) : notification.SendAt;
    notification.SendAt = new DateTimeOffset(day.Year, day.Month, day.Day, 8, 0, 0, day.Offset);
}

Parameterize Function merges functions that differ only in one value into a single function that takes that value as a parameter; it answers the Duplicated Code smell. The mechanics: pick one of the functions, turn the literal that differs into a parameter ("Introduce Parameter" in Rider and VS Code does this), point the other function's callers at the new function, delete the old one. The result is RemindAfter(int days, int customerId, DateTimeOffset cartUpdatedAt), and a reminder on day five doesn't need a new method.

ApplyQuietHours's isNight parameter, on the other hand, is redundant: every caller computes it the same way from the time it already passes to the method. Replace Parameter with Query stops taking a value the callee can find on its own and moves the calculation into the function. Its inverse is Replace Query with Parameter; in this post we needed both:

// Before: it asks for the time itself
public IReadOnlyList<Notification> DueReminders()
{
    DateTimeOffset now = DateTimeOffset.UtcNow;
    return _items.Where(n => n.Kind == "reminder" && n.SendAt <= now).ToList();
}

// After: the caller passes the time
public IReadOnlyList<Notification> DueReminders(DateTimeOffset now) =>
    _items.Where(n => n.Kind == "reminder" && n.SendAt <= now).ToList();

DateTimeOffset.UtcNow is a global. A method that reads it internally can return a different result for the same input on every call; that's why the old test had to use dates a year in the past and ten years in the future. With the time as a parameter, the test can try the exact boundary, "now" and "one minute later". The decision depends on one question: is the value the function's responsibility or the caller's? isNight is a property of the time, and the function already knows the time; "now" is outside state the function shouldn't know about. In the fifth post we solved the same problem with TimeProvider as a dependency; if something higher up the call chain already knows the time, a plain parameter is simpler.

Return Modified Value, Remove Setting Method and Factory Functions

ApplyQuietHours has another problem: it changes the object it receives, and you can't tell that from its signature. Return Modified Value turns a function that changes a value in place into a function that returns the new value, and leaves the assignment to the caller. The mechanics: make the function return the new value, assign the returned value in the caller, test, delete the assignment inside the function. The data that changes is now visible on the calling line:

public void RemindAfter(int days, int customerId, DateTimeOffset cartUpdatedAt) =>
    inbox.Add(Notification.Reminder(customerId, QuietHoursAdjusted(cartUpdatedAt.AddDays(days))));

private static bool IsNight(DateTimeOffset at) => at.Hour >= 22 || at.Hour < 8;

// Night-time reminders wait until 08:00.
private static DateTimeOffset QuietHoursAdjusted(DateTimeOffset at)
{
    if (!IsNight(at))
        return at;
    DateTimeOffset day = at.Hour >= 22 ? at.AddDays(1) : at;
    return new DateTimeOffset(day.Year, day.Month, day.Day, 8, 0, 0, day.Offset);
}

Since the time is calculated before the notification is created, SendAt no longer needs a setter. Id's setter was also called only once, in Inbox.Add. Remove Setting Method removes the setter of a field that must not change after the object is created and supplies the value in the constructor or a factory. When I deleted the two setters, the compiler showed the remaining assignments:

src/Bookstore.Notifications/Inbox.cs(11,9): error CS0200: Property or indexer 'Notification.Id' cannot be assigned to -- it is read only
src/Bookstore.Notifications/ReminderScheduler.cs(8,9): error CS0200: Property or indexer 'Notification.SendAt' cannot be assigned to -- it is read only

The second one went away with the last step of Return Modified Value. IsRead's setter stays, because being read really is state that changes over the object's lifetime. C# gives you three options: remove the setter entirely ({ get; }), allow it only in an object initializer (init), or hide it from the outside (private set). Don't forget that with init, a record can still be changed when you create a new copy with a with expression.

The constructor changed in the same series of commits. There were new Notification(customerId, "shipped", text, at) calls in three places, and the strings "shipped", "reminder" and "price-drop" were repeated at every caller. Replace Constructor with Factory Function turns constructor calls into named static functions. In C#, a constructor's name is the class's name, its return type is always that class, and it must return a new instance; a factory function has none of these three limits. The mechanics: write a factory function that calls the constructor, move callers to the factory, make the constructor private.

public class Notification
{
    private Notification(int customerId, string kind, string text, DateTimeOffset sendAt)
    {
        CustomerId = customerId;
        Kind = kind;
        Text = text;
        SendAt = sendAt;
    }

    public static Notification Shipped(int customerId, string text, DateTimeOffset at) =>
        new(customerId, "shipped", text, at);

    public static Notification Reminder(int customerId, DateTimeOffset sendAt) =>
        new(customerId, "reminder", "Your cart is waiting for you.", sendAt);

    public static Notification PriceDrop(int customerId, string text, DateTimeOffset at) =>
        new(customerId, "price-drop", text, at);

    public Guid Id { get; } = Guid.NewGuid();
    public int CustomerId { get; }
    public string Kind { get; }
    public string Text { get; }
    public DateTimeOffset SendAt { get; }
    public bool IsRead { get; set; }
}

Rider does this automatically as "Replace Constructor with Factory Method". Familiar examples in .NET are TimeSpan.FromMinutes, Task.FromResult and Guid.NewGuid. In the fifth post we parameterized a constructor to open a door for tests; a factory function closes that door to the outside and leaves a separate, named entrance for each use.

Remove Flag Argument and Public APIs

Anyone reading the call SendNotification(order, true, false) has to go to the method to learn what true and false mean. A flag argument is the caller choosing which code runs with a literal value. Remove Flag Argument creates a named function for each meaningful value. In the fourth post we split Calculate(request, isPreview: true) in two; the difference here is that the method is a package's public API. The mechanics: write a new function for each value of the flag, move callers that pass a literal to the new functions, and delete the old one once no caller passes the flag as a variable. Since we can't move callers outside the package, the last step waits for the next major version:

public enum SmsResult
{
    Sent,
    Skipped
}

// 0: done, -1: order without items, -2: no SMS could be sent
[Obsolete("Use NotifyShipped, NotifyShippedBySms or NotifyShippedBySmsUrgently. SendNotification will be removed in 2.0.")]
public int SendNotification(Order order, bool viaSms, bool urgent)
{
    if (order.Items.Count == 0)
        return -1;
    if (!viaSms)
    {
        NotifyShipped(order);
        return 0;
    }
    SmsResult result = urgent ? NotifyShippedBySmsUrgently(order) : NotifyShippedBySms(order);
    return result == SmsResult.Sent ? 0 : -2;
}

public void NotifyShipped(Order order) => PostShipped(order);

public SmsResult NotifyShippedBySms(Order order) => TextShipped(order, PostShipped(order));

public SmsResult NotifyShippedBySmsUrgently(Order order) => TextShipped(order, "URGENT: " + PostShipped(order));

private string PostShipped(Order order)
{
    if (order.Items.Count == 0)
        throw new ArgumentException("A shipped order must have at least one item.", nameof(order));
    string text = ShippedText(order);
    inbox.Add(Notification.Shipped(order.CustomerId, text, DateTimeOffset.UtcNow));
    return text;
}

private SmsResult TextShipped(Order order, string text)
{
    if (smsOptOut.Contains(order.CustomerId) || !phones.TryGetValue(order.CustomerId, out string? phone))
        return SmsResult.Skipped;
    sms.Send(phone, text);
    return SmsResult.Sent;
}

The flags didn't disappear entirely: urgent is now a string prefix passed to a private function. What matters is that every call from outside says what it does by name. The old method forwards to the new ones but keeps its own contract: it still returns -1 and -2 and doesn't throw. The characterization snapshot still calls the old method and not a single line of it changed; the compiler also lists every place that uses it:

tests/Bookstore.Notifications.Tests/NotifierTests.cs(15,32): warning CS0618: 'Notifier.SendNotification(Order, bool, bool)' is obsolete: 'Use NotifyShipped, NotifyShippedBySms or NotifyShippedBySmsUrgently. SendNotification will be removed in 2.0.'

In the test I silenced this warning on purpose with #pragma warning disable CS0618; that test proves the old contract is kept until 2.0. The version number follows from this: adding new methods is backward compatible, so a minor version (1.4.0); deleting the old one is a major version (2.0.0). The rules of semantic versioning are in the Versioning and Release Management series' "What Does a Version Number Say?" post.

The Answer: MissingMethodException

A parameter with a default value is a compiler feature. When the caller is compiled, the compiler writes the missing argument into the call site; at run time the method is a single signature with four parameters. The three-parameter signature no longer exists. I tried this with a small library and a console app that uses it (samples/binary-compat): the app is built against the library standing in for 1.3.0, then only the library's DLL is replaced with the 1.4.0 one:

$ dotnet build App -o out
$ dotnet out/App.dll
order 42: inbox + sms

$ dotnet build Lib -p:LibVersion=2 -o out
$ dotnet out/App.dll
Unhandled exception. System.MissingMethodException: Method not found: 'System.String Bookstore.Notifications.Client.ShippingNotifier.SendNotification(Int32, Boolean, Boolean)'.
   at Program.<Main>$(String[] args)

$ dotnet build App -p:LibVersion=2 -o out
$ dotnet out/App.dll
order 42: inbox + sms

The recompiled app works fine; the change is source compatible and binary breaking. In the NuGet world this is exactly the "intermediate package that isn't recompiled" scenario, and the error only shows up when that code path runs, in production. The right way is what we did above: add a new overload or a new method. The .NET SDK's package validation compares against the previous version's package and catches this case with error CP0002; the Versioning and Release Management series' "Publishing a NuGet Package" post covers the setup. I didn't run package validation for this post.

Replace Error Code with Exception and Replace Exception with Precheck

The old method reported two different kinds of failure the same way, with a negative number. Sending a shipping notification for an order with no items is the caller's bug; a correctly written program never does it. Failing to send an SMS, on the other hand, is an expected business outcome: the customer doesn't want SMS or hasn't given a phone number. Replace Error Code with Exception saves a caller who can forget to check the error code from a silent mistake; an exception can't be forgotten. The mechanics: make sure the callers that check the error code have a place to catch the exception, throw the exception where the code was returned, delete the branches that check the code. In the new API, -1 became an ArgumentException.

I didn't turn -2 into an exception. Throwing an exception for something that can happen on any order forces callers to write try/catch for control flow and gets "the SMS wasn't sent" logged like an error. That's what the SmsResult return type is for; the Clean Code series' "Error Handling in Clean Code: Exceptions, the Try Pattern and the Result Pattern" post covers this distinction in detail.

Replace Exception with Precheck works in the other direction: instead of catching an exception for a condition the caller can check in advance, it checks first. In the old code a customer with no phone number was caught with KeyNotFoundException:

// Before
string phone;
try
{
    phone = phones[order.CustomerId];
}
catch (KeyNotFoundException)
{
    return -2;
}

// After
if (smsOptOut.Contains(order.CustomerId) || !phones.TryGetValue(order.CustomerId, out string? phone))
    return -2;

Exceptions are expensive, but the real problem is readability: a catch block says "something can go wrong on this line", while a missing phone number is an ordinary situation. .NET's Try pattern (TryGetValue, TryParse) exists for exactly this refactoring. Where a precheck isn't enough is a race condition: checking that a file exists and then opening it still throws if the file is deleted between the two steps. There, catching the exception is the right answer.

Replace Function with Command and Its Inverse

The function that notifies customers who keep a book on their wishlist when its price drops took eight parameters, and the back office team wanted to see "how many people will get this?" before sending. Replace Function with Command moves the function into its own class: the parameters become fields, the body goes into an Execute method. In a command class, the function's intermediate steps can be split into separate methods, a lifecycle can be defined (prepare first, then run, undo if needed) and several operations can work on the same data. The mechanics: create an empty class, move the function there as Execute, change the old function's callers to new X(...).Execute(), then split the parameters and local variables into fields and methods.

public sealed class PriceDropAlert(Inbox inbox, IReadOnlyDictionary<int, IReadOnlyList<string>> wishlists,
    IReadOnlySet<int> marketingOptOut, string isbn, string title, decimal oldPrice, decimal newPrice)
{
    private decimal DropPercent => (oldPrice - newPrice) / oldPrice * 100;

    private string Text => FormattableString.Invariant($"{title} is now {newPrice:0.00} TRY ({DropPercent:0}% off).");

    // Back office preview: lists the recipients without sending anything.
    public IReadOnlyList<int> Recipients() =>
        DropPercent < 10
            ? []
            : wishlists.Where(w => w.Value.Contains(isbn) && !marketingOptOut.Contains(w.Key))
                .Select(w => w.Key)
                .Order()
                .ToList();

    public int Execute(DateTimeOffset now)
    {
        IReadOnlyList<int> recipients = Recipients();
        foreach (int customerId in recipients)
            inbox.Add(Notification.PriceDrop(customerId, Text, now));
        return recipients.Count;
    }
}

Two commits: first the body moved into Execute as it was, then the preview was split out. The command itself, together with undo and queues, is in the Design Patterns series' "Command Pattern: Turning Requests into Objects, Undo/Redo and Queuing" post.

Replace Command with Function is the inverse, and often the direction you need more. ShippedMessageBuilder, which produced the shipping text, was a stateless class with a single method; every use wrote new ShippedMessageBuilder(order).Build(). What's left of it is a private function:

// Before
public class ShippedMessageBuilder(Order order)
{
    public string Build() =>
        order.Items.Count == 1
            ? $"Your order of {order.Items[0].Title} has shipped."
            : $"Your order of {order.Items.Count} books has shipped.";
}

// After (inside Notifier)
private static string ShippedText(Order order) =>
    order.Items.Count == 1
        ? $"Your order of {order.Items[0].Title} has shipped."
        : $"Your order of {order.Items.Count} books has shipped.";

The test is simple: a command stays if it really uses something a function can't do (preview, undo, queuing, step-by-step setup); if not, it becomes a function.

IDE Support and All the Steps

The IDE column is based on the Microsoft Learn, VS Code and JetBrains docs; I didn't run the IDEs for this post. The compiler lines come from the real output above.

$ git log --oneline
39a5f78 chore: samples/binary-compat (a default parameter breaks old binaries)
b31365e refactor: replace command ShippedMessageBuilder with function ShippedText
fbbaf5d refactor: split PriceDropAlert into Recipients (preview) and Execute
3eff95c refactor: replace function PriceDropAlerts.Send with command PriceDropAlert
3cd9855 refactor: replace error codes in the new API (ArgumentException for -1, SmsResult for -2)
9d940d5 refactor: remove flag arguments (NotifyShipped, NotifyShippedBySms, NotifyShippedBySmsUrgently; SendNotification obsolete)
3ebd834 refactor: replace exception with precheck (TryGetValue for the phone)
8146b64 refactor: remove setting methods (Id and SendAt are set once)
edd292a refactor: replace constructor with factory functions (Shipped, Reminder, PriceDrop)
d59bca6 refactor: return modified value (QuietHoursAdjusted returns the time)
dae92ee refactor: replace parameter isNight with a query inside ApplyQuietHours
3375ebc refactor: parameterize RemindAfterOneDay/ThreeDays into RemindAfter(days)
532c96d refactor: replace query with parameter (DueReminders takes now)
ead604d refactor: separate query from modifier (Unread + MarkAsRead)
72c8117 test: characterize notifications (legacy)

Every step is refactor:. The new API throwing on an empty order may look like a change in behavior, but the new methods had no callers outside the package yet; the behavior of the old method everyone outside uses stayed pinned by the snapshot.

Common Pitfalls

Applying Replace Error Code with Exception to expected business outcomes. "The customer doesn't want SMS", "out of stock", "the coupon has expired" aren't errors but business rules. Reporting them with exceptions forces callers to manage flow with try/catch and hides real errors in the logs. Exceptions are for a program being used wrongly or an outside system behaving unexpectedly.

Adding a parameter with a default value to a public API. It looks source compatible and is binary breaking. If anyone consumes the package, add a new overload or a new method, mark the old one [Obsolete] and keep it until the major version.

Splitting query and modifier and then giving the modifier the wrong thing. Writing MarkAsRead(customerId) looks shorter, but it marks a notification that arrived between the two calls as read without it being seen. The modifier should mark what the query showed.

Turning every function into a command. The price of a command is an extra class and a lifecycle. Don't turn a function into a command just so it "looks more object-oriented"; if there's no real need like preview, undo or queuing, apply the inverse.

Deleting a flag, putting an enum with the same meaning in its place and still keeping a single method. Send(order, Channel.Sms) reads better than a bool, but the branching inside the method stays the same. If the values really do different jobs, use separate methods; if they only change a piece of data, a parameter.

A Small Experiment

★☆☆ Find a method in your codebase that returns a value and also saves, sends or marks something. What does calling it from a debugger watch window or a log line change? Split it into a query and a modifier.

★★☆ Find a public method with a default-valued parameter in one of your team's packages. Set up a small experiment like the one in samples/binary-compat: run an app built against the old version with the new DLL. Then turn on package validation and see whether it catches the same change.

★★★ Pick three methods that read DateTime.Now, DateTimeOffset.UtcNow or a configuration value internally. For each, decide: should it take the value as a parameter, as a dependency like TimeProvider, or stay as it is? Measure your decision by how much easier the test gets.

The API family is done too. One family of the catalog is left, and the series' last post covers it: eleven refactorings that work with inheritance. Pull Up and Push Down, Extract Superclass and Collapse Hierarchy, turning a type code into subclasses and back, and replacing inheritance with delegation.