Should I raise an exception/error when an optional argument is used but is not necessary?

Question

Take this constructed example:

def fetch_person(person_id, country_code=None):
    if is_fully_qualified(person_id):
        return person_source_1.fetch_person(person_id)
    else:
        return person_source_2.fetch_person(person_id, country_code)

If the function caller supplies the right kind of person_id the country_code is not necessary. Should I be guarding against the use in that case, like this:

def fetch_person(person_id, country_code=None):
    if is_fully_qualified(person_id):
        if country_code:
            raise Exception("The country_code argument is not nessary if person_id is fully qualified.")
        return person_source_1.fetch_person(person_id)
    else:
        return person_source_2.fetch_person(person_id, country_code)

I feel the user might be confused about the business logic if they supply the country code.

---

There seems to be some confusions about the question, maybe because of the nature of the example. Here is a simpler example:

def area(shape, x, y=None):
    if shape == 'square':
        return x ** 2
    elif shape == 'rectangle':
        return x * y
    elif shape == 'circle':
        return 3.14 * (x ** 2)
    else:
        raise ShapeNotSupportedException()

I can imagine three possible exceptions here. An exception in the square branch if y is given at all. An exception in the square branch if x != y. An exception in the circle branch if y is given at all, because circles don't have a second dimension to be defined.

Here is the one for just refusing y:

def area(shape, x, y=None):
    if shape == 'square':
        if y is not None:
            raise InvalidArgument()
        return x ** 2
    elif shape == 'rectangle':
        return x * y
    elif shape == 'circle':
        if y is not None:
            raise InvalidArgument()
        return 3.14 * (x ** 2)
    else:
        raise ShapeNotSupportedException()

Here is one where we are more lenient for squares, and only check if the argument makes sense or not:

def area(shape, x, y=None):
    if shape == 'square':
        if y is not None:
            if y != x:
                raise InvalidArgument()
        return x ** 2
    elif shape == 'rectangle':
        return x * y
    elif shape == 'circle':
        if y is not None:
            raise InvalidArgument()
        return 3.14 * (x ** 2)
    else:
        raise ShapeNotSupportedException()

The alternative is of course just to ignore y if it isn't needed.

Answer 1

Exceptions should be thrown in exceptional situations. If an exception is thrown that could be avoided, that’s a bug on the caller side.

So you force the caller to change their code from

Fetch_person (person_id, country_id)

To

If is_fully_qualified(person_id)
    Fetch_person(person_id)
Else
    Fetch_person(person_id, country_id)

So that is lots of unnecessary code just because you’re keen on throwing exceptions. But wait, it’s worse: if your function throws an exception when the country_id is not needed, then this function should as well, right? Which means we do the whole nonsense again one layer higher!

Plus the caller needs to know internal details of the called function and needs changing if these details change. Which must violate some single-whatever-principles that don’t even have a name because it’s assumed nobody would be stupid enough to violate them.

Answer 2

If the function caller supplies the right kind of person_id the country_code is not necessary.

This implies that a country_code is somehow embedded in a fully-qualified person_id. In the case of a fully-qualified person ID and a non-None country code, I would raise an exception of the two country codes did not match. But if they do match, it's a no harm, no foul situation.

Answer 3

Note the caveat mentioned below.

No, you shouldn't raise an exception.

If you take this approach, this inherently means that which data source you use is an internal implementation detail of the method, which in turn means that the consumer can't (and therefore shouldn't) know which data source you end up using.

It is unnecessary to punish/blame a caller for calling the method without distinguishing which data source will be used, as this distinction is an internal implementation detail.
This is doubly egregious because you're talking about throwing an exception for something that isn't even a problem. Your method is perfectly capable of handling the call as presented; which means an exception is not justified.

The current method structure is at odds with the reasoning for throwing an exception. The latter implies that the caller should be aware of the distinction, but the former only makes sense when the caller doesn't need to be aware of the distinction. The solution here is to either not throw an exception, or to change your method (by splitting it into two).

---

