2

We are creating the backend for a web app using a sort of layered architecture. The service layer is supposed to handle the business logic and passing data to the repositories. This service layer is going to be called from 3-4 projects (.Net Core MVC projects). Many of our services are starting to run into an issue of having too many dependencies and, in my opinion, breaking the SRP. For example, one of our requirements is that certain users can create orders. The base logic for this operation is simple:

  1. Save order to database
  2. Increment user's order count

However, we also have other requirements to create an order:

  1. User must be authorized and allowed to create orders
  2. Order must valid (price is positive, number of objects is positive, etc)
  3. We should log if an error occurred
  4. We should log that an order was created
  5. Created orders should be stored in cache
//All services are passed into constructor by DI
public class OrderService : IOrderService {
    //Services to handle extra checks (authorization, validation, logging, caching)
    private readonly IOrderAuthorizationService _authService;
    private readonly IOrderValidationService _validationService;
    private readonly ILogger _logger;
    private readonly ICacheService _cache;

    //Services to actually complete operation
    private readonly IUserRepo _userRepo;
    private readonly IOrderRepo _orderRepo;


    //Creates an order and increments the user's order count. Returns the created order or null if an order wasn't created
    public Order CreateOrder(CreateOrderModel model) {
        var isAuthorized = _authService.IsAuthorizedToCreateOrder();
        if (!isAuthorized)
            return null;

        //Validate price isn't negative, date is correct, etc
        var isValid = _validationService.IsValid(model);
        if (!isValid)
            return null;

        Order order;
        try {
            order = _orderRepo.CreateOrder(model);
            _userRepo.IncrementOrderCount(order.UserId);
        } catch (Exception e) {
            _logger.LogError(e);
            return null;
        }

        _logger.LogOrderCreated(model);
        _cache.Save(order);
    }
}

This method seems to break the SRP because of how many different things its doing. Most of our services need to handle these 4 base operations (authorization, validation, caching, logging). This means that many of the methods in our service layer end up looking very similar to the example above. Plus, it means that most of our services need 4-5 dependencies in the constructor. What is the best way to handle this scenario? One way I thought of was to use the decorator pattern. The decorator pattern with dependency injection would look something like:

public abstract class OrderDecorator : IOrderService {
    //Base order service which can be injected from DI container
    private BaseOrderService _orderService;
    public virtual Order CreateOrder(CreateOrderModel model) {
        return _orderService.CreateOrder(model);
    }
}

public class BaseOrderService : IOrderService {
    private readonly IUserRepo _userRepo;
    private readonly IOrderRepo _orderRepo;
    public Order CreateOrder(CreateOrderModel model) {
        var order = _orderRepo.CreateOrder(model);
        _userRepo.IncrementOrderCount(order.UserId);
        return order;
    }
}

public class OrderAuthorizationDecorator : OrderDecorator {
    public override Order CreateOrder(CreateOrderModel model) {
        if (!IsAuthorizedToCreateOrder())
            return null;
        return base.CreateOrder(model);
    }

    public bool IsAuthorizedToCreateOrder() {
        //Check if the currently logged in user is allowed to create order
        return true;
    }
}

public class OrderValidationDecorator : OrderDecorator {
    public override Order CreateOrder(CreateOrderModel model) {
        if (!IsValid())
            return null;
        return base.CreateOrder(model);
    }

    public bool IsValid(CreateOrderModel model) {
        //Check if model is valid
        return true;
    }
}

public class OrderCacheDecorator : OrderDecorator {
    public override Order CreateOrder(CreateOrderModel model) {
        var order = base.CreateOrder(model);
        if (order != null) {
            //Store in cache
        }
        return order;
    }
}

public class OrderLoggingDecorator : OrderDecorator {
    public override Order CreateOrder(CreateOrderModel model) {
        try {
            var order = base.CreateOrder(model);
            //Log order was created
            return order;
        } catch (Exception e) {
            //Log error
            return null;
        }
    }
}

This at least splits responsibilities into different classes however I'm not sure if this is the best implementation. I thought about leaving authorization/validation to the calling code but with so many different places it can be called, it seems easy to miss something. I thought about using some sort of service facade to combine dependencies but because the functionality is so different, I can't figure out how to split it up. Is the decorator pattern useful is situations like this? Is there a better way?

0

1 Answer 1

0

The main drawback I see with using the decorator pattern is how you can mix and match the order that decorators are applied.

It is a little difficult to know the usage from your question, but it seems like you will end up with:

new OrderAuthorizationDecorator(
    new OrderValidationDecorator(
        new OrderCacheDecorator(
            new OrderService(...))));

The problem is that you can change up the order that decorators are nested:

new OrderCacheDecorator( // <-- used to be OrderAuthorizationDecorator
    new OrderValidationDecorator(
        new OrderAuthorizationDecorator( // <-- used to be OrderCacheDecorator
            new OrderService(...))));

This compiles, but does not provide correct runtime behavior.

Typically authorization and validation are triggered by the framework. Your code seems like C#, so that would be the .NET MVC or API frameworks. Authorization happens before the controller is invoked, and validation usually has some integration into the framework as well. The challenge with your code base is that authorization and validation are pushed into the service layer.

In this sense, services act more like a controller than a service. This comes from a desire to reuse code across multiple projects — a valid concern, in my opinion. Service classes become coordinators between objects. I would argue that the original service class does not violate the Single Responsibility Principal. SRP is not "do one thing". SRP merely states that you should group code together when it has the same reason to change. The service class coordinates authorization, validation, caching and data access. When coordination logic changes, the service class will change, but you can still swap out authorization schemes without affecting the service class.

Rather than violating the SRP, the number of constructor parameters seems to be the larger issue. There are a few improvements that could be made to reduce constructor arguments:

  • Move logic to increment the order count into the OrderRepository. If creating an order should always increment this, then it sounds like a concern for the OrderRepository. This removes the IUserRepository dependency.

  • Caching is a data access concern. Upon creating a new order, the OrderRepository should cache the new order. This is actually where the decorator pattern could be successfully applied:

    public interface IOrderRepository
    {
         public Order CreateOrder(CreateOrderModel model);
    }
    
    public class OrderRepository : IOrderRepository
    {
         public Order CreateOrder(CreateOrderModel model)
         {
             // The real creation logic goes here
         }
    }
    
    public class CacheOrderRepository : IOrderRepository
    {
         private IOrderRepository repository;
         private readonly ICacheService cache;
    
         public CacheOrderRepository(IOrderRepository repository, ICacheService cache)
         {
             this.repository = repository;
             this.cache = cache;
         }
    
         public Order CreateOrder(CreateOrderModel model)
         {
             var order = repository.CreateOrder(model);
    
             cache.Save(order);
    
             return order;
         }
    }
    

    Then nest the repositories:

    var cache = new WhateverCacheService();
    var repository = new CacheOrderRepository(new OrderRepository(...), cache);
    

    This removes another dependency from the service class.

Validation, authorization and logging seem very specific to the use case, and would not be a good fit for the decorator pattern. You want these things to happen in a very specific order, and there is nothing wrong with making that procedural code.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.