Returning rental invoice data transfer object

Question

When trying to follow single responsibility principle, I often find myself in a position where there is so much to do that I do not know where to start. Maybe there is there a rule of thumb that I could use?

A concrete example originating from my test project.

    // doing way too many things
    public InvoiceDto GetInvoice(CartDto cart)
    {
        if (cart == null)
            throw new ArgumentNullException(nameof(cart));                                      // validation

        var cartequipments = _carts.GetById(cart.CartId).CartEquipments.ToList();               // quering
        var rentals = cartequipments.Select(o =>                                                // mapping
            new RentalDto                                                                       // object creation
            {
                Name = o.Equipment.EquipmentName,
                Price = _mapPriceCalculatorLogic                                                // buisiness logic
                    .Create(o.Equipment.EquipmentType)
                    .Calculate(o.RentDurationDays)
            }).ToList();
        var loyaltyPoints = _loyaltyPointsService                                               // buisiness logic
            .GetLoyaltyPoints(cartequipments.Select(o => o.Equipment.EquipmentType)); 
        var total = rentals.Sum(o => o.Price);                                                  // aggregation

        var invoiceDto = new InvoiceDto                                                         // object creation 
        {
            Title = $"Invoice id : {cart.CartId}",
            Rentals = rentals,
            LoyaltyPoints = loyaltyPoints,
            TotalPrice = total
        };

        return invoiceDto;
    }

My question is how would you clean this up and if possible what would the result look like (helper methods sources not needed).

Answer 1

Make separate methods for getting equipment from the cart, creating rentals from the equipment, getting loyalty points from the equipment, and creating the invoice from the rentals and loyalty points.

Answer 2

Your code is well formatted and easy to read. Congratulations.

With regards to SRP, I believe your code complies just fine. Each dependency: the cart, rentals, and loyalty points is retrieved with a single call. The mapping for those DTO are isolated in a small single purpose anonymous method. The single responsibility for the method matches its name GetInvoice.

If your uncomfortable with the idea of anonymous methods being separate from the method where they are defined, consider creating private static named methods in this method's class and referencing them.

The only SRP violation I'm concerned with is the invoice DTO. I would recommend using the raw cart id and letting your presentation code format the title.

Here's an example of how I may clean it up:

public InvoiceDto GetInvoice(CartDto cart)
{
    if (cart == null)
        throw new ArgumentNullException(nameof(cart));

    var cartequipments = _carts.GetById(cart.CartId).CartEquipments.ToList();
    var rentals = cartequipments.Select(o => MapEquipmentToRentalDto).ToList();
    var loyaltyPoints = _loyaltyPointsService
        .GetLoyaltyPoints(cartequipments.Select(o => o.Equipment.EquipmentType)); 
    var total = rentals.Sum(o => o.Price);

    var invoiceDto = new InvoiceDto 
    {
        InvoiceId = cart.CartId,
        Rentals = rentals,
        LoyaltyPoints = loyaltyPoints,
        TotalPrice = total
    };

    return invoiceDto;
}

private static RentalDto MapEquipmentToRentalDto(CartEquipment o)
{
    return new RentalDto 
    {
        Name = o.Equipment.EquipmentName,
        Price = _mapPriceCalculatorLogic
                .Create(o.Equipment.EquipmentType)
                .Calculate(o.RentDurationDays)
    };
}