24

I often come across methods/functions which have an additional boolean parameter which controls whether an exception is thrown on failure, or null is returned.

There are already discussions about which of those is the better choice in which case, so let's not focus on this here. See e.g. Return magic value, throw exception or return false on failure?

Let us instead assume that there is a good reason why we want to support both ways.

Personally I think such a method should rather be split in two: One that throws an exception on failure, the other that returns null on failure.

So, which is better?

A: One method with $exception_on_failure parameter.

/**
 * @param int $id
 * @param bool $exception_on_failure
 *
 * @return Item|null
 *   The item, or null if not found and $exception_on_failure is false.
 * @throws NoSuchItemException
 *   Thrown if item not found, and $exception_on_failure is true.
 */
function loadItem(int $id, bool $exception_on_failure): ?Item;

B: Two distinct methods.

/**
 * @param int $id
 *
 * @return Item|null
 *   The item, or null if not found.
 */
function loadItemOrNull(int $id): ?Item;

/**
 * @param int $id
 *
 * @return Item
 *   The item, if found (exception otherwise).
 *
 * @throws NoSuchItemException
 *   Thrown if item not found.
 */
function loadItem(int $id): Item;

EDIT: C: Something else?

A lot of people have suggested other options, or claim that both A and B are flawed. Such suggestions or opinions are welcome, relevant and useful. A complete answer can contain such extra information, but will also address the main question of whether a parameter to change the signature/behavior is a good idea.

Notes

In case someone is wondering: The examples are in PHP. But I think the question applies across languages as long as they are somewhat similar to PHP or Java.

donquixote
  • 857
  • 7
  • 13
  • 5
    While I have to work in PHP and am quite proficient, its simply a bad language, and its standard library is all over the place. Read https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/ for reference. So its not really surprising that some PHP developers adopt bad habits. But I have not seen this particular design smell, yet. – Polygnome Dec 23 '17 at 17:43
  • I am missing an essential point in the answers given: when you would use one or the other. You would use the exception method in case an item not being found would be unexpected and considered an error (time to panic). You would use the Try... method in case a missing item is not unlikely and certainly not an error but just a possibility you include in your normal flow control. – Martin Maat Dec 23 '17 at 17:43
  • Related: https://stackoverflow.com/questions/34439782/why-not-try-methods-everywhere/34440727#34440727 – Martin Maat Dec 23 '17 at 18:49
  • 1
    @Polygnome You are right, standard library functions are all over the place, and a lot of poor code exists, and there is the one-process-per-request problem. But it is getting better with each version. The types and object model does all or most of the things that one can expect from it. I want to see generics, co- and contravariance, callback types that specify a signature, etc. A lot of this is being discussed or on the way to be implemented. I think the overall direction is good, even though the standard library quirks will not go away easily. – donquixote Dec 23 '17 at 21:47
  • Bool parameters are bad. – el.pescado - нет войне Dec 23 '17 at 22:34
  • 4
    One suggestion: instead of `loadItemOrNull(id)`, consider whether `loadItemOr(id, defaultItem)` makes sense for you. If item is a String or number it often does. – user949300 Dec 24 '17 at 01:40
  • @user949300 I have done this sometimes. In PHP, one can pass in a `new \stdClass()` to get a unique value to compare against. – donquixote Dec 24 '17 at 09:24

10 Answers10

35

You're correct: two methods are much better for that, for several reasons:

  1. In Java, the signature of the method which potentially throws an exception will include this exception; the other method won't. It makes it particularly clear what to expect from the one and the other.

  2. In languages such as C# where the signature of the method tells nothing about the exceptions, the public methods should still be documented, and such documentation includes the exceptions. Documenting a single method would not be easy.

    Your example is perfect: the comments in the second piece of code look much clearer, and I would even short “ The item, if found (exception otherwise).” down to “ The item.”—the presence of a potential exception and the description you gave to it are self-explanatory.

  3. In a case of a single method, there are few cases where you would like to toggle the value of the boolean parameter on runtime, and if you do, it would mean that the caller will have to handle both cases (a null response and the exception), making the code much more difficult than it needs to be. Since the choice is made not at runtime, but when writing the code, two methods make perfect sense.

  4. Some frameworks, such as .NET Framework, have established conventions for this situation, and they solve it with two methods, just like you suggested. The only difference is that they use a pattern explained by Ewan in his answer, so int.Parse(string): int throws an exception, while int.TryParse(string, out int): bool doesn't. This naming convention is very strong in .NET's community and should be followed whenever the code matches the situation you describe.

