2

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.

  • I've just returned void when doing it this way. The exception lets you know something bad happened and what that was. It is not my preferred way of handling this situation, but it is the standard/well-known way in C#. – Kasey Speakman May 25 '17 at 21:59
  • 2
    <grammar nazi>You don't return void. You declare a void return signature.</grammar nazi> – candied_orange May 26 '17 at 1:14
  • @CandiedOrange: That's a type system wart in C#, more modern languages tend to have something like Void or Unit as a first-class type with only one value that's implicitly returned where appropriate. – Phoshi May 26 '17 at 8:16
  • @Phoshi if "something like void" is null and "unit" is the maybe monad/collection. Otherwise I've never heard of your "modern languages". I have heard of c# though where declaring a void return signature means you don't return anything, not even null. Means the method isn't a function but a subroutine. It's result can't be assigned to a variable. – candied_orange May 26 '17 at 14:20
  • @CandiedOrange: You will find an explicit "Void" or "Unit" type in: Haskell, rust, F#, Scala, Common Lisp, Python, Swift, Go, C++11, and others. It is mostly nice for making generic programming better - C# has Func and Action because of this wart, and any method accepting a Func or Action often needs another overload accepting the other. – Phoshi May 26 '17 at 15:26
8

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.

  • An acceptable exception to this rule is the try-do pattern where you don't want to throw an exception. – t3chb0t May 26 '17 at 18:07
1

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.

  • That's a good point about people ignoring return values. Sure, an exception can be caught and ignored too but I think the idea of an exception being thrown suggests that something happened that wasn't expected - such as the API being down or the shipment failing for some other reason. – Lock May 27 '17 at 2:34
  • @Lock Yes, exceptions can be caught and ignored. The primary difference is that the person calling the method with a return code needs to write code to handle success or failure, while the person calling the method needs to do nothing. If it works the next line will be executed but if it fails the exception continues. It takes work to ignore exceptions, but no work to ignore return codes. That's a good point and really at the heart of my answer. – Andy May 27 '17 at 13:27
  • @Lock However, an API being down unexpectedly doesn't really factor into the choice of throwing an exception or not. Its a design choice you make when you create the method. You may have an UpdateCache method that calls the API, and your design depends on the cache being updated to succeed. In that case, your method might throw if the API is down. But it could also be that your cache is a cache and for your design its ok if you use cached data which is a bit stale. In that case UpdateCache might try the API if it fails the caller won't know, because that's how the method was designed. – Andy May 27 '17 at 13:31
  • @Lock So it really boils down to what decision did the programmer make when they created the methods contract. Hopefully the decision is documented somewhere too. :-) – Andy May 27 '17 at 13:32
  • 1
    @Andy Hopefully the decision isn't just documented somewhere but where you can actually find it. – gnasher729 May 27 '17 at 19:17
0

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.

0

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);
        }
    }
  • How is a 3rd party's service being down not exceptional if your process depends on it? – Friek May 26 '17 at 18:31
  • Because "exceptional" things, to me, are events that you do not reasonably expect to happen. You should "expect" any 3rd party service to be unavailable from time to time, and your code should return a proper type that represents this. To clarify, my approach is sort of the opposite of Exception Driven Development, popular in some languages and frameworks (Python, Java, etc). – Graham May 26 '17 at 20:42
  • To me, I expect the sending fof the shipment to succeed. When it doesn't happen, something must be wrong such as the endpoint is down or I've provided incorrect data. To me, those are exceptional circumstances. I like your approach but I don't see where or why you would ever raise exceptions then? In your approach, what do you consider an exceptional circumstance? – Lock May 27 '17 at 2:31
  • You should "expect" any 3rd party service to be unavailable from time to time, and your code should return a proper type that represents this. then you fall in one of the Fallacies of the distributed computing – Laiv May 27 '17 at 10:18
  • @Lock Basically I've stopped the practice of throwing-then-catching my own exceptions. What is the purpose of doing if (condition) throw new ShoesNotTiedException(), then catching for the ShoesNotTiedException later on? Why use an exception for that? You get no benefit from the things an exception brings to the party (stack trace, etc). – Graham May 30 '17 at 12:59
0

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

  • I'm not entirely sure I'm understand what your point of view here is. – Lock May 27 '17 at 2:33

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.