Should the service layer keep any state? or have it injected via a constructor/setter only?

I have a OrderService that I use in my controllers.

What I did was just inherit from a custom controller where I added the OrderService as a property. I initialize the OrderService in the OnActionExecuting event.

When initializing the OrderService, I set the Order property.

This is because the OrderService performs actions on a Order, so it makes sense to set it once instead of having to pass in a Order into every method.

Is this design following good practices?

public class MyController : Controller
{
      public OrderService OrderService {get; set;}

      protected override void OnActionExecuting(...)
      {
          OrderService = new OrderService(getOrderIdFromCookie());

      }
}



   public class OrderService
   {
        private Order _order;

        public OrderService(int orderId)
        {
          _order = Dao.GetOrderById(orderId);
        }


        public void AddProduct(Product product)
        { 
            product.OrderId = _order.Id;   // assume开发者_StackOverflow社区s order is loaded
            ProductDao.Add(product);
        }

   }

This is really a language independent question, from what I understand, a service should not really hold any state of any kind.

I believe a Service layer class should be a singleton correct? (when setting it up with dependancy injection).

The methods shouldn't assume that the Order object has state.

If that is the case, then my design is not correct right?

The service layer should not be a Singleton.

It should also NOT include Object state.

A Service layer should be able to operate on any instance of its type rather than storing a single instance...which results in having to create multiple instances of the Service to work with multiple orders. For example:

public interface IOrderService
{
    public void AddProduct(Order order, Product product);
}

public class DaoOrderService : IOrderService 
{ 
    public Order GetOrder(orderId)
    {
        return Dao.GetOrderById(orderId);
    } 

    public void AddProduct(Order order, Product product) 
    {
        // The way this method works seems like it should be in a ProductService
        // rather than in the OrderService.
        product.OrderId = _order.Id;
        ProductDao.Add(product); 
    } 
}

And then your controller would have something like:

public class MyController : Controller 
{    
    public IOrderService OrderService { get; private set; }

    public MyController(IOrderService orderService)
    {
        OrderService = orderService ?? new DaoOrderService();
    }

    public MyController()
        : this(null)
    { }

    protected override void OnActionExecuting(...) 
    {             
        Order myOrder = OrderService.GetOrder(orderId);

        // Some stuff here
        orderService.AddProduct(myOrder, selectedProduct);
    } 
} 

Most of the ORM products on the market take a different approach which I tend to think is a better practice, which is that the traverse the object graph to persist changes.

That means that instead of

    public void AddProduct(Product product)
    { 
        product.OrderId = _order.Id;   // assumes order is loaded
        ProductDao.Add(product);
    }

You just do:

myOrder.Products.Add(product);
Service.Save(myOrder);

I consider a service usually as a group of methods that I call at any time I like. To my understanding, a service is always there and I do not have to create one. Although there may by use cases that can be implemented with something like service factories.

So it's my opinion as well, that a service should be stateless - at least from the users perspective. It may keep some general state, maintained or set by the service provider, but from the user perspective it should show up like a singleton.

Can't say whether it's correct or incorrect design, but I would avoid stateful or parameterized services (or call thoses classes differently)

You are correct that your approach is somewhat unconventional in the ways you describe.

Contemporary wisdom suggests that a "Service Layer" or "Service Class" does not hold state -- it would accept all inputs (incl. the order) as parameters to its API functions. But (fwiw) your service seems to be simply an in-memory .NET object -- not a WebService or EJB or other network-based service. You don't actually have to call it a 'service' at all.

Your object could be an 'OrderProcessor' or, even better (maybe), you could just combine your service class with the order class itself.

I wish I could give a more complete description, together with a solid recommendation, but it's difficult (for me) to be very opinionated just looking at a couple interfaces. The practices an patters you allude to were created to provide advantages in certain circumstances. If I don't know what your circumstances really are, then claiming to know the "right" answer is essentially Cargo Cult Programming.