Shopping cart purchase method in service layer

Question

The following is a class in the service layer of an ASP.NET Core web application, which currently contains a single method: PurchaseCart. The intention is that an ID is passed in to a "shopping cart" entity and that the method will complete the purchasing of all the items stored in that entity.

This entails the following:

• A PurchaseAttempt object is created from the cart object
• Its validity is checked
• Payment is attempted to be taken for the PurchaseAttempt
• If successful, a Purchase object is created from the PurchaseAttempt object
• Other activities are carried out relating to a successful purchase (OnPurchaseComplete)
• A response object is returned describing the outcome of the operation

I have a number of concerns about this code, which I've listed below.

public class PurchaseService : IPurchaseService {

private readonly IUnitOfWork _uow;
private readonly IMapper _mapper;
private readonly IPaymentProviderFactory _paymentProviderFactory;
private readonly IEmailHandler _emailHandler;
private readonly ILogger<PurchaseService> _logger;

public PurchaseService(IUnitOfWork uow, IMapper mapper, IPaymentProviderFactory paymentProviderFactory, IEmailHandler emailHandler, ILogger<PurchaseService> logger) {
    _uow = uow;
    _mapper = mapper;
    _paymentProviderFactory = paymentProviderFactory;
    _emailHandler = emailHandler;
    _logger = logger;
}

public PurchaseCartResponse PurchaseCart(Guid cartId) {

    var response = new PurchaseCartResponse();

    if (cartId != Guid.Empty) {

        // Get cart
        var cart = _uow.CartRepository.GetAll()
                           .Include(c => c.CartItems)
                           .ThenInclude(ci => ci.Product)
                           .Where(x => x.Id.Equals(cartId))
                           .SingleOrDefault();

        // Create purchase attempt from cart data and save
        var candidatePurchaseAttempt = new PurchaseAttempt(cart);
        _uow.PurchaseAttemptRepository.Create(candidatePurchaseAttempt);
        _uow.Save();

        // Set ID of purchase attempt in the response
        response.PurchaseAttemptId = candidatePurchaseAttempt.Guid;

        // Stop tracking the entity, so we can track the reloaded entity
        _uow.PurchaseAttemptRepository.DetachAll();

        // Reload with all related data
        var purchaseAttempt = _uow.PurchaseAttemptRepository.GetAll()
                                    .Include(pa => pa.Customer)
                                    .ThenInclude(c => c.Addresses)
                                    .Include(pa => pa.PurchaseItems)
                                    .ThenInclude(pi => pi.Product)
                                    .Include(pa => pa.Card)
                                    .Where(x => x.Id.Equals(candidatePurchaseAttempt.Id))
                                    .SingleOrDefault();

        // Check validity of purchase attempt (we're doing this after to save a bit of unnecessary loading in the cart
        var validity = purchaseAttempt.ValidateAll();

        if (validity.IsValid) {

            response.PaymentAttempted = true;

            // Take payment for purchase attempt record
            var paymentRequest = new ProcessPaymentRequest() {
                ChargeAmount = purchaseAttempt.Total,
                ChargeDescription = $"Purchase attempt {purchaseAttempt.Id}",
                Customer = purchaseAttempt.Customer,
                Card = purchaseAttempt.Card
            };
            var paymentProvider = _paymentProviderFactory.GetPaymentProvider(PaymentProviderTypes.Stripe);
            var paymentResponse = paymentProvider.ProcessPayment(paymentRequest);

            // Save customer if they were created by the payment provider (charge may still have been unsuccessful)
            if (!String.IsNullOrEmpty(paymentResponse.PaymentProviderCustomerId)) {
                purchaseAttempt.Customer.PaymentProviderCustomerId = paymentResponse.PaymentProviderCustomerId;
                _uow.PurchaseAttemptRepository.Update(purchaseAttempt);
                _uow.Save();
            }

            // Now proceed based on whether payment was successfully taken
            if (paymentResponse.Success) {

                // Create purchase record
                var purchase = new Model.Entities.Purchase(purchaseAttempt, paymentResponse);
                _uow.PurchaseRepository.Create(purchase);
                _uow.Save();

                // Take actions for completed purchase - sending emails etc.
                OnPurchaseComplete(purchase.Id);

                // Finalise response
                response.Success = true;
                response.PurchaseId = purchase.Guid;

            }
            else {

                // Payment processing failed
                purchaseAttempt.Status = PurchaseStatus.Failed;
                purchaseAttempt.PaymentProviderFailCode = paymentResponse.ErrorCode;
                purchaseAttempt.ErrorMessage = paymentResponse.ErrorMessage;
                _uow.PurchaseAttemptRepository.Update(purchaseAttempt);
                _uow.Save();

                response.SetGeneralError(paymentResponse.ErrorMessage ?? "An error occurred while trying to process the payment. No payment was taken.");

            }

        }
        else {

            // PurchaseAttempt failed validation
            purchaseAttempt.Status = PurchaseStatus.Failed;
            purchaseAttempt.ErrorMessage = "Data for your purchase isn't valid. Please restart your order, re-entering your data. Sorry for the inconvenience.";
            _uow.PurchaseAttemptRepository.Update(purchaseAttempt);
            _uow.Save();

            response.SetUnexpectedError("Invalid cart data");
            _logger.LogError($"PurchaseAttempt validation for ID {purchaseAttempt.Id} failed with error: '${validity.Error}'");

        }

    }
    else {
        response.SetUnexpectedError("Cart not found");
        _logger.LogError($"Cart not found with ID: {cartId}");
    }

    return response;

}

}

