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.

Improve this question
11
  • 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#. Commented May 25, 2017 at 21:59
  • 2
    <grammar nazi>You don't return void. You declare a void return signature.</grammar nazi> Commented May 26, 2017 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. Commented May 26, 2017 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. Commented May 26, 2017 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. Commented May 26, 2017 at 15:26

5 Answers 5

Reset to default
9

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.

Improve this answer
1
  • An acceptable exception to this rule is the try-do pattern where you don't want to throw an exception. Commented May 26, 2017 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.

Improve this answer
6
  • 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. Commented May 27, 2017 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. Commented May 27, 2017 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. Commented May 27, 2017 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. :-) Commented May 27, 2017 at 13:32
  • 1
    @Andy Hopefully the decision isn't just documented somewhere but where you can actually find it. Commented May 27, 2017 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.

Improve this answer
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); } }
Improve this answer
7
  • How is a 3rd party's service being down not exceptional if your process depends on it? Commented May 26, 2017 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). Commented May 26, 2017 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? Commented May 27, 2017 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 Commented May 27, 2017 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). Commented May 30, 2017 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

Improve this answer
1
  • I'm not entirely sure I'm understand what your point of view here is. Commented May 27, 2017 at 2:33

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.