Caveat
I assume that your caller interchangeably uses the two forms of ID, in a way that they cannot easily distinguish one from the other. In this case, your current method makes sense.

If the caller is explicitly aware of which kind of ID they are passing in, then it makes more sense to split this method into two separate methods, which in turn renders the question moot.

Answer 4

I would not throw here. You have a scenario in which the second parameter may be needed, if the caller of this function has the second parameter readily available they very well might simply provide it rather than write code to figure out of they need it or not. That should not be treated as a problem.

The only good reason the caller wouldn't simply provide the second parameter all the time is if it was expensive to obtain--say, they have to query the database to get it.

Answer 5

I think raising an exception defeats the purpose of them being allowed to execute the function with those arguments in the first place.

In my opinion, raising a warning would be better since it wouldn't interrupt their workflow (and adding the country code doesn't break the code), and if they really wanted that warning to go away they'd change it. I also think what you're doing (besides the exception) is okay, because the warning would be describing a better way of executing the function (which may clear up confusion as to why the country code argument is still there if it's not needed, saving the programmer time).

However, I'm also thinking that if you called fetch_person() 1000+ times, they'd get a huge log of warnings. Which means it would be best to supply another argument that would silence those warnings. HOWEVER, that's just clutter in the code and entirely overkill for something so small. So actually, I think it would be best to just document this beforehand (in a comment or on a documentation page) and remove the exception/warning all together.

Answer 6

I personally think that it depends on your use of the country code:

• If you use the country code as a fallback in case when the person ID is not fully qualified, do not throw. I would also suggest renaming the country_code parameter to something like default_country_code, since the country code derived from the person ID overrides the parameter.

• If you do not use it as a fallback and your query is “find me a person with this fully qualified ID in this country” or “find me a person with this (maybe non-fully qualified) ID in this country”, then check whether the country of the person is the same as the country given in the parameter and throw if it is not or return no person records, like if there was not such person (since you want person with that ID and also in that country). Now, your question is: Throw an (programmer's) error or return nothing instead?

Answer 7

Perhaps slightly change the API to make it clear what the role of the second argument is, and that it is not necessarily used:

def fetch_person(person_id, default_country_code=None):
    '''Fetches person by person_id.
    
    For unqualified person_id uses default_country_code as country code.
    '''
    ...

It is always best to design an API in such a way that it is hard to accidentally misuse or misunderstand. Adding the default prefix should make it clear that this value may not be used, so it should prevent the caller from making these 2 wrong guesses:

• that the provided country code may override some other value,
• that the provided country code will be verified.

Of course, that still depends on the context. It may be, that providing qualified person_id and the country code is a likely error (for some reasons resulting from the bussiness logic). In that case, when you think that the caller should really know better whether they are using qualified or unqualified person_id, providing 2 separate functions (as in gnasher729'a answer) seems to be better choice.

def fetch_person_by_qualified_id(person_id):
    # Maybe a warning would be enough, or just not getting a result?
    assert(is_fully_qualified(person_id), 'person_id must be fully qualified')
    return person_source_1.fetch_person(person_id)

def fetch_person_by_country_code(person_id, country_code):
    # Optionally add assertion or warning:
    # assert(not is_fully_qualified(person_id), '...')
    # Or:
    # assert(not is_fully_qualified(person_id) or
    #        get_country_code(person_id) == country_code, '...')
    # Or:
    # if is_fully_qualified(person_id):
    #     logger.warning('...')

    return person_source_2.fetch_person(person_id, country_code)

However, as long as you can go with the first option, it is preferred - coping with unexpected exceptions from API is never nice, and doing the right thing for the user even when they do something unnecessarily (like providing default country_code while the person_id is qualified) is welcome.

Answer 8

TL;DR

Exceptions are for unexpected states, not for expected variances in argument or return values. There also seems to be an implied X/Y question about the necessity of the second "optional" argument, which I address in the last paragraph of my answer as one of handling a collection of arguments that collectively represent an invalid combination.

Raise Only on Unexpected States Such as an Invalid Combination of Required and Optional Parameters

You seem to be using Python, but I'm going to take a more general stance here. There may be languages where this matters, but in a more general sense the whole point of an optional argument is that it's optional! That means that knowing when the argument can be skipped is generally the responsibility of the caller, unless the caller is relying on the callee to set a sensible default value.

If an argument is mandatory, then you should raise an exception when it's not passed. However, if it's optional, then you should either ignore it or use it at your discretion. An optional argument, almost by definition, is not an exceptional situation. It's probably not even an error.

Exceptions should be raised when there's an unexpected state, while errors or warnings should be used when a state is expected but perhaps undesirable. Depending on your language, exceptions are usually more expensive than simple errors or warnings, so you shouldn't use them for likely or expected error conditions without a very good reason.

If the signature of your method matters for some reason, and you don't want to handle the case of optional arguments in the callee, see if your chosen language supports method overloading or a decorator pattern where you can call different methods based on the signature. However, for languages like Python or Ruby, I can't envision a concrete example where passing an optional value matters much unless you have an edge case like "I was passed a valid person_id that didn't need a country code, but received an invalid country_code anyway."

The edge case above is potentially a valid reason to raise an exception, unless it's safe to ignore since the value was optional in the first place. Other than that, though, your problem seems more like a question of object validation (e.g. do "I have a person_id that needs a country_code or not?") rather than about the usefulness or correctness of exception-raising per se. YMMV.

Answer 9

Offer a more specific API for clients who know more

It sounds like you expect your clients to have certain information in some cases, but you don't give them an easy option to use this information. Apparently, you expect your clients to sometimes know that the person they provide is qualified, which means that they don't need to provide a country code.

But if you choose exceptions to deal with this situation, you punish clients who simply don't know whether their person is qualified. Instead, you should provide a more specific API to be used by clients who have more information, and provide a fallback API for those clients who have less. In concrete terms, if a client knows that their person is qualified, they should use an API that does not even allow them to specify a country code. If the client is unsure, they should use a less specific API and provide a fallback country code in case the person is not qualified.

This can be done like this (naming is hard and could be improved here):

def fetch_qualified_person(person_id):
  return person_source_1.fetch_person(person_id)

def fetch_unqualified_person(person_id, country_code):
  return person_source_2.fetch_person(person_id, country_code)

def fetch_unknown_person(person_id, country_code):
  if is_fully_qualified(person_id):
    return fetch_qualified_person(person_id):
  else:
    return fetch_unqualified_person(person_id, country_code)

This gives the power back to the client: If they know a lot, they can use the information they have. If they don't, they have a safe fallback solution. In any case, the clients don't have to worry about their calls "blowing up".

Answer 10

Syntactically, the country code is optional. Semantically, it is required, because the abstraction supplied by fetch_person indicates that the caller cannot know when the country id is not required.

Since the argument is required, that should be reflected by the signature:

def fetch_person(person_id, country_code):
    if is_fully_qualified(person_id):
        return person_source_1.fetch_person(person_id)
    else:
        return person_source_2.fetch_person(person_id, country_code)

---

The dependency inversion principle, however, suggests that having fetch_person both decide how to fetch a person and fetch the person is the wrong design. The caller should instead pass a person ID and a function that takes just a person ID, and fetch_person takes care of applying the function to the ID.

# In this constructed example, dependency injection reduces
# fetch_person to a trivial function applicator. But in a real
# example, there might be more steps involved. We're just converting
# a quasi-optional argument into a required argument that is used
# unconditionally.
def fetch_person(person_id, fetcher):
    return fetcher(person_id)


if is_fully_qualifed(p1):
    f = person_source_1.fetch_person
else:
    # If your example was pseudocode and not Python,
    # use whatever technique you need to make a closure
    # over the correct value of country_code
    def make_closure(cc):
        return lambda p: person_source_2.fetch_person(p, cc)

    f = make_closure(country_code)

fetch_person(p1, f)

Answer 11

Categorically not, and no scenario exists where this could ever be a good thing

Optional arguments are exactly that, optional. That means they can be used or not used, as the user of the library chooses. The user of the library, not the author of the library.

Exceptions always indicate that something has happened which is not on the "happy path" and which the user of the library needs to deal with. This may be normal system behaviour (like a file not existing) or may be unexpected and fatal (like running out of disk space). Regardless, they always indicate that we are not on the "happy path" any more. The very name, "exception", tells you that it is not a normal system response.

The user of the library explicitly specifying an optional parameter is not a deviation from the "happy path". If they have explicitly specified the default value, the library response is, by definition, the same as if they had specified the default value. Therefore, by definition, there should never be an exception.

What happens if you do? Well, you've just stopped that parameter being optional. You have a function call which needs one parameter for one type of user, and two parameters for another type of user. That means you shouldn't have one function, you should have two separate functions for the two purposes. If you think there could be some confusion about the business logic, this is how you must solve it.

Worse than that though, someone needs to handle that exception. You're using an exception for something which no software engineer would ever consider it being used. If you're lucky, a tester will see this and you'll have your code thrown back at you with a "fix this now" note. If you're really unlucky though, your library will be several layers down in the call stack, and the data may only trip this case occasionally. In that case the application will catch the exception at some point much further up, and will then bomb out with a message which is utterly meaningless at the level where it happens.

Answer 12

The other answers do a good job of covering things like API design and the proper use of exceptions, so I won't go into that here.

I think there is still an underlying question of "how do I alert someone that they might have made a mistake during development" that hasn't been addressed. For example, if in your scenario:

1. In some contexts, the caller may not know whether or not the person is "fully qualified" - so they pass both by default (and maybe country_code is None or whatever other sentinel value you use to indicate nothingness)
2. In other contexts, the caller might be able to know whether or not the person is "fully qualified"
3. We've made the determination that the bloat of 3 APIs (a generic that handles either scenario, and specifics for if you know whether your person is fully qualified) isn't worth the maintenance hassle

Obviously, the best solution is always "don't let someone make a mistake" - but that isn't always possible. In that case, the next best thing is to let a developer or tester know during the release process.

There are three common ways to handle this:

Logging or Tracing

Depending on the framework you're working in, there are often ways to either trace the execution of a function, or log messages somewhere. These often have variable severity levels, and can be suppressed based on the context they're run in. For example, in Python you might use logging.info.

Conditional Compilation

Many languages and build systems have ways to include or exclude code from the final binary or deliverable. For example, in C# you could have a block of code like this using preprocessor directives or a ConditionalAttribute.

public void DoSomeWork()
{
// using these directly is discouraged, so either factor this type of check out or use a Conditional("DEBUG") attribute
#if DEBUG
    MyLogger.Log(MyMessage)
#endif
    SomeMethodICall();
}

public void DoSomeOtherWork()
{
    LogMyMessage();
    SomeMethodICall();
}

[Conditional("DEBUG")]
private void LogMyMessage()
{
    MyLogger.Log(MyMessage)
}

Since CPython isn't compiled, you don't really have these options out of the box. In order to prevent it from hitting the bytecode you would need some kind of pre-processor, such as pyprocessor. Otherwise if you just want runtime checks, you could use if __debug__: (and run your program with -O or -OO).

Static Analysis

This one doesn't work quite as well for your scenario, since there is no obvious way to catch this statically (unless you were able to use type hinting to distinguish between possible person_id types). Broadly speaking, however, static analysis is a great way to catch these types of issues. Many IDEs and build systems let you add additional static analysis and linting rules to your project, that could even be defined for specific functions that are known problem areas.

PyLint for Python supports custom AST-based checkers. MyPy supports custom type checking operations.

Answer 13

I feel the user might be confused about the business logic if they supply the country code.

"In the face of ambiguity, refuse the temptation to guess."

Ask the user what they want, discuss the consequences to make sure that's what they really want, implement it, give to the user, ask if they are happy with it.

Answer 14

This is a silly API in the first place. You allow passing in a faulty object along with the fix for it. This is inexplicable.

You would be better of validating the id for being complete and throwing if it is not, leaving out the country code argument altogether.