Concerns:

1. My main concern is with how I'm utilising the service layer in general and whether it violates SRP (or anything else for that matter). I find myself having to carry out a number of not-necessarily-related operations in order to complete a given process. As much as I try to move the implementation of the operations elsewhere, I find myself with a long method that does a number of different things. I would really like to find a way to better structure my code that avoids such monolithic methods, but am struggling to reduce it further than I already have in a way that's meaningful. In some cases, such as the method "OnPurchaseComplete", I'm moving functionality (in this case the sending of emails - code not listed for brevity) into a method that will likely not be called from anywhere else, which seems like a bad solution.
2. My secondary issues are relating to my use of EntityFramework. After creating an entity, it doesn't have all of its nagivation properties accessible, which I need to have loaded. As such I'm reloading the object after creation (creating of PurchaseAttempt, followed by loading it again after). This seems like an inelegance forced on me by the technology, not something I would otherwise choose to do. Lazy-loading would solve it, but I have avoided going that route due to performance issues (it would require multiple database hits whereas the approach I'm currently using where I specify all navigation properties to load with the use of "Include" is minimising trips to the database).

Please tell me anywhere you think I can improve my approach. I'm happy to update the question / add additional code if needed.

Answer 1

First, you should get in the habit of doing early-exits. You have if (cartId != Guid.Empty), followed by a massive block of code. Then an else with a couple simple error-handling lines. The better way to do this, so the error sequence is obvious to any viewer without dissecting the method, is:

if (cartId == Guid.Empty)
{
    response.SetUnexpectedError("Cart not found");
    _logger.LogError($"Cart not found with ID: {cartId}");
    return response;
}

// rest of code here

---

Your method is doing way too much; split it into multiple methods. Candidates would be the blocks of if (paymentResponse.Success) and the matching else and the block of if (validity.IsValid) and its matching else.

---

In regards to your concerns, you are doing OK by SRP, it seems, but are lacking other architectural concerns; congrats on noticing this yourself!

I'm moving functionality (in this case the sending of emails - code not listed for brevity) into a method that will likely not be called from anywhere else, which seems like a bad solution.

This is tripping you up. It's OK if a method/function is used in only one place--just make it a private method on the class. Private methods are implementation details. The caller doesn't know that you have a method there. It shouldn't know. It only needs to know the public methods--the "API". If you want to unit test this method as an individual piece, then make it internal and use the InternalsVisibleTo attribute on the assembly with the name of the test assembly as an argument.

After creating an entity, it doesn't have all of its nagivation (sic) properties accessible, which I need to have loaded.

That's OK; I have to do it that way myself. It's far better to do a simple reload than to populate each property separately--and have to change it every time you add a new property. Have you checked whether you need to untrack the item first, though? I've noticed that I usually don't need to untrack it if all I've done is the insert. Just grab it out of the DB, and since EF is the one creating the instance, it will have the navigation properties populated.

---

Nitpick: Use the C# standard style for braces:

if() // or while(), etc.
{
}
else
{
}