5

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

2 Answers 2

1

This method seems to break the SRP because of how many different things its doing.

I disagree. There is only one actual responsibility in this service: orchestration.

The service is telling the repository to save the order to the database and, subsequently in a separate instruction, to increase the order count. The service does not know anything about the storage mechanism.

The service is asking the authorization service whether this command is authorized or not. The service does not know how such authorization is defined or confirmed.

The service is asking the validator whether the input is valid or not. It does not know the validation rules.

The service is telling the logger to log, it does not know where the logs are being written.

The service is loading the cache, but it makes sense since it's already got the relevant data and the cache needs to get it from somewhere.

The separation of responsibilities is really good here, in my opinion.

Think of it this way: a team consisting of a handful of people who each specialize in a job are able to just talk to one another in order to cooperate and otherwise just do their job. However, as the team grows in size, the cooperative overhead grows exponentially and you need to introduce a manager. They don't actually do "the work", they simply manager the workers in order to make sure that the cooperation between the workers is efficient and productive.

Your service is effectively a manager. It's not doing the work itself, it is orchestrating the workers (who do the actual work) and keeping track that all the necessary steps are being done.
Managers have several workers, and therefore have several dependencies. That is business as usual. There's a sensible upper limit but you're not there yet.

I do not think you need to refactor your original code.

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.

Just because two things look alike does not necessarily mean that they are the same (and therefore in need of reusable abstraction). Maybe there is some benefit gained from abstracting this similar flow in your business services. Maybe you're just abstracting for abstraction's sake. I cannot judge this based on the information in the question.

Are you doing it as an optimization to prevent reinventing the same wheel over and over, or are you doing it to create a "checklist" for developers so they don't forget a step? Given the simplicity of the logic in your first snippet, I'm guessing it's the latter, and I don't believe that this is the right reason to introduction the abstraction.

But as I said, I cannot conclusively judge this based on what you've said alone.

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.

1
  • Even rearranging Cache and ID generation into repositories, the main problem remains. Procedural style tends to this. And it will get worse when services start to depend on each other (circular references). When services have the business logic they are not orchestrating they are doing something else. The problem is that it's not self-evident at the first glance. While I agree with your suggestions, I think that will only solve the problem now but not in the long run.
    – Laiv
    Sep 10, 2022 at 10:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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