4
\$\begingroup\$

Considering the code sample below, which approach of service method design would be considered best-practice and why? The one used in SaveOrder1 or the one in SaveOrder2?

UPDATE: To be clear, I'm not talking about web service here, but application-type-agnostic service class from my business logic layer. I marked parts of code to focus on for this discussion, but included whole example for people to be able to actually try it.

UPDATE 2: As everyone is obsessed with exceptions, I just want to clarify that Alerts are not only errors. I need to return multiple messages of different importance/type from business logic in my service method, and handling exceptions is not what solves that problem.

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;

namespace Patterns
{
    class Program
    {
        static void Main(string[] args)
        {
            SaveOrder1();
            SaveOrder2();
        }

        static void SaveOrder1()
        {
            var order = new Order();
            var service = new MyService();

            //////////////////////////////////////////////////
            // Usage of service method marked as OPTION 1
            //////////////////////////////////////////////////
            var result = service.SaveOrder(order);

            ShowAlerts(result.Alerts);
            if (result.Alerts.HasErrors)
                return;

            Console.WriteLine(result.ReturnedValue);
            //////////////////////////////////////////////////
        }

        static void SaveOrder2()
        {
            var order = new Order();
            var service = new MyService();

            //////////////////////////////////////////////////
            // Usage of service method marked as OPTION 2
            //////////////////////////////////////////////////
            AlertCollection alerts;
            var orderID = service.SaveOrder(order, out alerts);

            ShowAlerts(alerts);
            if (alerts.HasErrors)
                return;

            Console.WriteLine(orderID);
            //////////////////////////////////////////////////
        }

        static void ShowAlerts(IEnumerable<Alert> alerts)
        {
            foreach (var alert in alerts)
            {
                Console.WriteLine("{0}: {1}", alert.AlertType, alert.Message);
            }
        }
    }

    class MyService
    {
        // OPTION 1
        public ServiceMethodResult<Guid> SaveOrder(Order order)
        {
            var result = new ServiceMethodResult<Guid>();

            if (order.CustomerID == Guid.Empty)
            {
                result.Alerts.Add(AlertType.Error, "CustomerID missing.");
                return result;
            }

            order.OrderID = Guid.NewGuid();  // Order will get ID when saved...

            result.Alerts.Add(AlertType.Success, "Order saved.");
            result.ReturnedValue = order.OrderID;

            return result;
        }

        // OPTION 2
        public Guid? SaveOrder(Order order, out AlertCollection alerts)
        {
            alerts = new AlertCollection();

            if (order.CustomerID == Guid.Empty)
            {
                alerts.Add(AlertType.Error, "CustomerID missing.");
                return null;
            }

            order.OrderID = Guid.NewGuid();  // Order will get ID when saved...

            alerts.Add(AlertType.Success, "Order saved.");

            return order.OrderID;
        }
    }

    class Order
    {
        public Guid OrderID { get; set; }
        public Guid CustomerID { get; set; }
    }

    class ServiceMethodResult<TResult>
    {
        public ServiceMethodResult()
        {
            this.Alerts = new AlertCollection();
        }

        public TResult ReturnedValue { get; set; }
        public AlertCollection Alerts { get; private set; }
    }

    class AlertCollection : Collection<Alert>
    {
        public void Add(AlertType alertType, string message)
        {
            this.Add(new Alert { AlertType = alertType, Message = message });
        }

        public bool HasErrors
        {
            get { return this.Count(a => a.AlertType == AlertType.Error) > 0; }
        }
    }

    class Alert
    {
        public AlertType AlertType { get; set; }
        public string Message { get; set; }
    }

    enum AlertType
    {
        Error,
        Warning,
        Success,
        Notice
    }
}
\$\endgroup\$
0

6 Answers 6

1
\$\begingroup\$

I can't think of a more appropriate place to have a discussion like this. Here are my thoughts. I prefer Option #2 with a subtle change in the calling convention, like so.

    static void SaveOrder2() {
        var order = new Order();
        var service = new MyService();
        AlertCollection alerts;
        Guid? orderID;

        ...

        if ((orderID = service.SaveOrder(order, out alerts)) == null) {
            ShowAlerts(alerts);
            return;
        }

        Console.WriteLine(orderID);
    }