Arseni Mourzenko
  • 134,780
  • 31
  • 343
  • 513
  • 1
    Thank you, this is the answer I was looking for! The purpose was I started a discussion about this for Drupal CMS, https://www.drupal.org/project/drupal/issues/2932772, and I don't want this to be about my personal preference, but back it up with feedback from others. – donquixote Dec 23 '17 at 12:24
  • 4
    Actually long standing tradition for at least .NET is to NOT use two methods, but instead pick the right choice for the right task. Exceptions are for exceptional circumstances, not to handle expected control flow. Failing to parse a string into an integer? Not a very unexpected or exceptional circumstance. `int.Parse()` is considered a really bad design decision, which is exactly why the .NET team went ahead and implement `TryParse`. – Voo Dec 23 '17 at 17:07
  • 1
    Providing two methods instead of thinking about what kind of use case we're dealing with, is the easy way out: Don't think about what you're doing, instead put the responsibility on all the users of your API. Sure it works, but that's not the hallmark of good API design (or any design for the matter. Read e.g. [Joel's take on this](https://www.joelonsoftware.com/2000/04/12/choices/). – Voo Dec 23 '17 at 17:10
  • @Voo Valid point. Indeed providing two methods puts the choice on the caller. Perhaps for some parameter values an item is expected to exist, while for other parameter values a non-existing item is considered a regular case. Perhaps the component is used in one application where the items are always expected to exist, and in another application where non-existing items are considered normal. Perhaps the caller calls `->itemExists($id)` before calling `->loadItem($id)`. Or perhaps it is just a choice made by other people in the past, and we have to take it for granted. – donquixote Dec 23 '17 at 21:32
  • @Voo I still think that, while worth mentioning, it is an answer to a separate question. See https://softwareengineering.meta.stackexchange.com/questions/8674/how-can-i-avoid-smarter-than-you-answers – donquixote Dec 23 '17 at 21:39
  • @donquixote In my opinion particularly on SE the focus shouldn't be on "how can I make this work", but "what's the best approach to solving the actual problem?" (I consider this an X-Y problem). My intention isn't to say that you should never have two methods, but that you should first think long and hard about the intention of the API and how it's generally being used. The situation where after long deliberation you really think you need both methods should be exceedingly rare.. I can't think of a single example in both Java or .NET from the top of my head. – Voo Dec 23 '17 at 22:13
  • See [e.g. this](https://blogs.msdn.microsoft.com/ericlippert/2008/09/10/vexing-exceptions/) on the topic. And even if we accepted the fact that we really, really want two methods, there can't be one single answer to this, since it depends at least on your language and its idioms. C# has very different approaches to exceptions than say Go does. – Voo Dec 23 '17 at 22:15
  • @Voo I think a good answer has both. It addresses the original question and also says why it might be the wrong question. E.g. "should i use a dirty needle or a clean needle for my heroin?". A complete answer should be "don't do heroin. but if you do, use a clean needle." Each of those pieces of information is essential for a part of the audience. – donquixote Dec 23 '17 at 22:24
  • @doquixote With that I can agree. That's why I put a comment under this answer instead of writing another one that would duplicate the first half. I like being lazy and not repeat what others have already explained perfectly fine :-) – Voo Dec 23 '17 at 22:26
  • Strongly disagree with not noting the behavior when the item is found. I shouldn't have to look down to the exception section to find that. It should be right in the main description. – jpmc26 Dec 24 '17 at 06:01
  • @Voo You're wrong about .NET. Consider the enumerable methods like `Single` vs. `SingleOrDefault`. .NET lets you decide whether you need an error or not and call the appropriate methods. – jpmc26 Dec 24 '17 at 06:02
  • 1
    Reason 1b: Some languages (Haskell, Swift) require nullability to be present in the signature. Specifically, Haskell has an entirely different type for nullable values (`data Maybe a = Just a | Nothing`). – John Dvorak Dec 24 '17 at 20:15
9

An interesting variation is the Swift language. In Swift, you declare a function as "throwing", that is it is allowed to throw errors (similar but not quite the same as say Java exceptions). The caller can call this function in four ways:

a. In a try/catch statement catching and handling the exception.

b. If the caller is itself declared as throwing, just call it, and any error thrown turns into an error thrown by the caller.

c. Marking the function call with try! which means "I'm sure this call isn't going to throw". If the called function does throw, the application is guaranteed to crash.

d. Marking the function call with try? which means "I know the function can throw but I don't care about which error exactly is thrown". This changes the return value of the function from type "T" to type "optional<T>", and an exception thrown is turned into returning nil.

(a) and (d) are the cases you are asking about. What if the function can return nil and can throw exceptions? In that case the type of the return value would be "optional<R>" for some type R, and (d) would change this to "optional<optional<R>>" which is a bit cryptic and hard to understand but completely fine. And a Swift function can return optional<Void>, so (d) can be used for throwing functions returning Void.

gnasher729
  • 42,090
  • 4
  • 59
  • 119
2

I like the bool TryMethodName(out returnValue) approach.

It gives you a conclusive indication of whether the method succeeded or not and the naming matches wrapping the exception throwing method in a try catch block.

If you just return null, you don't know if it failed or the return value was legitimately null.

eg:

//throws exceptions when error occurs
Item LoadItem(int id);

//returns false when error occurs
bool TryLoadItem(int id, out Item item)

example usage (out means pass by reference uninitialised)

Item i;
if(TryLoadItem(3, i))
{
    Print(i.Name);
}
else
{
    Print("unable to load item :(");
}
Ewan
  • 70,664
  • 5
  • 76
  • 161
  • Sorry, you lost me here. How would your "`bool TryMethodName(out returnValue)` approach" look in the original example? – donquixote Dec 23 '17 at 11:55
  • edited to add an example – Ewan Dec 23 '17 at 11:58
  • 1
    So you use a by-reference parameter instead of a return value, so you have the return value free for the bool success? Is there a programming language that supports this "out" keyword? – donquixote Dec 23 '17 at 12:01
  • 2
    yes, sorry didnt realise 'out' is specific to c# – Ewan Dec 23 '17 at 12:07
  • What will be the value of `item` on failure? It will be `null`, right? So `item === null` exactly if the return value is `false`, right? So we don't really gain much with this construction, or do we? – donquixote Dec 23 '17 at 12:14
  • 1
    not always no. But what if item 3 _is_ null? you wouldnt know if there had been an error or not – Ewan Dec 23 '17 at 12:15
  • But back to the actual question: Assuming you want to support both behaviors, the LoadItem() with exception and the TryLoadItem with bool return value (whether this is a good idea or not). Would you rather have those two methods, or have one method with a parameter that determines whether an exception shall be thrown? – donquixote Dec 23 '17 at 12:16
  • Id rather have two methods. the parameter approach doesnt solve the problem when you return value may be null – Ewan Dec 23 '17 at 12:17
  • So you want to be able to distinguish "item does not exist" vs "internal failure". if item is not found then the method will return true, but the item will be null. Right? I think this is reasonable. But somehow misses the point of the question, I would say. – donquixote Dec 23 '17 at 12:19
  • well, the problem with the example B is it doesnt really give any advantage over A (unless its java as in other answer) But a two method approach in general allows this better solution – Ewan Dec 23 '17 at 12:23
  • Your input is valuable (+1), but the other answer addresses the core of the question, so I accept that one. Whether "failure" and "not found" need distinct behavior is a discussion worthwhile to have, but independent of the question whether one should use one or two methods. Maybe this question would better fit here, https://softwareengineering.stackexchange.com/questions/159096/return-magic-value-throw-exception-or-return-false-on-failure?noredirect=1&lq=1 – donquixote Dec 23 '17 at 12:27
  • "well, the problem with the example B is it doesnt really give any advantage over A" -> this was the point exactly, I wanted A and B to be equivalent except for whether it is one or two methods, so that the only reason to pick one or the other is whether it is one or two methods. – donquixote Dec 23 '17 at 12:35
  • oook. but that does seem to artificially limit the question. you could add TryLoadModule to drupal just as easily – Ewan Dec 23 '17 at 12:50
  • Yes we could. But whether this is a good idea can be discussed separately from whether to have one or two methods, and also depends a lot on the use case. Divide and conquer! – donquixote Dec 23 '17 at 12:59
  • "Divide and conquer" as in: one decision at a time. – donquixote Dec 23 '17 at 12:59
  • This is how we do it in Delphi. (StrToInt and TryStrToInt is just one example.) – Andreas Rejbrand Dec 23 '17 at 15:42
  • One more question: An alternative to the TryLoadItem would be to return null if the item does not exist, but throw an exception if there is an actual failure. (This would still be more like B in my original example, because A would mean that an exception is thrown even if the item does not exist, and no failure occured.) The argument would be that if an error occured, we want to know more about it, and a simple value of FALSE does not tell us much. Also this would allow to work with regular return value instead of by-reference parameter. – donquixote Dec 23 '17 at 16:52
  • I assumed that was what you meant. but you dont distinguish between 'not exists' and 'exists but value is null' – Ewan Dec 23 '17 at 16:57
  • Ok, if we are looking for some kind of entity or database row, then there is no meaningful value "null", only "not found" / "does not exist". – donquixote Dec 23 '17 at 21:00
  • if youre that specific then you don't need to throw an exception at all. In general you have to account for a null value return – Ewan Dec 23 '17 at 21:24
2

Usually a boolean parameter indicates a function can be split into two, and as a rule you should always split it up rather than pass in a boolean.

Max
  • 267
  • 2
  • 8
  • 1
    I wouldn't except this as a rule without adding some qualification or nuance. Removing a boolean as argument may force you to write a dumb if/else in some other place and thus you encode data in your written code and not in variables. – Gerard Dec 24 '17 at 12:40
  • @Gerard can you give me an example please? – Max Dec 24 '17 at 12:41
  • @Max When you have a boolean variable `b`: Instead of calling `foo(…, b);` you have to write `if (b) then foo_x(…) else foo_y(…);` and repeating the other arguments, or something like `((b) ? foo_x : foo_y)(…)` if the language has a ternary operator and first class functions/methods. And this maybe even repeated from several different places in the program. – BlackJack Dec 24 '17 at 18:16
  • @BlackJack well yeah I might agree with you. I thought of the example, `if (myGrandmaMadeCookies) eatThem() else makeFood()` which yeah I might wrap in another function like this `checkIfShouldCook(myGrandmaMadeCookies)`. I wonder if the nuance you mentioned has to do with whether we're passing in a boolean on how we want the function to behave, as in the original question, vs. we're passing in some info about our model, as in the grandma example I just gave. – Max Dec 24 '17 at 18:27
  • 1
    The nuance referred to in my comment refers to "you should always". Maybe rephrase it as "if a, b and c are applicable then [...]". The "rule" you mention is great a great paradigm, but very variant on the use-case. – Gerard Dec 25 '17 at 20:14
1

Clean Code advises to avoid boolean flag arguments, because their meaning is opaque at the call site:

loadItem(id, true) // does this throw or not? We need to check the docs ...

whereas separate methods can use expressive names:

loadItemOrNull(id); // meaning is obvious; no need to refer to docs
meriton
  • 4,022
  • 17
  • 18
1

If I had to use either A or B then I would use B, two separate methods, for the reasons stated in Arseni Mourzenkos answer.

But there is another method1: In Java it is called Optional, but you can apply the same concept to other languages where you can define your own types, if the language does not already provide a similar class.

You define your method like this:

Optional<Item> loadItem(int id);

In your method you either return Optional.of(item) if you found the item or you return Optional.empty() in case you don't. You never throw2 and never return null.

For the client of your method it is something like null, but with the big difference that it forces to think about the case of missing items. The user cannot simply ignore the fact that there may be no result. To get the item out of the Optional an explicit action is required:

  • loadItem(1).get() will throw a NoSuchElementException or return the found item.
  • There is also loadItem(1).orElseThrow(() -> new MyException("No item 1")) to use a custom exception if the returned Optional is empty.
  • loadItem(1).orElse(defaultItem) returns either the found item or the passed defaultItem (which may also be null) without throwing an exception.
  • loadItem(1).map(Item::getName) would again return an Optional with the items name, if present.
  • There are some more methods, see the Javadoc for Optional.

The advantage is that now it is up to the client code what should happen if there is no item and still you just need to provide a single method.

1 I guess it is something like the try? in gnasher729s answer but it is not entirely clear to me as I don't know Swift and it seems to be a language feature so it is specific to Swift.

2 You can throw exceptions for other reasons, e.g. if you don't know whether there is an item or not because you had problems communicating with the database.

siegi
  • 292
  • 1
  • 3
  • 6
0

As another point agreeing with using two methods, this can be very easy to implement. I'll write my example in C#, as I don't know PHP's syntax.

public Widget GetWidgetOrNull(int id) {
    // All the lines to get your item, returning null if not found
}

public Widget GetWidget(int id) {
    var widget = GetWidgetOrNull(id);

    if (widget == null) {
        throw new WidgetNotFoundException();
    }

    return widget;
}
krillgar
  • 144
  • 10
0

I prefer two methods, this way, you will not only tell me you are returning a value, but which value exactly.

public int GetValue(); // If you can't get the value, you'll throw me an exception
public int GetValueOrDefault(); // If you can't get the value, you'll return me 0, since it's the default for ints

The TryDoSomething() is not a bad pattern either.

public bool TryParse(string value, out int parsedValue); // If you return me false, I won't bother checking parsedValue.

I'm more used to see the former in database interactions while the latter is more used in parse stuff.

As other have already told you, flag parameters are usually a code smell (code smells dont mean you are doing something wrong, just that there's a probability that you are doing it wrong). Though you name it precisely, there are sometimes nuances that make hard to know how to use that flag and you end up looking inside the method body.

A Bravo Dev
  • 254
  • 1
  • 4
-1

While other people suggest otherwise, I would suggest that having a method with a parameter to indicates how errors should be handled is better than requiring the use of separate methods for the two usage scenarios. While it may be desirable to also have distinct entry points for the "error throws" versus "error returns error indication" usages, having an entry point that can serve both usage patterns will avoid code duplication in wrapper methods. As a simple example, if one has functions:

Thing getThing(string x);
Thing tryGetThing (string x);

and one wants functions that behave likewise but with x wrapped in brackets, it would be necessary to write two sets of wrapper functions:

Thing getThingWithBrackets(string x)
{
  getThing("["+x+"]");
}
Thing tryGetThingWithBrackets (string x);
{
  return tryGetThing("["+x+"]");
}

If there's an argument for how to handle errors, only one wrapper would be needed:

Thing getThingWithBrackets(string x, bool returnNullIfFailure)
{
  return getThing("["+x+"]", returnNullIfFailure);
}

If the wrapper is simple enough, the code duplication might not be too bad, but if it might ever need to change, duplicating the code will in turn require duplicating any changes. Even if one opts to add entry points that don't use the extra argument:

Thing getThingWithBrackets(string x)
{
   return getThingWithBrackets(x, false);
}
Thing tryGetThingWithBrackets(string x)
{
   return tryGetThingWithBrackets(x, true);
}

one can keep all the "business logic" in one method--the one which accepts an argument indicating what to do in case of failure.

supercat
  • 8,335
  • 22
  • 28
  • You are right: Decorators and wrappers, and even regular implementation, will have some code duplication when using two methods. – donquixote Dec 23 '17 at 22:07
  • Surely put the versions with and without exception-throwing in separate packages, and make the wrappers generic? – Will Crawford Dec 24 '17 at 01:40
-1

I think all of the answers which explain that you should not have a flag that controls whether an exception is thrown or not are good. Their advice would be my advice to anyone who feels the need to ask that question.

I did, however, want to point out that there is a meaningful exception to this rule. Once you are advanced enough to start developing APIs that hundreds of other people will use, there are cases where you might want to provide such a flag. When you are writing an API, you're not just writing to the purists. You're writing to real customers who have real desires. Part of your job is to make them happy with your API. That becomes a social problem, rather than a programming problem, and sometimes social problems dictate solutions which would be less than ideal as programming solutions on their own.

You may find that you have two distinct users of your API, one of which wants exceptions and one that does not. A real life example where this could occur is with a library that is used both in development and on an embedded system. In development environments, users will likely want to have exceptions everywhere. Exceptions are very popular for handling unexpected situations. However, they are forbidden in many embedded situations because they're too hard to analyze real-time constraints around. When you actually care about not just the average time it takes to execute your function, but the time it takes for any one individual fun, the idea of unwinding the stack at any arbitrary place is very undesirable.

A real life example for me: a math library. If your math library has a Vector class which supports getting a unit vector in the same direction, you have to be able to divide by the magnitude. If the magnitude is 0, you have a divide by zero situation. In development you want to catch these. You really don't want surprising divide by 0's hanging around. Throwing an exception is a very popular solution for this. It almost never happens (it is truly exceptional behavior), but when it does, you want to know it.

On the embedded platform, you don't want those exceptions. It would make more sense to do a check if (this->mag() == 0) return Vector(0, 0, 0);. In fact, I see this in real code.

Now think from the perspective of a business. You can try to teach two different ways of using an API:

// Development version - exceptions        // Embeded version - return 0s
Vector right = forward.cross(up);          Vector right = forward.cross(up);
Vector localUp = right.cross(forward);     Vector localUp = right.cross(forward);
Vector forwardHat = forward.unit();        Vector forwardHat = forward.tryUnit();
Vector rightHat = right.unit();            Vector rightHat = right.tryUnit();
Vector localUpHat = localUp.unit()         Vector localUpHat = localUp.tryUnit();

This satisfies the opinions of most of the answers here, but from a company perspective this is undesirable. Embedded developers will have to learn one style of coding, and development developers will have to learn another. This can be rather pesky, and forces people into one way of thinking. Potentially worse: how do you go about proving that the embedded version doesn't include any exception throwing code? The throwing version can easily blend in, hiding somewhere in your code until an expensive Failure Review Board finds it.

If, on the other hand, you have a flag which turns on or off exception handling, both groups can learn the exact same style of coding. In fact, in many cases, you can even use one group's code in the other group's projects. In the case of this code using unit(), if you actually care about what happens in a divide by 0 case, you should have written the test yourself. Thus, if you move it to an embedded system, where you just get bad results, that behavior is "sane." Now, from a business perspective, I train my coders once, and they use the same APIs and the same styles in all parts of my business.

In the vast majority of cases, you want to follow the advice of others: use idioms like tryGetUnit() or similar for functions that don't throw exceptions, and instead return a sentinel value like null. If you think users may want to use both exception handling and non-exception handling code side by side within a single program, stick with the tryGetUnit() notation. However, there is a corner case where the reality of business can make it better to use a flag. If you find yourself in that corner case, do not be afraid to toss the "rules" by the wayside. That's what rules are for!

Cort Ammon
  • 10,840
  • 3
  • 23
  • 32