Refactoring Catalog 5: Simplifying Conditional Logic — Decompose/Consolidate Conditional, Guard Clauses, Replace Conditional with Polymorphism and Introduce Special Case

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: What Do the Tests Say When an Object Replaces null for Guests?

The fifth post of the catalog block covers seven refactorings that simplify conditional logic: Decompose Conditional, Consolidate Conditional Expression, Replace Nested Conditional with Guard Clauses, Replace Control Flag with Break, Replace Conditional with Polymorphism, Introduce Special Case and Introduce Assertion. The names are as on refactoring.com/catalog; the explanations and examples are ours. The scene is CheckoutPolicy in the Ordering module of Bookstore, the online bookstore used as the shared example across this site's series. A cart's owner can be a guest who hasn't signed in, and the code expresses that with null:

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

public string Greeting(Cart cart)
{
    if (cart.Customer == null)
        return "Welcome, guest!";
    return $"Welcome back, {cart.Customer.Name}!";
}

public int AwardLoyaltyPoints(Cart cart)
{
    int points = (int)(cart.Subtotal / 10);
    if (cart.Customer != null)
        cart.Customer.LoyaltyPoints += points;
    return points;
}

public string LoyaltyBadge(Cart cart)
{
    if (cart.Customer == null)
        return "0 points";
    return $"{cart.Customer.LoyaltyPoints} points";
}

The same null check appears in Discount and CheckoutError too. Halfway through the post we'll remove these checks with a special case object: a guest is no longer null but a Customer created by Customer.CreateGuest() with 0 points. The if in the method that awards points looks unnecessary too, since every cart now has a customer:

public bool IsGuest { get; private init; }

public static Customer CreateGuest() => new() { Name = "guest", IsGuest = true };

// CheckoutPolicy
public int AwardLoyaltyPoints(Cart cart)
{
    int points = (int)(cart.Subtotal / 10);
    cart.Customer.LoyaltyPoints += points;
    return points;
}

public string LoyaltyBadge(Cart cart) => $"{cart.Customer.LoyaltyPoints} points";

The safety net is a Verify snapshot holding the output of every method for each cart type, each payment method and two dates (36 combinations), plus two tests for the method that awards points. Do the tests stay green after this change? The order confirmation page calls AwardLoyaltyPoints first and then LoyaltyBadge. What does a guest who checks out with a 120 TRY cart see on that page?

Decompose Conditional and Consolidate Conditional Expression

First the discount calculation. Before the characterization test it looked like this:

public decimal Discount(Cart cart, DateOnly today)
{
    if (cart.Customer == null)
        return 0m;
    if (cart.Lines.Count == 0)
        return 0m;
    if (cart.Payment == PaymentMethod.CashOnDelivery)
        return 0m;

    if (today.Month == 11 && today.Day >= 24 && today.Day <= 30 && cart.Subtotal >= 250m)
        return Math.Round(cart.Subtotal * 0.15m, 2, MidpointRounding.AwayFromZero);
    else
        return Math.Round(cart.Subtotal * 0.05m, 2, MidpointRounding.AwayFromZero);
}

The second if's condition says how it works, not what it means; the reader has to work out that "24 to 30 November" is the campaign week. Decompose Conditional extracts the condition and each of the two branches into named functions. It's really Extract Function from the tenth post applied to conditionals, and it answers the Long Function smell from the Clean Code series' "Code Smell Catalog" post. The mechanics: extract the condition into a function and test; extract the "then" branch and test; extract the "else" branch and test.

The three ifs at the top are a different problem: all three give the same answer and really ask a single question: "is this cart eligible for a discount?" Consolidate Conditional Expression merges checks with the same result into one condition and then extracts that condition into a named function. Before merging, make sure the checks have no side effects; if one writes a log line or increments a counter, merging changes when that side effect runs. In C#, property patterns read better than a chain of || for the merge:

public decimal Discount(Cart cart, DateOnly today)
{
    if (!IsEligibleForDiscount(cart))
        return 0m;

    return QualifiesForCampaign(cart, today) ? CampaignDiscount(cart) : RegularDiscount(cart);
}

