Domain-Driven Refactoring: Long Methods

Posts in this series:

In the last post, I walked through the main, immediate code smell we saw of a long method, and I would classify this method as long:

public class AssignOfferHandler : IRequestHandler<AssignOfferRequest>
{
    private readonly AppDbContext _appDbContext;
    private readonly HttpClient _httpClient;

    public AssignOfferHandler(
        AppDbContext appDbContext,
        HttpClient httpClient)
    {
        _appDbContext = appDbContext;
        _httpClient = httpClient;
    }

    public async Task<Unit> Handle(AssignOfferRequest request, CancellationToken cancellationToken)
    {
        var member = await _appDbContext.Members.FindAsync(request.MemberId, cancellationToken);
        var offerType = await _appDbContext.OfferTypes.FindAsync(request.OfferTypeId, cancellationToken);

        // Calculate offer value
        var response = await _httpClient.GetAsync(
            $"/calculate-offer-value?email={member.Email}&offerType={offerType.Name}",
            cancellationToken);

        response.EnsureSuccessStatusCode();

        await using var responseStream = await response.Content.ReadAsStreamAsync(cancellationToken);
        var value = await JsonSerializer.DeserializeAsync<int>(responseStream, cancellationToken: cancellationToken);

        // Calculate expiration date
        DateTime dateExpiring;

        switch (offerType.ExpirationType)
        {
            case ExpirationType.Assignment:
                dateExpiring = DateTime.Today.AddDays(offerType.DaysValid);
                break;
            case ExpirationType.Fixed:
                dateExpiring = offerType.BeginDate?.AddDays(offerType.DaysValid)
                               ?? throw new InvalidOperationException();
                break;
            default:
                throw new ArgumentOutOfRangeException();
        }

        // Assign offer
        var offer = new Offer
        {
            MemberAssigned = member,
            Type = offerType,
            Value = value,
            DateExpiring = dateExpiring
        };
        member.AssignedOffers.Add(offer);
        member.NumberOfActiveOffers++;

        await _appDbContext.Offers.AddAsync(offer, cancellationToken);

        await _appDbContext.SaveChangesAsync(cancellationToken);

        return Unit.Value;
    }
}

The telltale signs are code comments denoting notable sections of our method. You might see comments, or worse, regions, to try to break up a long method. However, we want self-documenting code instead of code comments (which lie), so the answer is to extract methods for these more complex sections.

So what should we extract into methods? When examining our code, we broke down the overall set of operations to 3 overall parts:

  1. Load data from database into objects
  2. Mutate objects
  3. Save objects' data into database

We could extract each of these parts into a method, but examining the original code, we saw that our code comments weren't written as these three parts. It was broken down further:

  1. Data access to load data models we care about
  2. Calculate offer value using external web API
  3. Calculate expiration date based on the OfferType
  4. Mutate our data models to assign an offer to our member
  5. Save our data to the database

And only 2-4 had comments. Initially, we can look at these and pick out the most complex part to extract a method for, which I think is #4. Let's extract a method for that part, using the text in the comment as a guide:

public async Task<Unit> Handle(AssignOfferRequest request, CancellationToken cancellationToken)
{
    var member = await _appDbContext.Members.FindAsync(request.MemberId, cancellationToken);
    var offerType = await _appDbContext.OfferTypes.FindAsync(request.OfferTypeId, cancellationToken);

    // Calculate offer value
    var response = await _httpClient.GetAsync(
        $"/calculate-offer-value?email={member.Email}&offerType={offerType.Name}",
        cancellationToken);

    response.EnsureSuccessStatusCode();

    await using var responseStream = await response.Content.ReadAsStreamAsync(cancellationToken);
    var value = await JsonSerializer.DeserializeAsync<int>(responseStream, cancellationToken: cancellationToken);

    // Calculate expiration date
    DateTime dateExpiring;

    switch (offerType.ExpirationType)
    {
        case ExpirationType.Assignment:
            dateExpiring = DateTime.Today.AddDays(offerType.DaysValid);
            break;
        case ExpirationType.Fixed:
            dateExpiring = offerType.BeginDate?.AddDays(offerType.DaysValid)
                           ?? throw new InvalidOperationException();
            break;
        default:
            throw new ArgumentOutOfRangeException();
    }

    // Assign offer
    var offer = AssignOffer(member, offerType, value, dateExpiring);

    await _appDbContext.Offers.AddAsync(offer, cancellationToken);

    await _appDbContext.SaveChangesAsync(cancellationToken);

    return Unit.Value;
}

