Refactoring Catalog 4: Move Function/Field, Moving and Sliding Statements, Split Loop, Replace Loop with Pipeline and Remove Dead Code

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, the queries Entity Framework Core 10.0.15 with the SQL Server provider.

Guess First: What Happens to the Query When the Rule Moves into the Entity?

The fourth post of the catalog block covers the refactorings that move something from one place to another: Move Function, Move Field, Move Statements into Function, Move Statements to Callers, Slide Statements, Split Loop, Replace Loop with Pipeline and Remove Dead Code. The names are as on refactoring.com/catalog; the explanations and examples are ours. The scene is the stock report in the Inventory module of Bookstore, the online bookstore used as the shared example across this site's series:

public class StockReport(Dictionary<string, int> reorderLevels)
{
    private const bool UseLegacyHeader = false;
    private int _lastItemCount;

    public string Build(List<StockItem> items)
    {
        StringBuilder sb = new();
        decimal totalValue = 0;
        List<StockItem> reorder = [];
        Dictionary<string, int> perWarehouse = [];
        _lastItemCount = items.Count;
        foreach (StockItem item in items)
        {
            totalValue += item.Quantity * item.UnitCost;
            if (IsBelowReorderLevel(item))
                reorder.Add(item);
            if (!perWarehouse.ContainsKey(item.Warehouse))
                perWarehouse[item.Warehouse] = 0;
            perWarehouse[item.Warehouse] += item.Quantity;
        }

        if (UseLegacyHeader)
            sb.AppendLine("STOCK REPORT");
        sb.AppendLine(FormattableString.Invariant($"Stock value: {totalValue:0.00}"));
        foreach (string warehouse in perWarehouse.Keys.Order())
            sb.AppendLine($"{warehouse}: {perWarehouse[warehouse]} copies");
        sb.AppendLine("Reorder:");
        foreach (StockItem item in reorder)
            sb.AppendLine("! " + FormatLine(item));
        return sb.ToString();
    }

    public string ReorderEmailBody(List<StockItem> items)
    {
        StringBuilder sb = new();
        sb.AppendLine("Please reorder:");
        foreach (StockItem item in items.Where(IsBelowReorderLevel))
            sb.AppendLine("! " + FormatLine(item));
        return sb.ToString();
    }

    private bool IsBelowReorderLevel(StockItem item)
    {
        int level = reorderLevels.TryGetValue(item.Isbn, out int configured) ? configured : 5;
        return item.Quantity < level;
    }

    private static string FormatLine(StockItem item) =>
        $"{item.Isbn} {item.Title} ({item.Warehouse}): {item.Quantity}";

    private static string FormatLineWithCost(StockItem item) =>
        FormattableString.Invariant($"{item.Isbn} {item.Title}: {item.Quantity} x {item.UnitCost:0.00}");
}

Halfway through the post we'll take the reorder rule out of the report and move it into StockItem itself, as a property called NeedsReorder. The stock screen wants to use that rule in the database too: db.StockItems.Where(i => i.NeedsReorder). Does this query work with Entity Framework Core on SQL Server? If not, what does the fix suggested by the error message do?

Move Statements into Function and Slide Statements

The characterization tests are two Verify snapshots: the full text of the report and of the email. The first step is small: both callers do the same thing right before calling FormatLine, adding "! " in front of the line. Move Statements into Function takes a line that always comes before or after every call of a function into the function itself: move the repeated line into the function, delete it from the callers, update the function's name to the new behavior. The result was FormatReorderLine, and "! " lives in one place. Since the repeated line is part of a function call, this is the smallest form of the Duplicated Code smell.

Slide Statements brings related lines next to each other. Four variables are declared at the top of Build, and the StringBuilder is only used after the loop. Sliding a variable's declaration next to its first use helps the reader and makes the next steps, Split Loop and Extract Function, possible; tangled pieces of code can't be split. There is one rule when sliding: if the line you slide shares a side effect with the lines it passes, because they write the same variable, or one calls a method that changes state the other reads, the order is part of the behavior. In Visual Studio, "Move declaration near reference" is the automated form of this for variable declarations.

Split Loop: One Loop, Three Jobs

The single loop computed three separate things: the stock value, the reorder list and the number of copies per warehouse. Split Loop copies the loop and leaves only one job in each copy:

decimal totalValue = 0;
foreach (StockItem item in items)
    totalValue += item.Quantity * item.UnitCost;

List<StockItem> reorder = [];
foreach (StockItem item in items)
{
    if (IsBelowReorderLevel(item))
        reorder.Add(item);
}

Dictionary<string, int> perWarehouse = [];
foreach (StockItem item in items)
{
    if (!perWarehouse.ContainsKey(item.Warehouse))
        perWarehouse[item.Warehouse] = 0;
    perWarehouse[item.Warehouse] += item.Quantity;
}

The code got longer and the list is walked three times; at first glance it looks like a step backwards. The win is that each loop now has a single meaning: you can extract each into its own function or, as in the next step, turn it into a single expression. Before Split Loop, make sure the jobs in the loop body are independent of each other; if one uses an intermediate result of the other, for example an early exit with break or a counter read in the other job's condition, splitting changes behavior.

Replace Loop with Pipeline and Performance

Loops that each do one job are ready to become LINQ. Replace Loop with Pipeline turns the filtering, mapping and aggregation a loop does into a series of operations; it's the answer to the Loops smell from the Clean Code series post "A Code Smell Catalog":

decimal totalValue = items.Sum(i => i.Quantity * i.UnitCost);
List<StockItem> reorder = items.Where(IsBelowReorderLevel).ToList();
Dictionary<string, int> perWarehouse = items
    .GroupBy(i => i.Warehouse)
    .ToDictionary(g => g.Key, g => g.Sum(i => i.Quantity));

The mechanics: open a variable for the collection the loop walks, turn each job in the loop body into an operation one at a time, Where for an if, Select for a mapping, Sum or Aggregate for an accumulation, test after each operation, delete the loop. Visual Studio has a refactoring that turns a foreach loop into LINQ, "Convert to LINQ" and "Convert to Linq (call form)" in the Ctrl+. menu; its docs also note that LINQ is typically less efficient than a foreach loop.

That note matters. The code above walks the list three times, calls delegates, and GroupBy builds an intermediate structure. For a stock report the difference isn't measurable: the report runs a few times a day over a few thousand rows. But on the hot path, a calculation that runs on every HTTP request, millions of times a day, it can be different. Decide by measuring, not guessing; the C# LINQ series post "LINQ Performance: Multiple Enumeration, Materializing and Measuring with BenchmarkDotNet" explains where LINQ's cost matters and how to measure it. I didn't run a benchmark for this post; the report code stayed in the pipeline form for readability.

Move Field and Move Function: Data and Behavior in One Place

IsBelowReorderLevel works only with StockItem's data; the only foreign data is the dictionary of levels by ISBN given to the report's constructor. This is the Feature Envy smell from the Clean Code series: the method is more interested in another class's data than its own. But the data has to move first.

Move Field moves a field to the class that uses it most or changes together with it. The reorder level is a property of a book's stock record, not of the report. The mechanics: open the field on the target class with its default value, 5 for books not in the dictionary, change the places that fill the source so they fill the target too, switch readers to the target, delete the source. In a real application this also means an EF Core migration: a new column on the StockItems table and moving the values from the old configuration into it. So Move Field is a minute in code, and in data it's work carried out with the expand-contract steps from the seventh post.

Once the data has moved, Move Function is natural: copy the method to the target class, leave a thin call forwarding to it in the source, test, switch the callers to the target, delete the one in the source.

public class StockItem
{
    public int Id { get; set; }
    public string Isbn { get; set; } = "";
    public string Title { get; set; } = "";
    public string Warehouse { get; set; } = "";
    public int Quantity { get; set; }
    public decimal UnitCost { get; set; }
    public int ReorderLevel { get; set; } = 5;

    public bool NeedsReorder => Quantity < ReorderLevel;
}

The report now says items.Where(i => i.NeedsReorder), and the constructor parameter is gone. The snapshots didn't change. Next is the stock screen's query.

The Answer: First an Exception, Then the Whole Table

To try the query without connecting to a database I used EF Core's ToQueryString() method; it generates the SQL but doesn't open a connection. The result:

System.InvalidOperationException : The LINQ expression 'DbSet<StockItem>()
    .Where(s => s.NeedsReorder)' could not be translated. Additional information: Translation of member 'NeedsReorder' on entity type 'StockItem' failed. This commonly occurs when the specified member is unmapped. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to 'AsEnumerable', 'AsAsyncEnumerable', 'ToList', or 'ToListAsync'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