private static bool IsEligibleForDiscount(Cart cart) =>
    cart is not ({ Customer: null } or { Lines.Count: 0 } or { Payment: PaymentMethod.CashOnDelivery });

private static bool QualifiesForCampaign(Cart cart, DateOnly today) =>
    IsCampaignWeek(today) && cart.Subtotal >= CampaignMinimum;

private static bool IsCampaignWeek(DateOnly today) =>
    today is { Month: 11, Day: >= 24 and <= 30 };

private static decimal CampaignDiscount(Cart cart) => Round(cart.Subtotal * 0.15m);

private static decimal RegularDiscount(Cart cart) => Round(cart.Subtotal * 0.05m);

Decompose and consolidate are two separate commits, and the snapshot didn't change after either. Lines.Count: 0 is an extended property pattern; it lets you reach nested properties with a dot. The campaign week now lives in one place under its own name, so it's obvious which line to change when the dates change.

Replace Nested Conditional with Guard Clauses and Replace Control Flag with Break

The method that says whether a cart can go to payment is nested three levels deep and collects its result in a variable:

public string? CheckoutError(Cart cart)
{
    string? error;
    if (cart.Lines.Count > 0)
    {
        if (cart.Customer != null || cart.Payment != PaymentMethod.BankTransfer)
        {
            bool tooMany = false;
            foreach (CartLine line in cart.Lines)
            {
                if (!tooMany && line.Quantity > OrderSettings.MaxQuantityPerLine)
                    tooMany = true;
            }
            if (tooMany)
                error = $"At most {OrderSettings.MaxQuantityPerLine} copies per line.";
            else
                error = null;
        }
        else
        {
            error = "Guests cannot pay by bank transfer.";
        }
    }
    else
    {
        error = "The cart is empty.";
    }
    return error;
}

Nested conditionals suggest that every branch matters equally. Not here: an empty cart and a guest paying by bank transfer are exceptional cases, and the real work is checking the lines. Replace Nested Conditional with Guard Clauses moves the exceptional case to the top and returns immediately. The mechanics: turn the outermost condition into a guard clause (invert the condition, return the branch's result right away), test, move on to the next level. Watch De Morgan's law when you invert: the inverse of cart.Customer != null || cart.Payment != PaymentMethod.BankTransfer is cart.Customer == null && cart.Payment == PaymentMethod.BankTransfer. The "Invert if" refactoring in Visual Studio and VS Code takes away the risk of doing that inversion by hand. What's new compared with the fourth post is the mechanics; we already saw the idea of a guard clause there in the coupon code.

After the guard clauses, tooMany is what's left. It's a control flag: a variable that tells the loop when it should be done. The loop keeps walking the whole list after it finds the first long line, and the !tooMany check repeats on every iteration. Replace Control Flag with Break puts a break, continue or return where the flag changes and deletes the flag. Since what we're looking for here is already the method's result, return is the shortest:

public string? CheckoutError(Cart cart)
{
    if (cart.Lines.Count == 0)
        return "The cart is empty.";
    if (cart.Customer == null && cart.Payment == PaymentMethod.BankTransfer)
        return "Guests cannot pay by bank transfer.";

    foreach (CartLine line in cart.Lines)
    {
        if (line.Quantity > OrderSettings.MaxQuantityPerLine)
            return $"At most {OrderSettings.MaxQuantityPerLine} copies per line.";
    }
    return null;
}

What's left of a search loop can usually become LINQ; the next commit turned the loop into cart.Lines.Any(l => l.Quantity > OrderSettings.MaxQuantityPerLine). That's Replace Loop with Pipeline from the thirteenth post. Three commits, a green snapshot three times.

Replace Conditional with Polymorphism: Switch Expression First

Three methods decided based on the payment method: the fee, the name shown on screen and the confirmation note. All three were classic switch statements with break over the same PaymentMethod enum. Before going to polymorphism I looked at C#'s own tool: the IDE0066 rule reports switch statements that can become a switch expression. I set the rule to warning in .editorconfig, built, and then applied the fix from the command line:

$ dotnet build --no-incremental src/Bookstore.Ordering
src/Bookstore.Ordering/CheckoutPolicy.cs(79,9): warning IDE0066: Use 'switch' expression
src/Bookstore.Ordering/CheckoutPolicy.cs(98,9): warning IDE0066: Use 'switch' expression
src/Bookstore.Ordering/CheckoutPolicy.cs(113,9): warning IDE0066: Use 'switch' expression

$ dotnet format style src/Bookstore.Ordering --diagnostics IDE0066 -v n
Formatting code files in workspace 'src/Bookstore.Ordering/Bookstore.Ordering.csproj'.
Formatted code file 'src/Bookstore.Ordering/CheckoutPolicy.cs'.

The tool converted all three switches. The result for the fee method:

public decimal PaymentFee(Cart cart)
{
    var fee = cart.Payment switch
    {
        PaymentMethod.Card => 0m,
        PaymentMethod.BankTransfer => 0m,
        PaymentMethod.CashOnDelivery => cart.Subtotal >= 1000m ? 0m : 15m,
        _ => throw new ArgumentOutOfRangeException(nameof(cart), cart.Payment, "Unknown payment method."),
    };
    return fee;
}

The behavior is the same, but the tool left a var that breaks the team's rule and kept the temporary variable. In a separate commit I made the methods expression-bodied and merged the two arms with the same result into PaymentMethod.Card or PaymentMethod.BankTransfer. The plan describes this kind of simplification as "without going to polymorphism", and for most code this is the place to stop.

So when do you take one more step? We have three switches over the same enum; in the Clean Code series' terms, the Repeated Switches smell. When a new payment method arrives (say, card payment at the door), three places will have to change at once. The fourth post's "new type or new operation" table applies here too; I won't repeat it. In Bookstore, a new payment method is more likely than a new question asked of every payment method. The mechanics of Replace Conditional with Polymorphism: create a class for each type (if there isn't one) and a single factory that returns the right instance; move the method with the switch to the common type; copy each arm into the override or implementation in the matching class and test; when the switch holds nothing but the factory, delete the arms.