The reason I like option #2 is because it is more concise. It lets you get rid of the ServiceMethodResult class all together and keeps your success or failure logic based on whether an OrderID has successfully been assigned, period. It doesn't rely on a LINQ statement that checks for the existence of a message type flag in a collection. I think that the code behind saving an order should be as atomic and clear as possible. I'm not sure I agree with the Guid thing, but that isn't really the question.

Come to think of it, if your alerts are really being used as progress indicators and not just errors, I would think about using ref instead of out to pass it throught the rest of your processes. But again, that doesn't really change the design pattern. In C++ returning a bool or a numeric value while also passing in pointer(s) to class instance(s) as function arguments is an EXTREMELY common pattern. My guess is, that is why they put out and ref parameters into the .NET framework. It certainly wasn't for speed.

I hope this is the kind of critiquing you were looking for. Take care!

Update from discussion

This is an example pattern that you could use if you implement ref. I created a fictitious scenario for a complex ordering process. This was written straight into the text window without any real functional analysis. So, please think of it as a rough pseudo-code design with lots of room for changes. But, it does accommodate atomicity and heavily exploits the encapsulation features provided by the language.

Update #2 - notice that now you can create as many different types of models as you want without ever having to create new AlertCollection instances by hand. They are already there.

  • Added ModelBase class
  • Added SomeOtherModel class that derives from ModelBase
  • Changed Order class to also derive from ModelBase
  • Added SomeOtherModel instance in the Main method
static void Main(string[] args){
    var order = new Order();
    var model = new SomeOtherModel();
    order.GetOrderDetails(); // Fill the order with all the order info. shirt sizes etc...

    //----------------------------------------------
    // This abstracts the whole order save process 
    // from the caller. It either worked or it didn't.
    //----------------------------------------------
    if(!order.Save()) {
        // react to failed order process
        // you can access order.Alerts
    }
    else {
        // do whatever is next
        // you can also access order.Alerts
    }

    // View seperate Alerts collections
    Console.WriteLine(order.Alerts.Count);
    Console.WriteLine(model.Alerts.Count);
}

//----------------------------------------------
// Use as base class for all Models
//----------------------------------------------
internal class ModelBase{
    public AlertCollection Alerts = new AlertCollection();
    ...
    // Add abstract or virtual methods and properties for anything 
    // that ALL models should do or have to prevent duplicating code.
}

public class SomeOtherModel : ModelBase {
    public SomeOtherModel(){
        Alerts.Add(AlertType.Notice, "SomeOtherModel instance created successfully.");
    }

    ...
}

//----------------------------------------------
// By making the Alerts collection part of 
// the ModelBase, it will automatically be 
// created for any class that inherits from it.
// Notice that the Alerts collection is no longer 
// declared in the Order class.
//----------------------------------------------
public class Order : ModelBase {
    public Guid OrderID { get; set; }
    public Guid CustomerID { get; set; }

    ...
    ...

    //----------------------------------------------
    // The Save() function clearly outlines the steps 
    // carried out in the complex save procedure
    //----------------------------------------------
    public bool Save(){
        if (!PreValidate())
            return false;
        if (!ProcessPayment())
            return false;
        if (!SaveToDatabase())
            return false;
        if (!ReserveInventory())
            return false;
        if (!SendConfirmEmail())
            return false;

        return true;
    }

    //----------------------------------------------
    // These functions call the OrderSave() method of each 
    // IOrderServiceProvider. Obviously, 'ref this' can't 
    // pass a null value.
    //----------------------------------------------
    private bool PreValidate() {
        return new ValidationService().OrderSave(ref this)
    }

    private bool ProcessPayment() {
        return new PaymentService().OrderSave(ref this)
    }

    private bool SaveToDatabase() {
        return new DataService().OrderSave(ref this)
    }

