The Problem: Your 900-Line Controller
You open the order controller and scroll. And scroll. Somewhere around line 450 you hit the cancellation action. It queries the database, checks permissions, validates business rules, sends an email, logs to a third-party service, updates related records, and transforms the response. That's one HTTP endpoint. Now multiply that across 20 endpoints and you have a maintenance nightmare. Refactoring a fat controller: a .NET guide for developers starts with admitting the controller has become a dumping ground, and understanding why that happened in the first place.
This article walks through extracting logic from a bloated controller, moving responsibility where it belongs, and keeping your codebase testable and sane as it grows.
Spot the Smells: What a Fat Controller Looks Like
A fat controller typically contains:
- Direct DbContext queries and EF Core save calls
- Business rule validation scattered throughout the action
- Email sending, logging, or external service calls
- Data transformation and DTO mapping
- Transaction management and rollback logic
Here's what we're working with. This isn't exaggerated—we've seen worse in production:
[ApiController]
[Route("api/orders")]
public class OrdersController : ControllerBase
{
private readonly OrderDbContext _db;
private readonly IEmailService _emailService;
private readonly ILogger<OrdersController> _logger;
public OrdersController(OrderDbContext db, IEmailService emailService, ILogger<OrdersController> logger)
{
_db = db;
_emailService = emailService;
_logger = logger;
}
[HttpPost("{id}/cancel")]
public async Task<IActionResult> CancelOrder(int id)
{
try
{
var order = await _db.Orders
.Include(o => o.Items)
.Include(o => o.Customer)
.FirstOrDefaultAsync(o => o.Id == id);
if (order == null)
return NotFound();
// Business rule: can only cancel if status is Pending or Processing
if (order.Status != OrderStatus.Pending && order.Status != OrderStatus.Processing)
return BadRequest("Order cannot be cancelled in current state");
// Business rule: cannot cancel orders over a certain age
if ((DateTime.UtcNow - order.CreatedAt).TotalDays > 30)
return BadRequest("Orders older than 30 days cannot be cancelled");
// Business rule: check for shipped items
var shippedItems = order.Items.Where(i => i.Status == ItemStatus.Shipped).ToList();
if (shippedItems.Any())
return BadRequest("Cannot cancel orders with shipped items");
// Update order state
order.Status = OrderStatus.Cancelled;
order.CancelledAt = DateTime.UtcNow;
order.CancellationReason = "Customer requested";
// Refund logic
order.RefundAmount = order.Items.Sum(i => i.Price);
order.RefundProcessedAt = DateTime.UtcNow;
// Update item statuses
foreach (var item in order.Items)
{
item.Status = ItemStatus.Cancelled;
}
await _db.SaveChangesAsync();
// Send email
await _emailService.SendCancellationConfirmation(order.Customer.Email, order.Id);
// Log event
_logger.LogInformation("Order {OrderId} cancelled by customer {CustomerId}", order.Id, order.Customer.Id);
// Return DTO
var response = new OrderDto
{
Id = order.Id,
Status = order.Status.ToString(),
RefundAmount = order.RefundAmount,
CancelledAt = order.CancelledAt
};
return Ok(response);
}
catch (Exception ex)
{
_logger.LogError(ex, "Error cancelling order {OrderId}", id);
return StatusCode(500, "An error occurred");
}
}
}This action violates the single responsibility principle in ways that make testing painful and changes risky. Let's fix it.
Extract Use Cases: Move Logic Into Handlers
The controller should translate HTTP requests and responses. Everything else belongs in an application service or command handler. We'll use a simple handler pattern—no heavy CQRS framework required.
First, create a command class representing the intent:
public class CancelOrderCommand
{
public int OrderId { get; set; }
public string Reason { get; set; }
}Then create a handler that orchestrates the use case:
public class CancelOrderCommandHandler
{
private readonly OrderRepository _orderRepository;
private readonly IEmailService _emailService;
private readonly ILogger<CancelOrderCommandHandler> _logger;
public CancelOrderCommandHandler(
OrderRepository orderRepository,
IEmailService emailService,
ILogger<CancelOrderCommandHandler> logger)
{
_orderRepository = orderRepository;
_emailService = emailService;
_logger = logger;
}
public async Task<CancelOrderResult> Handle(CancelOrderCommand command)
{
var order = await _orderRepository.GetByIdWithItems(command.OrderId);
if (order == null)
throw new OrderNotFoundException(command.OrderId);
// Delegate validation to the domain
order.Cancel(command.Reason);
await _orderRepository.Save(order);
// Side effects: email and logging
await _emailService.SendCancellationConfirmation(
order.Customer.Email,
order.Id);
_logger.LogInformation(
"Order {OrderId} cancelled. Refund: {Amount}",
order.Id,
order.RefundAmount);
return new CancelOrderResult
{
OrderId = order.Id,
Status = order.Status,
RefundAmount = order.RefundAmount,
CancelledAt = order.CancelledAt.Value
};
}
}Now the controller becomes a thin HTTP translator:
[ApiController]
[Route("api/orders")]
public class OrdersController : ControllerBase
{
private readonly CancelOrderCommandHandler _cancelOrderHandler;
public OrdersController(CancelOrderCommandHandler cancelOrderHandler)
{
_cancelOrderHandler = cancelOrderHandler;
}
[HttpPost("{id}/cancel")]
public async Task<IActionResult> CancelOrder(int id, [FromBody] CancelOrderRequest request)
{
try
{
var command = new CancelOrderCommand { OrderId = id, Reason = request.Reason };
var result = await _cancelOrderHandler.Handle(command);
return Ok(new OrderDto
{
Id = result.OrderId,
Status = result.Status.ToString(),
RefundAmount = result.RefundAmount,
CancelledAt = result.CancelledAt
});
}
catch (OrderNotFoundException)
{
return NotFound();
}
catch (OrderCancellationException ex)
{
return BadRequest(ex.Message);
}
}
}Move Rules Into the Domain: The Order Entity
Validation and invariants belong on the entity itself. An Order entity should know its own rules. This is domain-driven design applied pragmatically.
public class Order
{
public int Id { get; private set; }
public OrderStatus Status { get; private set; }
public DateTime CreatedAt { get; private set; }
public DateTime? CancelledAt { get; private set; }
public decimal RefundAmount { get; private set; }
public List<OrderItem> Items { get; private set; }
public Customer Customer { get; private set; }
public void Cancel(string reason)
{
// Rule 1: Cannot cancel if already in a terminal state
if (Status == OrderStatus.Cancelled || Status == OrderStatus.Completed)
throw new OrderCancellationException($"Cannot cancel order in {Status} state");
// Rule 2: Cannot cancel orders older than 30 days
if ((DateTime.UtcNow - CreatedAt).TotalDays > 30)
throw new OrderCancellationException("Orders older than 30 days cannot be cancelled");
// Rule 3: Cannot cancel if items are shipped
var shippedItems = Items.Where(i => i.Status == ItemStatus.Shipped).ToList();
if (shippedItems.Any())
throw new OrderCancellationException("Cannot cancel orders with shipped items");
// State transition
Status = OrderStatus.Cancelled;
CancelledAt = DateTime.UtcNow;
RefundAmount = Items.Sum(i => i.Price);
// Cascade cancellation to items
foreach (var item in Items)
{
item.Cancel();
}
}
}Now the handler doesn't decide—it asks the entity. The entity enforces its invariants. Testing the business logic doesn't require a database or HTTP layer.
Add Tests First: Characterization Tests Make Refactoring Safe
Before refactoring, write characterization tests around the current behavior. These tests document what the system does today, right or wrong. They give you a safety net.
[TestFixture]
public class CancelOrderTests
{
private Order _order;
[SetUp]
public void Setup()
{
_order = new Order
{
Id = 1,
Status = OrderStatus.Pending,
CreatedAt = DateTime.UtcNow.AddDays(-5),
Items = new List<OrderItem>
{
new OrderItem { Id = 1, Status = ItemStatus.Pending, Price = 100 }
},
Customer = new Customer { Email = "test@example.com" }
};
}
[Test]
public void Cancel_WhenOrderIsPending_Succeeds()
{
_order.Cancel("Customer requested");
Assert.That(_order.Status, Is.EqualTo(OrderStatus.Cancelled));
Assert.That(_order.CancelledAt, Is.Not.Null);
Assert.That(_order.RefundAmount, Is.EqualTo(100));
}
[Test]
public void Cancel_WhenOrderIsOlderThan30Days_ThrowsException()
{
_order.CreatedAt = DateTime.UtcNow.AddDays(-31);
var ex = Assert.Throws<OrderCancellationException>(() => _order.Cancel("Too old"));
Assert.That(ex.Message, Does.Contain("30 days"));
}
[Test]
public void Cancel_WhenItemIsShipped_ThrowsException()
{
_order.Items[0].Status = ItemStatus.Shipped;
var ex = Assert.Throws<OrderCancellationException>(() => _order.Cancel("Shipped"));
Assert.That(ex.Message, Does.Contain("shipped"));
}
}These tests document behavior without touching the database. Once they pass, you can refactor the controller and handler with confidence. If a test fails after refactoring, you've accidentally changed behavior.
Refactor in Small PRs: One Action at a Time
Don't refactor the entire controller at once. Pick one endpoint, extract its logic, test it, merge it, then move to the next. This approach:
- Keeps pull requests small and reviewable
- Makes reverting easier if something breaks
- Spreads the work across the team
- Reduces merge conflict pain
A typical PR workflow:
- Extract the command class and handler for one action
- Write unit tests for the handler and domain entity
- Update the controller to use the handler
- Write an integration test covering the HTTP layer
- Deploy and monitor
One PR, one endpoint, done. Repeat for the next 19 endpoints.
Common Mistakes
Mistake 1: Moving code without extracting rules
Just moving DbContext calls into a service layer doesn't help if the business logic stays scattered. The handler becomes a thinner controller. Extract the rules into the domain entity instead.
Mistake 2: Creating a god handler
If your handler has 10 dependencies and 500 lines, you've just moved the problem. Keep handlers focused on orchestration. If a handler is doing too much, split it into smaller handlers or create an application service that composes them.
Mistake 3: Refactoring without tests
Refactoring without tests is debugging. Write characterization tests first. They document current behavior and catch regressions.
Mistake 4: Big bang refactors
Refactoring everything at once creates a massive PR that sits in review for a week, conflicts with everyone else's work, and introduces subtle bugs. Refactor incrementally. One action per PR.
Key Takeaways
- Controllers should translate HTTP; they shouldn't contain business logic
- Extract logic into command handlers or application services
- Move validation and rules into the domain entity where they belong
- Write characterization tests before refactoring to catch regressions
- Refactor one endpoint at a time in small, reviewable pull requests
- Keep handlers focused on orchestration, not implementation details
- A 900-line controller is always composed of smaller, extractable use cases
Comments
Loading comments…