Lesson 31. Reviewing a C# Codebase

Mission link: The mission ends here: given C# written with a habit that merely compiles, name the habit and rewrite it idiomatically. Every earlier lesson collected one of those habits and, more importantly, the cost that makes it a habit rather than a preference. This lesson is the index, and the argument for reviewing in the order that puts the invisible failures first.
Primary source: Docs: "Common C# code conventions", Microsoft Learn
Prerequisites: Lesson 30, Lesson 16, Lesson 1

Warm-up

1. ▢ Name one thing a C# compiler will happily accept that this arc has argued you should not write.

2. ▢ What was wrong with mocking a class whose members are not virtual?

3. ▢ A reviewer writes "this should be a record". Is that a review comment?

Know this

A review comment is worth what its cost statement is worth. "Make this a record" is an opinion, and the author is entitled to disagree with it. "This type has value semantics in every other respect, so as a class two equal orders compare unequal, and the dictionary lookup two files away silently misses" is a cost, and there is nothing to disagree with. The whole arc has been assembling the second kind of sentence, which is why it spent so long on what each construct actually does at run time.

Three questions turn a reaction into a comment:

• What does this construct promise?
• Who pays for the promise, and in what currency? A signature, a lock, a round trip, a lifetime.
• When would you find out it was broken? This is the one that ranks the finding.

Give the mechanical half to the tools, and mean it. Conventions exist for readability, consistency and collaboration, and the documentation is clear that they are enforceable rather than negotiable in review: enable code analysis to enforce the rules you choose, and add an .editorconfig so the editor applies them, after which code analysis produces warnings and diagnostics when it detects rule violations and each CI build notifies developers when they violate any of the rules (Common C# code conventions).

A human review spent on brace placement is a review not spent on a captive dependency. Configure the first, then never discuss it again.

Some guidance is still a judgment, and the conventions say so in the language of costs. Three from the same page are worth carrying into a review, because each names a consequence rather than a taste:

• Only catch exceptions that can be properly handled, and avoid catching general exceptions, with the documentation's own rule that sample code should not catch System.Exception without an exception filter. Use specific exception types.
• Use asynchronous programming with async and await for I/O-bound operations, and be cautious of deadlocks.
• Use implicit typing when the type is obvious from the right side of the assignment, and do not assume the type is clear from a method name. A type counts as clear when the right side is a new operator, an explicit cast, or a literal.

That third one is a reading rule disguised as a writing rule. var total = Compute(); tells a reviewer nothing, and a reviewer who cannot see the type cannot see the cost.

The review pass, ordered by what the compiler will never tell you. This is the arc as a checklist. Read the third column first, because it is what decides which comment to write when you only have the attention for one:

Notice the third column. Almost none of these announce themselves. That is the argument for reviewing in this order rather than top to bottom through the file: the findings worth your attention are the ones no other mechanism will report.

Name the habit, then write the replacement. The mission asks for both halves, and the second is not politeness. A comment that names a cost and stops leaves the author holding a verdict with no move, and the most common result is a change that relocates the problem. "Take a CancellationToken and pass it to the calls inside" is a review. "This is not cancellable" is a complaint.

Where the arc leaves you, honestly. You can model a domain and defend the choice from what the CLR does; read and write async code and say what happens to a method at each await; query with LINQ and know where the query stops being SQL; ship a service whose lifetimes, configuration and data access agree with each other; and take an unfamiliar C# file apart by asking what each construct promises and who pays.

What this arc did not cover is worth saying too, because knowing the edge of your own map is part of the same skill: Unity and game development, CLR internals past what explains value semantics and async, parallel LINQ, and the security and operational concerns of running the service you can now build. Each of those is a topic, not a footnote, and treating them as gaps you know about is more useful than the feeling of having finished.

Practice

1. ▢ Two reviewers comment on the same line, which reads catch (Exception) { }. One writes "don't swallow exceptions". The other writes something better. What?

2. ▢ Sort these into "an analyzer should catch this" and "only a person will catch this": brace placement, a scoped service injected into a singleton, System.String written instead of string, and a repository interface whose method cannot be cancelled.

3. ▢ A file is full of var. Which of these declarations does the guidance allow, and why does it matter to a reviewer rather than to the compiler?

   • var message = "This is clearly a string.";
   • var count = ExampleClass.ResultSoFar();
   • var order = new Order();

4. ▢ Which statement is correct?

   • a) A good review finds every deviation from the team's conventions, since consistency is what conventions are for
   • b) The findings worth a reviewer's attention are the ones no other mechanism reports, because most of them fail silently
   • c) Naming the cost is enough; proposing the rewrite is the author's job
   • d) A construct that compiles and passes its tests has been reviewed by better tools than a person

5. ▢ The last exercise. Review this. Name at least four habits, and for each say what it costs and what you would write instead.

public class OrderService
{
    private readonly ApplicationDbContext _db;

    public OrderService(ApplicationDbContext db) => _db = db;

    public async void Recalculate(IEnumerable<int> orderIds, decimal multiplier)
    {
        var tasks = orderIds.Select(id => _db.Orders.FindAsync(id).AsTask());
        var orders = await Task.WhenAll(tasks);

        foreach (var order in orders)
        {
            try
            {
                order.Total = order.Subtotal * multiplier;
            }
            catch (Exception)
            {
            }
        }

        await _db.SaveChangesAsync();
    }
}

Real-world reps

• [ ] Take a C# file you did not write and run this lesson's table down it. Note which findings you could state as a cost, and which are still preferences.
• [ ] Check whether the repository has an .editorconfig and code analysis enabled. If not, that is the first review comment, and it retires a whole category of them.
• [ ] Tomorrow: write one review comment that names a cost and proposes the rewrite, and notice how differently it is answered.

Going further

• Docs: "Common C# code conventions", Microsoft Learn
• Docs: "Handling and throwing exceptions in .NET", Microsoft Learn
• Docs: "async keyword", Microsoft Learn
• Resources

Not landing? Reread the primary source at the top, since this lesson compresses it and compression is where understanding leaks. Check the glossary for any term that felt slippery.

If the lesson itself is unclear rather than the material, that is a defect: open an issue.