Should my method return void or a bool to indicate it was successful?

Question

I am integrating an ERP with a 3rd party reseller. When we finish processing orders that came from the 3rd party, we send back 2 API calls:

1. Set the status of the order to ReadyToShip whereby we pass shipment information.
2. Set the status of the order to Shipped.

I am unsure what the return type of this method should be. I am using exceptions to notify the calling code that there was a problem in setting the status of these orders. Because of this, should my method ExportShipments simply return void? Or should I not being returning void, but perhaps a bool or even a custom return object?

My code looks like this, currently using bool, however I think the use of the bool is not really doing anything here as its really never going to return false (because it will raise an exception):

    public bool ExportShipments(int iconicOrderId, string shippingProvider, string trackingNumber)
    {
        TheIconicModels.Order order = orderRepository.GetOrderById(iconicOrderId);
        bool setStatusToReadyToShipResult = true, setStatusToShippedResult = true;
        if ( order.Statuses.Status == OrderStatusConstants.PENDING)
        {
            setStatusToReadyToShipResult = orderRepository.SetStatusToReadyToShip(order, shippingProvider, trackingNumber);
        }
        if ( order.Statuses.Status == OrderStatusConstants.READY_TOS_SHIP)
        {
            setStatusToShippedResult = orderRepository.SetStatusToShipped(order);
        }

        return setStatusToReadyToShipResult && setStatusToShippedResult;
    }

    public bool SetStatusToReadyToShip(Order order, string shippingProvider, string trackingNumber)
    {
        var parameters = new List<AbstractParam>() {
            new OrderItemsParam(order),
            new ShippingProviderParam(shippingProvider),
            new TrackingNumberParam(trackingNumber),
            new DeliveryTypeParam("dropship")
        };

        TheIconicApiResult result = this.apiService.SendPostRequest("SetStatusToReadyToShip", parameters, String.Empty);
        var jsonResponse = JsonConvert.DeserializeObject<RootObject>(result.ResponseBody);

        return jsonResponse.SuccessResponse != null;
    }

I need to know if the operation was successful as at a higher level, I mark this shipment as sent.

Ideally, what should my method be returning? I'm thinking void and just catching any exceptions from a higher level but would appreciate some advice.

Answer 1

I would also like to mention the idea of Command Query Separation

Ideally,

• Queries return a result (and don't have side-effects)
• Commands change the state of a system.

As it seems to me that your methods are essentially commands, I would have them have a void return signature and raise exceptions on errors.

Than again, even if your queries encounter errors, they too should raise exceptions and not "return" something indicating an error. Programmers sometime use null to signify that a query failed, but I think that is ill advised.

Answer 2

The method exists to do something. That's its contract. If it fails to do whatever it was expected to do, an exception should be thrown. The issue with return codes is that people can ignore them, which silently hides issues with the execution of the program.

Answer 3

If you expect failures to happen often, as a matter of normal business, then a return value might be an OK approach. But if failures are an exception, just throw an exception. That's what they're meant for.

Answer 4

I've come to dislike using exceptions for non-exceptional events. Failing business validations is not exceptional. A 3rd party API being down is not exceptional. Your own database being unavailable is PROBABLY exceptional, unless you anticipated that it might be offline from time to time. As such, I like to use a generically-typed return/result class object to communicate success/error for most calls be layers in apps.

public class CallResult
    {
        public bool Success { get; set; }
        public ValidationResult[] TransactionErrors { get; private set; }
        public string[] SuccessMessages { get; private set; } // in case you need to signal a partial success, or a custom result message

        public CallResult()
        {
            TransactionErrors = new ValidationResult[0];
            SuccessMessages = new string[0];
            Success = true; // Easier to let it be 'true' by default, and 'false' only when you add an error or set it manually!
        }

        public void AddTransactionError(string errorMessage, string[] memberNames = null)
        {
            var newError = new ValidationResult(errorMessage, memberNames ?? new[] { "" });
            TransactionErrors = TransactionErrors.Concat(new[] { newError }).ToArray();
            Success = false;
        }

        public void MergeWithErrorsFrom(CallResult transaction)
        {
            if (transaction == null || !transaction.TransactionErrors.Any())
                return;

            SuccessMessages = SuccessMessages.Concat(transaction.SuccessMessages).ToArray();
            TransactionErrors = TransactionErrors.Concat(transaction.TransactionErrors).ToArray();
            if (TransactionErrors.Any())
                Success = false;
        }

        public void AddSuccessMessage(string message)
        {
            SuccessMessages = SuccessMessages.Concat(new[] { message }).ToArray();
        }
    }

    public class CallResult<T> : CallResult
    {
        public T ResultObject { get; set; }

        public void MergeWithResultsFrom(CallResult<T> transaction)
        {
            MergeWithErrorsFrom(transaction);
            ResultObject = transaction.ResultObject;
        }

        public CallResult<T> AttemptLoad(Func<T> loadFunc, string emptyLoadMessage)
        {
            ResultObject = loadFunc();
            if (ResultObject == null)
                AddTransactionError(emptyLoadMessage);
            return this;
        }

        public static CallResult<T> Attempt(Func<T> loadFunc, string emptyLoadMessage)
        {
            return (new CallResult<T>()).AttemptLoad(loadFunc, emptyLoadMessage);
        }
    }

Answer 5

I am using exceptions to notify the calling code that there was a problem in setting the status of these orders.

First to rephrase, slightly, what @Graham said

Do not use exceptions for flow control.

---

You are conflating the meaning of the bool and the exception. Doing this sets up future-you for a world of hurt.

The bool says the order shipped or not - true or false. The exception says the database fetch failed. One is not the other. Sure if the fetch blows up the order won't get shipped. That is a cause and effect relation if anything. But they are not the same thing.

Always define all of your states. true and false. You will suffer from code that does not completely define things. I'm dealing with a system now that defines 2 state variables as "yes" and blank. Do this kind of crap and you'll always be dealing with inconsistent, slightly twisted code to deal with "non domain values".

Someday you will need to handle that exception for what it really is and suddenly discover that you must virtually re-write your shipping state handling code.

---

edit

I'm not entirely sure I'm understand what your point of view here is

Return a bool. Exceptions are for problems that will not allow the program to execute or should not continue executing. Catch exceptions to enable graceful program failure.

I need to know if the operation was successful as at a higher level, I mark this shipment as sent. ... its really never going to return false (because it will raise an exception) ... Because of this, should my method ExportShipments simply return void?

What shipping clerk anywhere, when asked "did it ship?" Replies, I got a databaseNotFound exception. It either shipped - true, or it did not ship - false.

So you assume there will never be a "not shipped" status. Why do you even bother calling the method? Why don't you just make a call that checks for a database connection?

So with a return void + a potential exception, the code design means to say "if the database messed up somehow, it did not ship." A problem with the database is a technical implementation issue which has absolutely nothing do to with the shipping business. You should be coding in terms of the business, which is shipping not calling databases.

end edit