public interface IPaymentOption
{
    string Label { get; }
    decimal Fee(decimal subtotal);
    string ConfirmationNote(decimal subtotal);
}

public sealed class CashOnDeliveryPayment : IPaymentOption
{
    private const decimal FreeAbove = 1000m;
    private const decimal CourierFee = 15m;

    public string Label => "Cash on delivery";
    public decimal Fee(decimal subtotal) => subtotal >= FreeAbove ? 0m : CourierFee;
    public string ConfirmationNote(decimal subtotal) =>
        FormattableString.Invariant($"Please have {subtotal + Fee(subtotal):0.00} TRY ready for the courier.");
}

public static class PaymentOptions
{
    public static IPaymentOption For(PaymentMethod method) => method switch
    {
        PaymentMethod.Card => new CardPayment(),
        PaymentMethod.BankTransfer => new BankTransferPayment(),
        PaymentMethod.CashOnDelivery => new CashOnDeliveryPayment(),
        _ => throw new ArgumentOutOfRangeException(nameof(method), method, "Unknown payment method."),
    };
}

// CheckoutPolicy
public decimal PaymentFee(Cart cart) => PaymentOptions.For(cart.Payment).Fee(cart.Subtotal);

CardPayment and BankTransferPayment look the same. I chose an interface as the common type; a base class would also work, but there's no shared behavior, only a shared question. The enum is still there, because it's the value stored with the cart and received from the API; and the switch didn't disappear, it moved down to the factory at the system's boundary. Putting subclasses in place of the enum is a separate refactoring, Replace Type Code with Subclasses; we'll cover it with the inheritance family in the series' last post. The structures themselves are in the Design Patterns series' "Strategy Pattern in Depth" and "State Pattern" posts; here we only have the steps that lead there.

Introduce Special Case: A Guest Is a Customer