    private bool ReserveInventory() {
        return new InventoryService().OrderSave(ref this)
    }

    private bool SendConfirmEmail() {
        return new MessagingService().OrderSave(ref this)
    }

    ...
}

//----------------------------------------------
// Simple interface for complex processes involved with 
// processing orders. All implementations have access 
// to the order that they work on, including the order.Alerts 
// collection
//----------------------------------------------
public interface IOrderServiceProvider{
    bool OrderSave(ref Order order);
    // bool OrderPutOnBackOrder(ref Order order);
    // bool OrderUpdateFulfillmentStatus(ref Order order);
    // etc...
}

//----------------------------------------------
// Encapsulates business rules for validating 
// the quality of order information.
//----------------------------------------------
public class ValidationService : IOrderServiceProvider {
    private Order m_order = null;

    public bool OrderSave(ref Order order){
        m_order = order;
        return this.ValidateOrderContents();            
    }

    ...
}

//----------------------------------------------
// Encapsulates all functions related to inventory 
// management.
//----------------------------------------------
public class InventoryService : IOrderServiceProvider {
    private Order m_order = null;

    public bool OrderSave(ref Order order){
        m_order = order;
        return this.ReserveInvetory();            
    }

    ...
}

//----------------------------------------------
// Encapsulates all functions related to processing
// payments
//----------------------------------------------
public class PaymentService : IOrderServiceProvider {
    private Order m_order = null;

    public bool OrderSave(ref Order order){
        m_order = order;
        return this.ProcessOrderPayment();            
    }

    ...
}

//----------------------------------------------
// Encapsulates all functions that modify the 
// Orders database during order-level processes
//----------------------------------------------
public class DataService : IOrderServiceProvider {
    private Order m_order = null;

    public bool OrderSave(ref Order order){
        m_order = order;

        if(!this.SaveToDataBase()){
            order.Alerts.Add(AlertType.Error, "Unable to save order");
            return false;
        }

        return true;
    }

    private bool SaveToDataBase(){
        // SQL Stuff pseudo-code
        BeginTransaction();
        //----------------------------------------------
        // Assign db info to order.
        //----------------------------------------------
        if(ExecuteSaveCommand(m_order.Fields)){
            CommitTransaction();
            order.OrderID = result["OrderID"];
            order.CustomerID = result["CustomerID"];
            return true;
        }
        else{
            RollbackTransaction();
            return false;
        }
    }

    ...
}

//----------------------------------------------
// Encapsulates all functions related to process
// messaging.
//----------------------------------------------
public class MessagingService : IOrderServiceProvider {
    private Order m_order = null; 

    public bool OrderSave(ref Order order){     
        m_order = order;        
        return this.SendOrderConfirmationEmail();
    }

    ...
}

Now, you have an Order class that is a Model and also consumes IOrderServiceProvider classes that do order-specific functions.

Encapsulation, Inheritance, Polymorphism ... that's all I'm sayin' here.

\$\endgroup\$
10
  • \$\begingroup\$ Now that's a constructive answer :) Thanks! Regarding ref, I did use it in the beginning, but decided to use out exclusively to avoid checking for null in every method and instantiating alerts collection conditionally. Also I might accidentally replace it with new collection and loose previously collected alerts. By using out compiler forces me to instantiate new collection, and method doesn't have to care about outside world. \$\endgroup\$
    – Anil
    Dec 18, 2013 at 12:11
  • \$\begingroup\$ That being said, I somehow felt it is a bit cumbersome to explicitly declare variable with type and then pass it to the method. Also, inside the application service methods, when I call domain services, I have to declare multiple alerts variables and deal with their names. That's why I came up with this "almighty result" thing. It somehow feels more fluent. But there are pros and cons on every side :) \$\endgroup\$
    – Anil
    Dec 18, 2013 at 12:20
  • \$\begingroup\$ One more thing, I'm passing the alerts back to caller regardless of success of called method. If it didn't succeed then alerts will show the reasons in form of error messages (as much as possible at once to avoid frustration of submitting 5 times just to find out that 5 fields were not ok). On the other side, if it was successful, alerts will contain confirmation message and any possible additional notice or warning (by warning I assume a condition which did not prevent the execution, but user should be warned about the state of affairs) \$\endgroup\$
    – Anil
    Dec 18, 2013 at 12:35
  • \$\begingroup\$ Those are all valid points. In the end it's totally up to you. But, it does sort of seem that your last comment argues for implementing the ref parameter. The beauty of a pattern is that it repeats itself. You only have to create the AlertCollection once. You don't have to check for null because your can't pass ref null. I'll update my answer with what I mean. \$\endgroup\$ Dec 18, 2013 at 19:59
  • \$\begingroup\$ What I should have said in my last comment was... You don't have to check for null because your design doesn't facilitate it. \$\endgroup\$ Dec 18, 2013 at 22:46