The extracted method becomes:

private static Offer AssignOffer(
    Member member, 
    OfferType offerType, 
    int value, 
    DateTime dateExpiring)
{
    var offer = new Offer
    {
        MemberAssigned = member,
        Type = offerType,
        Value = value,
        DateExpiring = dateExpiring
    };
    member.AssignedOffers.Add(offer);
    member.NumberOfActiveOffers++;
    return offer;
}

The original method I think is cleaner, but we're missing something here.

Extracting and composing methods

One common mistake I see is to only extract methods for individual complex sections, resulting in a method with different levels of abstraction. If we're trying to make our code more intention-revealing, it's much more difficult if the reader has a single method that zooms in and out at different conceptual levels.

That's where I like to use the Compose Method refactoring, where I break a long method down into multiple methods, each as a single intention-revealing step:

public async Task<Unit> Handle(AssignOfferRequest request, CancellationToken cancellationToken)
{
    var member = await _appDbContext.Members.FindAsync(request.MemberId, cancellationToken);
    var offerType = await _appDbContext.OfferTypes.FindAsync(request.OfferTypeId, cancellationToken);

    // Calculate offer value
    var value = await CalculateOfferValue(member, offerType, cancellationToken);

    // Calculate expiration date
    var dateExpiring = CalculateExpirationDate(offerType);

    // Assign offer
    var offer = AssignOffer(member, offerType, value, dateExpiring);

    await _appDbContext.Offers.AddAsync(offer, cancellationToken);
    await _appDbContext.SaveChangesAsync(cancellationToken);

    return Unit.Value;
}

I usually leave the data access ones alone because they're already (mostly) intention-revealing, but that's just a preference. I can remove the code comments because they're not adding anything at this point. Each method now does exactly what the code comment was describing:

private async Task<int> CalculateOfferValue(Member member, OfferType offerType,
    CancellationToken cancellationToken)
{
    var response = await _httpClient.GetAsync(
        $"/calculate-offer-value?email={member.Email}&offerType={offerType.Name}",
        cancellationToken);

    response.EnsureSuccessStatusCode();

    await using var responseStream = await response.Content.ReadAsStreamAsync(cancellationToken);
    var value = await JsonSerializer.DeserializeAsync<int>(responseStream, cancellationToken: cancellationToken);
    return value;
}

And ReSharper tells me I can make that switch statement into a switch expression, so let's do that:

private static DateTime CalculateExpirationDate(OfferType offerType)
{
    DateTime dateExpiring = offerType.ExpirationType switch
    {
        ExpirationType.Assignment => DateTime.Today.AddDays(offerType.DaysValid),
        ExpirationType.Fixed => offerType.BeginDate?.AddDays(offerType.DaysValid) ??
                                throw new InvalidOperationException(),
        _ => throw new ArgumentOutOfRangeException(nameof(offerType))
    };

    return dateExpiring;
}

That extra variable doesn't seem to be adding much, so let's apply Inline Variable to dateExpiring:

private static DateTime CalculateExpirationDate(OfferType offerType) =>
    offerType.ExpirationType switch
    {
        ExpirationType.Assignment => DateTime.Today.AddDays(offerType.DaysValid),
        ExpirationType.Fixed => offerType.BeginDate?.AddDays(offerType.DaysValid) ??
                                throw new InvalidOperationException(),
        _ => throw new ArgumentOutOfRangeException(nameof(offerType))
    };

Inline Variable, the inverse of Extract Variable, is what I like to call a "defactoring", where we're applying a reversal of a refactoring. I do this constantly in my refactoring, trying one direction, seeing if it looks OK, and if not, apply the inverse refactoring.

Now that I'm complete, I might just stop here, as my original method is concise and readable. However, this series isn't just about making procedural code easier to understand, it's about behavioral models with DDD. In the next post, we'll look at these individual methods to see where we can extract our logic to a more DDD, object-oriented location.