The cart.Customer == null check was repeated in four methods, and each gave the same kind of answer for a guest: a greeting, zero points, no discount, no bank transfer. Introduce Special Case collects these scattered answers to the same special value into an object that represents that value. When it's used for null, its more common name is Null Object; the Design Patterns series' "Code-Level Patterns Beyond GoF: Null Object, Specification, Result and Pipeline" post covers the pattern itself. The mechanics: add a property to the real type that asks "is this the special case?" (IsGuest); write a function that creates the special case object (CreateGuest); change the places that produce null to call that function; then either remove each null check (if the object's own value is enough) or turn it into IsGuest.

C# helps here through nullable reference types. When I changed Customer? in Cart to Customer, the compiler listed every place passing null:

tests/Bookstore.Ordering.Tests/CheckoutPolicyTests.cs(13,36): warning CS8625: Cannot convert null literal to non-nullable reference type.
tests/Bookstore.Ordering.Tests/CheckoutPolicyTests.cs(14,35): warning CS8625: Cannot convert null literal to non-nullable reference type.

Both were in the tests; if production code had created a cart, it would have shown up in the list too. In the greeting and the bank transfer check a guest really does behave differently; there the null check became IsGuest, and the pattern in IsEligibleForDiscount became { Customer.IsGuest: true }. The checks in the badge and points methods I removed as shown above, because a guest's points are already 0. The snapshot is green, the two tests are green; all 36 tests passed and the commit went in.

The Answer: The Tests Are Green, the Guest Sees 12 Points

The tests stayed green, but a guest sees "12 points" on the confirmation page. In the old code the null check did two jobs at once: show a guest 0 instead of points, and not write points for a guest. The second job was invisible, because you can't write anything to null. A special case object, on the other hand, is a writable object; once the check was gone, the points were written to it. To notice, I had to write a test that does what the confirmation page does:

failed Bookstore.Ordering.Tests.CheckoutPolicyTests.Guest_badge_stays_at_zero_after_checkout (0ms)
  Assert.Equal() Failure: Strings differ
             ↓ (pos 0)
  Expected: "0 points"
  Actual:   "12 points"
             ↑ (pos 0)

The characterization tests pinned each method on its own, not two methods called one after the other. The fix is to move the rule into the special case itself: nobody outside can write the points any more; the customer earns them.

private int _loyaltyPoints;

public int LoyaltyPoints { get => _loyaltyPoints; init => _loyaltyPoints = value; }

public void EarnPoints(int points)
{
    // Guests see how many points they would earn but never collect them.
    if (IsGuest)
        return;
    _loyaltyPoints += points;
}

This commit starts with fix: because it changes behavior: it restores the behavior the refactoring commit broke. Removing the setter is a repeat of Encapsulate Variable from the eleventh post, and that's what really protects us. One more note: CreateGuest returns a new object on every call. A special case is often written as a single static instance; had it been written that way while the setter was still there, the 12 points would have gone onto the object all guests share and shown up for the next guest too. If you use a single instance, make it immutable.

Introduce Assertion: Debug.Assert Is Not Validation

Some conditions are the code's own assumptions: they must always hold for a correctly written program. A discount must never be negative and must never exceed the cart total. Introduce Assertion writes that assumption into the code; the reader doesn't have to look for a comment, and if the assumption breaks, it's noticed immediately during development:

decimal discount = QualifiesForCampaign(cart, today) ? CampaignDiscount(cart) : RegularDiscount(cart);
Debug.Assert(discount >= 0 && discount <= cart.Subtotal, "A discount is never negative and never exceeds the subtotal.");
return discount;

An important detail: Debug.Assert is marked with [Conditional("DEBUG")]. In a Release build the call isn't compiled, so the condition is never evaluated. I tried a small file-based app with a negatively priced cart:

$ dotnet run assert.cs
Process terminated.
Assertion failed.
A discount is never negative and never exceeds the subtotal.
   at Bookstore.Ordering.CheckoutPolicy.Discount(Cart cart, DateOnly today) in src/Bookstore.Ordering/CheckoutPolicy.cs:line 39
   at Program.<Main>$(String[] args) in samples/assert.cs:line 9

$ dotnet run -c Release assert.cs
Discount: -6.00

In Debug the process terminated; in Release a negative discount was calculated silently. In tests, xUnit.net v3 turns a failed assertion into a test failure ("Trace/Debug.Assert() Failure") without taking down the process. The real problem here isn't the assertion: a negative price isn't an assumption of our code but input from outside. Input is validated at the system's boundary, with a check that runs in every build. I added that to CartLine as a separate fix: commit:

public sealed record CartLine(string Isbn, string Title, decimal UnitPrice, int Quantity, bool IsEbook)
{
    public decimal UnitPrice { get; } = UnitPrice >= 0
        ? UnitPrice
        : throw new ArgumentOutOfRangeException(nameof(UnitPrice), UnitPrice, "A price cannot be negative.");
}

Now the same sample says "A price cannot be negative." with an ArgumentOutOfRangeException in both builds, and the assertion only guards the case that really could be our own bug. The rule is short: assertions to catch broken code, guards and exceptions to catch bad data from a user or another system. The fail fast section of the SOLID and Design Principles series' "Lesser-Known Principles" post is a longer version of this distinction.

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 IDE0066 and dotnet format line comes from the real output above.

$ git log --oneline
fc506d6 chore: samples/assert.cs (Debug vs Release assertion)
11970a0 fix: CartLine rejects negative prices
b6e725d refactor: introduce assertion (discount within 0..subtotal)
988435d fix: guests no longer collect loyalty points (Customer.EarnPoints)
97fbd79 refactor: introduce special case Customer.CreateGuest instead of null
abf022b refactor: replace the payment switches with IPaymentOption classes
98f0ed7 refactor: expression-bodied switches, explicit types
9107b59 refactor: switch statements to switch expressions (dotnet format, IDE0066)
d398dba refactor: replace the search loop with Any
8addeb3 refactor: replace control flag tooMany with return
5ce072e refactor: replace nested conditional with guard clauses in CheckoutError
00bf99c refactor: consolidate the no-discount checks into IsEligibleForDiscount
789f5c1 refactor: decompose the campaign condition in Discount
9ca78a1 test: characterize CheckoutPolicy (legacy)

Most of the catalog's refactorings without an inverse are in this family. Extracting a condition into a named function has an inverse (Inline Function), but there's hardly a situation that calls for turning guard clauses back into nested ifs.

Common Pitfalls

A special case silently giving wrong data. null can't do anything; a special case object can do everything: collect points, appear in a report as a customer called "guest", be counted as a record with Id 0. When you remove a null check, find the side effect the check was guarding too; move the rule into the object itself and make it immutable if you can.

Using assertions for input validation. Debug.Assert doesn't exist in a Release build; checking a value from a user, a database or another service with it means not checking it at all in production. Use guards and exceptions at the boundary.

Inverting the condition wrongly when turning it into a guard clause. Inverting a condition that mixes || and && by hand is a classic source of bugs. Use the IDE's "Invert if" refactoring, or extract the condition into a named function first and then just add a !.

Turning every switch into polymorphism. A short switch expression that lives in one place is often the most readable form. The price of polymorphism is extra classes and code spread across several files; if the same switch doesn't repeat in more than one place, don't pay it.

Merging side effects while consolidating. If one of the separate ifs writes a log line or increments a metric, putting them into a single || chain changes which checks run; because of short-circuiting, some never run at all.

A Small Experiment

★☆☆ Add dotnet_diagnostic.IDE0066.severity = warning to .editorconfig in your own project and build. How many switch statements can become switch expressions? Look at the code dotnet format style --diagnostics IDE0066 produced: did it leave anything that breaks your team's rules?

★★☆ Find the place in your codebase where a null check for the same object repeats most. Before designing the special case object, look at whether each check does two things: does it return a value, or does it prevent a side effect?

★★★ Search your project for Debug.Assert calls. For each one ask: can this condition break without a bug in our own code? If it can, turn it into a guard at the boundary and verify it with a test in a Release build.

The conditionals are simpler too. Two families of the catalog are left. The next post continues with twelve refactorings that reshape the API a class exposes: Separate Query from Modifier, Remove Flag Argument, Parameterize Function, the transformations around factory functions and commands, and the two-way move between error codes and exceptions.