EF Core sees the query not as C# but as an expression tree and translates it to SQL. The body of NeedsReorder is a compiled getter; EF Core can't look inside it, and it can't translate a property that isn't mapped to a column. Move Function was a pure refactoring on the C# side; on the query side it changed behavior.

The second path the error message suggests is moving the filtering to the client with AsEnumerable() or ToList(). That works, but you have to look at how. The SQL generated by the part before AsEnumerable() has no filter; the whole table comes to the client:

SELECT [s].[Id], [s].[Isbn], [s].[Quantity], [s].[ReorderLevel], [s].[Title], [s].[UnitCost], [s].[Warehouse]
FROM [StockItems] AS [s]

When the rule is written inside the query as a lambda, the filter goes down into SQL:

SELECT [s].[Id], [s].[Isbn], [s].[Quantity], [s].[ReorderLevel], [s].[Title], [s].[UnitCost], [s].[Warehouse]
FROM [StockItems] AS [s]
WHERE [s].[Quantity] < [s].[ReorderLevel]

The first goes unnoticed on a hundred-row table in development; on a production stock table with hundreds of thousands of rows it moves the whole table to the application server every time the screen opens. The answer is neither: define the rule once as an expression EF Core can read, and use it both in the query and in the property.

public static readonly Expression<Func<StockItem, bool>> NeedsReorderExpression =
    i => i.Quantity < i.ReorderLevel;

private static readonly Func<StockItem, bool> NeedsReorderRule = NeedsReorderExpression.Compile();

public bool NeedsReorder => NeedsReorderRule(this);

// Query side
string sql = db.StockItems.Where(StockItem.NeedsReorderExpression).ToQueryString();

The tests pin three things: the query with the expression produces WHERE [s].[Quantity] < [s].[ReorderLevel], the query with the property is rejected with "could not be translated", and the property and the expression give the same answer at the boundary, when the quantity equals the level. I generated the SQL with ToQueryString(); I didn't run the query against a real SQL Server. The same trap applies to Order.Total from the eleventh post: before moving a property that ends up in queries into the entity, search for whether it is used in a query.

Move Statements to Callers: When a New Caller Arrives

The warehouse team asked for a pick list: the books in one warehouse, with the same line format but without the "!" mark. FormatReorderLine does the wrong thing for this caller. Move Statements to Callers is the inverse of Move Statements into Function: if the callers no longer want the same thing, distribute the line at the end or start of the function back to the callers. Adding a bool addMark parameter to the function looks shorter, but it would be the kind of flag parameter we split in the fourth post.

The two-hats rule from the first post applies here too. First a refactor: commit that doesn't change behavior: "! " went back to the two callers, and the function is called FormatLine again. Then a separate feat: commit added the new feature:

public string PickList(IReadOnlyList<StockItem> items, string warehouse)
{
    StringBuilder sb = new();
    sb.AppendLine($"Pick list for {warehouse}:");
    foreach (StockItem item in items.Where(i => i.Warehouse == warehouse))
        sb.AppendLine(FormatLine(item));
    return sb.ToString();
}

This small preparation before adding the feature is the everyday form of Kent Beck's often-quoted idea: first make the desired change easy, then make the easy change.

Remove Dead Code: Analyzers, Coverage and Reflection

The report has three dead pieces: FormatLineWithCost, never called; _lastItemCount, written but never read; and the old header guarded by a false constant. By default the compiler reports only the last one, CS0162. The first two have IDE0051 and IDE0052, but these are code style rules, and they need two settings to run in dotnet build: a severity in .editorconfig and EnforceCodeStyleInBuild. With both turned on (lines shortened):

$ dotnet build --no-incremental -p:EnforceCodeStyleInBuild=true
src/Bookstore.Inventory/StockReport.cs(22,13): warning CS0162: Unreachable code detected
src/Bookstore.Inventory/StockReport.cs(53,27): warning IDE0051: Private member 'StockReport.FormatLineWithCost' is unused
src/Bookstore.Inventory/StockReport.cs(8,17): warning IDE0052: Private member 'StockReport._lastItemCount' can be removed as the value assigned to it is never read

I later moved the setting into Directory.Build.props at the repo root, so every build runs these rules. I tried the fix from the command line too. dotnet format style --diagnostics IDE0051 IDE0052 applied Roslyn's code fix for IDE0051 and deleted the method; for IDE0052 it said this:

