35

I'm building an API, a function that uploads a file. This function will return nothing/void if the file was uploaded correctly and throws an exception when there was some problem.

Why an exception and not just false? Because inside an exception I can specify the reason of failure (no connection, missing filename, wrong password, missing file description, etc.). I wanted to build a custom exception (with some enum to help the API user to handle all the errors).

Is this a good practice or is it better returning an object (with a boolean inside, an optional error message and the enum for errors)?

JimmyJames supports Canada
  • 30,578
  • 3
  • 59
  • 108

5 Answers5

43

Throwing an exception is simply an additional way of making a method return a value. The caller can check for a return value just as easily as catch an exception and check that. Therefore, deciding between throw and return requires other criteria.

Throwing exceptions should often be avoided if it endangers the efficiency of your program (constructing an exception object and unwinding the call stack is much more work for the computer than just pushing a value onto it). But if the purpose of your method is to upload a file, then the bottleneck is always going to be the network and file system I/O, so it's pointless to optimize the return method.

It's also a bad idea to throw exceptions for what should be a simple control flow (e.g. when a search succeeds by finding its value), because that violates the expectations of API users. But a method failing to fulfill its purpose is an exceptional case (or at least it should be), so I see no reason for not throwing an exception. And if you do that, you might just as well make it a custom, more informative exception (but it's a good idea to make it a subclass of a standard, more general exception like IOException).

Kilian Foth
  • 110,899
41

There is absolutely no reason for returning true on success if you don't return false on failure. What should the client code look like?

if (result = tryMyAPICall()) {
    // business logic
}
else {
    // this will *never* happen anyways
}

In this case, the caller needs a try-catch block anyways, but then they can better write:

try {
    result = tryMyAPICall();
    // business logic
    // will only reach this line when no exception
    // no reason to use an if-condition
} catch (SomeException se) { }

So the true return value is completely irelevant for the caller. So just keep the method void.


In general, there are three ways to design failure modes.

  1. Return true/false
  2. Use void, throw (checked) exception
  3. Return a intermediate result object (or a general purpose ADT for this use-case, like Result<S, E> = Success<S> | Fail<E>, if your language supports this. Java e.g. will be able to do this once we get sealed types & records).

Return true/false

This is used in some older, mostly c-style APIs. The downsides are obvious, you have no idea what went wrong. PHP does this quite often, leading to code like this:

if (xyz_parse($data) === FALSE)
   $error = xyz_last_error();

In multi-threaded contexts, this is even worse.

Throw (checked) exceptions

This is a nice way to do it. At some points, you can expect failure. Java does this with sockets. The basic assumption is that a call should succeed, but everyone knows that certain operations might fail. Socket connections are among them. So the caller gets forced to handle the failure. its a nice design, because it makes sure the caller actually handles the failure, and gives the caller an elegant way to deal with the failure.

Return result object

This is another nice way to handle this. Its often used for parsing or simply things that need to get validated.

ValidationResult result = parser.validate(data);
if (result.isValid())
    // business logic
else
    error = result.getvalidationError();

Nice, clean logic for the caller as well.

Using an ADT

With an ADT, the above code would look like this (yes, even in java, once sealed types & full type patters for switch are implemented):

var result = parser.parse(data);
switch (result) {
    case Success (MyDocument document) -> process(document);
    case Fail (ValidationError error) -> showError(error);
}

Which also has a nice logic and a nice control flow to it.


There is a bit of debate when to use the second case, and when to use the third. Some people believe that exceptions should be exceptional and that you should not design with the possibility of exceptions in mind, and will pretty much always use the third options. That is fine. But we have checked exceptions in Java, so I see no reason not to use them. I use checked exceptions when the basic assumption is that the call should succeed (like using a socket), but failure is possible, and I use the third option when its very unclear whether the call should succeed (like validating data). But there are different opinions on this.

In your case, I would go with void + Exception. You expect the file upload to succeed, and when it does not, that is exceptional. But the caller is forced to handle that failure mode, and you can return an exception that properly describes what kind of error occurred.

Polygnome
  • 2,071
  • 15
  • 16
8

This really comes down to whether a failure is exceptional or expected.

If an error isn't something you generally expect, then it's reasonably likely the API user will just call your method without any special error handling. Throwing an exception allows that to bubble up the stack to a place where it gets noticed.

If on the other hand errors are commonplace, you should optimize for the developer who is checking for them, and try-catch clauses are a bit more cumbersome than an if/elif or switch series.

Always design an API in the mindset of someone who is using it.

6

If your method has a return value, throwing an exception might surprise its users. If I see a method return a boolean, I am pretty convinced it will return true if it succeeds and false if it doesn't, and I will structure my code using an if-else clause. If your exception is going to be based on an enum anyway, you may as well return an enum value instead, similar to windows HResults, and keep an enum value for when the method succeeds.

It is also annoying to have a method throw exception when it is not the final step in a procedure. Having to write a try-catch and divert control flow to the catch block is a good recipe for spaghetti, and should be avoided.

If you are going forward with exceptions, try returning void instead, users will figure out it succeed if no exceptions are thrown, and none will try to use an if-else clause to divert control flow.

5

Don't return a boolean if you never intend to return false. Just make the method void and document that it will throw an exception (IOException is suitable) on failure.

The reason for this is that if you do return a boolean, the user of your API may conclude that he/she can do this:

if (fileUpload(file) == false) {
    // Handle failure.
}

That will not work of course; that is, there is an incongruity between your method's contract and its behaviour. If you throw a checked exception, the user of the method has to handle failure:

try {
    fileUpload(file);
} catch (IOException e) {
    // Handle failure.
}