4
\$\begingroup\$

Your 1st Option looks better to me, however need some modifictions:

  • Validate data before sending to service.
  • Change SaveOrder1 method

    if (result.Alerts.HasErrors) {
        ShowAlerts(result.Alerts); 
        return;
    }
    
  • move HasErrors to result, rather than Alerts

  • put business validation on service, like if minimum total should be greater than xxx etc
  • Change HasErrors to (if you move HasErrors to result, the code will be this.Alerts.Any()

    public bool HasErrors
    {
        get { return this.Any(); }
    }
    
  • wrap your service code in try catch and set "Unhandled Exception" with generic message in Alerts
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Everyone seems to be obsessed with freaking exceptions :) Alerts.Any() does not mean there are errors in alert collection! Imagine a ProcessOrder method which would return a success alert "order is processed" and also some additional notice (info about shipping) and maybe even some warning which says "You are approaching the limit of... something, please consider doing this and that". Why? Because there are a lot of things being checked during order processing and user should be notified, not only about failure but also about success and other stuff. \$\endgroup\$
    – Anil
    Dec 17, 2013 at 11:38
  • \$\begingroup\$ If not clear, I need to return multiple messages of different importance/type from business logic in my service method and handling exceptions is not what solves the problem here. \$\endgroup\$
    – Anil
    Dec 17, 2013 at 11:39
  • 1
    \$\begingroup\$ @Anil you can also post your own answer that says everything you want to hear. \$\endgroup\$ Dec 17, 2013 at 11:43
2
\$\begingroup\$

I find it a bit odd that a method like SaveOrder(..) would return an ServiceMethodResult which has a heavy focus on your Alerts (I know, I know! They are not exceptions...!), as in your first example. I also dislike the out parameter approach. If I were to go with the out parameter approach I would rename it from AlertCollection alerts to something like OrderSuccess.

But both of your examples are not optimal. You shouldn't do all of this in your SaveOrder() method but when placing your order, or even before.