$ dotnet format style src/Bookstore.Inventory --diagnostics IDE0051 IDE0052
Unable to fix IDE0052. No associated code fix found.

That makes sense: deleting an unread field means deleting the line that writes it too, and that line may have a side effect. I deleted the field, the assignment and the branch guarded by the constant by hand. The mechanics of Remove Dead Code are short: verify the code really isn't called, delete it, test. The deleted code stays in git history; there's no need to comment it out "in case we need it".

The hard part is the verification step. IDE0051 only knows private members. Saying a public method or class is "unused" means knowing that nobody calls it through reflection, convention-based DI registration, a Razor view, a serializer or another repo. In the third post we saw a class whose name didn't end in "Rule" silently drop out of the DI container; deleting the same class because it "has no references" has the same result. The coverage report from the second post helps here: lines that never run are candidates, but not certain ones, because not every piece of code the tests miss is dead. Logs or metrics are more reliable for seeing calls in production. Feature flag leftovers are typical dead code too: once a flag is on everywhere, the off branch, like UseLegacyHeader, lives on for years without anyone reading it.

IDE Support and All the Steps

The IDE column is based on the Microsoft Learn and JetBrains docs; I didn't run the IDEs for this post. The analyzer and dotnet format entries come from the real output above.

$ git log --oneline
a611871 refactor: StockReport accepts IReadOnlyList (CA1002)
371be39 refactor: remove dead code (IDE0051, IDE0052, CS0162)
acb84f8 feat: warehouse pick list
1b4f2c6 refactor: move the '! ' prefix back to the callers
8a63ec4 refactor: reorder rule as an expression EF Core can translate
ff584ac refactor: move IsBelowReorderLevel to StockItem.NeedsReorder
333a09b refactor: move reorder level from StockReport to StockItem
187644b refactor: replace the three loops with LINQ pipelines
65d00ad refactor: split the stock loop into three loops
e4ae4f4 refactor: slide statements in Build (StringBuilder next to the output)
3ec9a3e refactor: move the '! ' prefix into FormatReorderLine
3e83e1a test: characterize StockReport (legacy)

The last commit widens the parameter of Build and ReorderEmailBody from List to IReadOnlyList in response to the CA1002 warning; a small repeat of the Change Function Declaration from the tenth post.

Common Pitfalls

The query sliding to the client in Move Function. When you move a condition into an entity property or method, EF Core can't translate it to SQL; the AsEnumerable() the error message suggests often pulls the whole table into memory. If the rule is also used in queries, define it as an Expression and verify the SQL in a test with ToQueryString().

Deleting code that looks dead. Public code called through reflection, a DI convention, a serializer, a Razor view or another repo never shows up as "unused" in any analyzer. Before deleting, search for the name as a string and look at production logs.

Applying Replace Loop with Pipeline everywhere. The readability win is real for most loops, but converting a frequently called calculation without measuring means taking a performance risk blindly. Don't do the opposite either: leaving everything in loops because "LINQ is slow" without measuring.

Missing a side effect in Slide Statements. The order of two lines is part of the behavior if they change the same state. Sliding a log line or a counter increment upwards can change the output.

Doing Move Field only in code. If the moved field is a column, the migration, the data move and the old readers are part of the job; the expand-contract from the seventh post applies here too.

A Small Experiment

★☆☆ Add IDE0051 and IDE0052 as warning to .editorconfig in your own project, turn on EnforceCodeStyleInBuild and build. How many dead members showed up? How many of them could dotnet format style --diagnostics IDE0051 delete automatically?

★★☆ Find a loop that does more than one job. First apply Slide Statements and Split Loop, then turn each loop into LINQ. Run the tests after each step. At which step did something break?

★★★ Look for a place where a computed property on an entity is used in an EF Core query. Look at the SQL it produces with ToQueryString(). Is the filter in the SQL, or does it slide to the client somewhere in the query through AsEnumerable() or ToList()?

The moving family is done too: data, behavior and lines are in the right place. Three families remain in the catalog. The next post continues with the refactorings that simplify conditional logic: Decompose Conditional, Consolidate Conditional Expression, Replace Nested Conditional with Guard Clauses, Replace Conditional with Polymorphism and Introduce Special Case.