For example;

  1. Would you want to know that the item your put in your shopping cart is actually out of stock when you are placing the order? Or would you want to know it before putting it in the shopping cart? This is a check I would do before even allowing the user to put it in the shopping cart (i.e. if(item.Stock == 0) { return OutOfStock; }

  2. Would you want to know at checkout that the delivery company is unable to deliver to your destination? Or would you want to know before checkout? This is also a check I would do before even allowing the user to checkout.

So all of this would be taken care of before invoking SaveOrder().

@Max You're not reading what I'm writing here. Alerts are not only failures and errors!

Yes I am. Because if you take care of all the things I mentioned above the only thing you need to take care of in the final step of placing the order is exceptions or returning a message to the user that the order has been placed. In the former its a simple matter of wrapping try/catch around the SaveOrder() function and in the latter it's a simple matter of, well, returning a success message.

There is also hardly any idea to return additional info such as "stock is running low" (well, this do depend on what kind of service you are offering), and even if you do find additional info to be valuable to the user you could and should include it in the same message and not divide it into several messages. By dividing it into several messages you are just hassling the user with poor UX and a user is more inclined to read a single message, than several messages.

\$\endgroup\$
2
  • \$\begingroup\$ Your points about validation before invoking final method are valid. That is something I do for more complex validations, but that doesn't solve the problem still. I want to be able to return multiple alerts (messages) from service method and I have valid reasons for that. Example: First message I add to alerts is message about successfully placed order "Order Processed", but then I check and see that customer has placed his 5th order and I move him to higher discount group, so I add second message "You have placed your 5th order and we moved you to next discount group! Enjoy!". \$\endgroup\$
    – Anil
    Dec 17, 2013 at 12:11
  • \$\begingroup\$ What I described is something we do a lot in our application and it works really well. All nested domain service calls, inside one application service method, return their messages (if any) and application service method returns them all together. I didn't want to disclose following for the sake of not pushing comments into one direction, but we are currently using the approach with "out" parameter, which I don't like. So I came up with the solution of having a generic ServiceMethodResult class, but wanted to check opinions here. \$\endgroup\$
    – Anil
    Dec 17, 2013 at 12:22
2
\$\begingroup\$

I also find the example unclear. But I get a general sense of what you're doing so I can at least speak to the alternatives.

Regarding patterns the for returning application states to the client from a web service, there are really only three approaches from a black box perspective.

  1. Use a status enumeration (your approach) on the return object or an 'errors' collection.

  2. Use an implicit value: eg, return null for nullable types

  3. Use HTTP Status codes and status descriptions

I've tried all three, but I'm a proponent of #3 because I typically work with JavaScript / AJAX on the client side and the implication of sending back an HTTP status code is that in the JavaScript you simply implement the error callback or status code conditional code. Since it also removes the status states from the return object, it ends up being cleaner code in the web service as well.

Note: I'm assuming since your question is C# you probably have access to the System.Web and System.Net namespaces - which is where HttpContext and HttpStatusCode reside.

public class MyServiceImplementation : IMyServiceContract
{
    public BusinessObject DoWork(object parameter)
    {
        BusinessObject returnItem = null;

        try
        {
            returnItem = PrimaryBusinessClass.DoWork(parameter);
        }
        catch (ArgumentException ex)
        {
            LogException(ex);
            HttpContext.Current.Response.StatusCode = (int)HttpStatusCode.BadRequest;
            HttpContext.Current.Response.StatusDescription = "Invalid arguments - [descriptive message]";
        }
        catch (MyProjectException ex)
        {
            LogException(ex);
            HttpContext.Current.Response.StatusCode = (int)HttpStatusCode.InternalServerError;
            HttpContext.Current.Response.StatusDescription = "An error occurred in the application code - [message]";
        }
        catch (Exception ex)
        {
            LogException(ex);
            HttpContext.Current.Response.StatusCode = (int)HttpStatusCode.InternalServerError;
            HttpContext.Current.Response.StatusDescription = "An unexpected error occurred - [message]";
        }

        return returnItem;
    }
}

What it buys you:

  1. Lighter / Pure return object - Should be exactly what you asked for, or empty
  2. Easier client side debugging - Web debuggers in modern browsers know to distinguish between HTTP status codes such as 200 (OK), 404, 500 etc and will surface the message / colorize the response.
  3. Cleaner service code - Separates your business logic from your web response messaging and error log messaging.
  4. Better Exception Handling - Using specific top-level exceptions to trap expected (and unexpected) error states encourages the use of proper classifiable exceptions that are thrown from the internal code. This is also significant in that you can use known and custom HTTP codes for known application error states.

Just my two cents.

\$\endgroup\$
8
  • \$\begingroup\$ Not sure why such focus on exceptions? I'll repeat the comment to a poster above: 1. I don't like to use exceptions to control the flow of my business logic. Exceptions are for something exceptional (e.g. couldn't access the database) 2. How would you return an information about success of the operation, or about a warning that didn't prevent the execution, or simply some notification, like "Order will be shipped in 5 days"? \$\endgroup\$
    – Anil
    Dec 17, 2013 at 8:47
  • \$\begingroup\$ Also the problem with your approach is that you can only get one response. But I want to be able to return multiple alerts to caller of the service method (I'm not implying web service here, but a service class in my application. Notice that the example isn't even a web application) \$\endgroup\$
    – Anil
    Dec 17, 2013 at 8:52
  • \$\begingroup\$ @Anil - Exceptions should NEVER be used for flow control. Agreed! But I'm not so sure that's what Michael Smith is doing. He's just handling different kind of exceptional cases. \$\endgroup\$
    – Max
    Dec 17, 2013 at 10:16
  • \$\begingroup\$ @Max Imagine a ProcessOrder method which would return a success alert "order is processed" and also some additional notice (info about shipping) and maybe even some warning which says "You are approaching the limit of... something, please consider doing this and that". Why? Because there are a lot of things being checked during order processing and user should be notified. As already said, I need to return multiple messages of different importance/type from business logic in my service method and handling exceptions is not what solves the problem here. \$\endgroup\$
    – Anil
    Dec 17, 2013 at 11:29
  • \$\begingroup\$ @Anil - I disagree, if (as in Michael Smith's example) PrimaryBusinessClass.DoWork(parameter) would throw an exception as in your usecase of processing an order you would want to deploy a graceful rollback. If, on the other hand, an order process returns with insufficient products in the warehouse for the shipment or if the order process returns with a invalid delivery address - then that's not exceptions and should be returned as they are, which Michael's example adheres to. It's also rarely needed to return multiples failure messages to the user as you want - just return the first one \$\endgroup\$
    – Max
    Dec 17, 2013 at 11:39
1
\$\begingroup\$

Honestly I'd rather see a validation routine called first and an exception thrown with validation errors in a custom exception type.

That said, I'm also not a fan of out parameters. So #1 over #2.

\$\endgroup\$
3
  • \$\begingroup\$ Hmm... not sure about that. Why? Two reasons: 1. I don't like to use exceptions to control the flow of my business logic. Exceptions are for something exceptional (e.g. couldn't access the database) 2. How would you return an information about success of the operation, or about a warning that didn't prevent the execution, or simply some notification, like "Order will be shipped in 5 days"? \$\endgroup\$
    – Anil
    Dec 17, 2013 at 8:10
  • \$\begingroup\$ I believe has errors is an exceptional case and isn't really flow control. Assuming previous client validations. I would expect the order or Id to be returned on success. Since "will be shipped in 5 days" is not really a concern of saving, I would expect another operation to determine this from the state of the returned order. \$\endgroup\$
    – crad
    Dec 17, 2013 at 8:45
  • \$\begingroup\$ Agreed, shipping info is not something SaveOrder should do, but that is not an important detail now... Imagine a ProcessOrder method which would return a success alert "order is processed" and also some additional notice (info about shipping) and maybe even some warning which says "You are approaching the limit of... something, please consider doing this and that". Anyway I have an impression that we are loosing focus here. As already said, I need to return multiple messages of different importance/type from my service method and handling exceptions is not what solves my problem here. \$\endgroup\$
    – Anil
    Dec 17, 2013 at 11:25
1
\$\begingroup\$

SaveOrder2 is better in this case.

We use out parameter when we try to do something and return value indicates whether the method succeeded or not, so we don't have to make null checks, etc. on the variables returned/set by the method.

Also, the code is relatively clean and more readable.

var orderID = service.SaveOrder(order, out alerts);

if(orderID.HasValue)
{
   Console.WriteLine(orderID);
}
else
{
   ShowAlerts(alerts);
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer. One issue with this approach is that alerts are not passed to caller/user if everything is fine, and alerts in that case contain one or more messages for the user (e.g. one success message, and one notice about moving customer to new discount group due to his Xth order). So basically by alerts I don't assume just errors, but also success statuses and different notices and warnings that did not prevent successful execution of the method. \$\endgroup\$
    – Anil
    Dec 18, 2013 at 12:25
  • \$\begingroup\$ It is just about the structure of the code. We can execute ShowAlerts(alerts) in both if and else block. Also, if there is some specific code that we have to execute either in case of success or failure, we can write that in the corresponding block. \$\endgroup\$ Dec 18, 2013 at 13:30

Your Answer

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

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