14

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.

10
  • 32
    If the caller might know whether the country code is needed, why not offer two separate functions fetch_person_qualified(person_id) and fetch_person_unqualified(person_id, country_code)?
    – amon
    Mar 6 at 22:10
  • 5
    Either the caller knows beforehand whether a person_id is qualified or not (which in turn means they would be better off calling a specific method for fetching a qualified/unqualified person) or they don't, and they'll eventually run into an exception because they provided country_id when they shouldn't have, or vice-versa. Which in turn just forces them to wrap the call in a try/except block, one layer above from your function.
    – jfaccioni
    Mar 7 at 14:56
  • 1
    What's the target audience (i.e. callers) for your code?
    – Pablo H
    Mar 7 at 16:13
  • 7
    Whatever you do, document it so that the caller knows what to expect, and why.
    – gidds
    Mar 7 at 21:36
  • 3
    You are forgetting the use-case where the person_id is a variable - therefore the programmer cannot know when he is writing the code to use your function what type of person_id it is and thus does not know if he should pass a country_code or not
    – slebetman
    Mar 8 at 14:22

14 Answers 14

42

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.

1
  • Your example is not great. The first code block and else block should both have "expensively_caluclate_country_id at the beginning, that would show that an exception might inform them that they always calculate the country when it's unnecessary. Not that I love the exception idea anyway, I just think you are hiding an aspect of the operation to make your point.
    – Bill K
    Mar 9 at 2:35
26

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.

18

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.

4
  • 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 no, it still doesn't make sense. If the caller cannot easily distinguish the ID before calling, how (and when) would the caller know that it also needs to supply the second parameter? Is the caller expected to always call the function with just the ID, and if a null/empty value is returned or an exception is thrown then call again with both parameters?
    – Josh Part
    Mar 8 at 21:03
  • @JoshPart The topic in focus is a caller providing the optional argument when it is not needed, which implies the optional parameter is always being passed. A caller who is uncertain would in good faith always have to supply the optional parameter, or ultimately accept the possibility of causing exceptions.
    – Flater
    Mar 8 at 23:35
  • which means it's pointless that the second parameter is optional if it is always passed by the caller because of uncertainty; and if the caller has to accept the possibility of causing exceptions, then the caller has to use this function in a trial and error fashion either by always passing the second parameter and then calling again with only the first if an exception happens, or doing it the other way around as I mentioned in my previous comment. Either way, the current approach doesn't make sense.
    – Josh Part
    Mar 8 at 23:56
  • to be clear: I don't disagree at all with your answer; I just point out that IMO that last "caveat" part doesn't really add anything useful to it
    – Josh Part
    Mar 9 at 0:00
11

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.

8

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.

1
  • 2
    A proper warning library should handle the "warning overload". Also warnings only for when it is justified to notify the user.
    – qwr
    Mar 8 at 4:53
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?

3
  • 2
    +1 I am a fan of validating the response against the redundant argument because if something goes wrong somewhere, at least the user knows that the record returned is consistent with the data they provided and isn't silently different. Mar 8 at 5:41
  • 1
    I agree. If the fully-qualified person_id exists in the USA but the country_code provided is for Canada, then you should throw an exception. Perhaps it's "person not found" or perhaps it has some detail explaining the issue more specifically, depending on whether that information is useful to the caller.
    – workerjoe
    Mar 8 at 16:57
  • 1
    @workerjoe That really depends on the specific bussiness logic. The country_code may be e.g. a default to deal with legacy data source, and may e.g. conflict with some updated records. Nothing wrong with that. We can't really tell from the question alone.
    – Frax
    Mar 8 at 22:23
5

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.

4

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.

1
  • "Exceptions are for unexpected states, not for expected variances in argument or return values.". I see no reason why you would avoid throwing an exception if you know a function has returned something strange or unusual, and the best thing you can do is get the hell out of there because something has gone wrong somewhere. Mar 8 at 5:37
3

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".

1

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)
1

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.

0

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.

-1

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.

-3

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.

5
  • 3
    It depends on what a "fully qualified" person is. OP is choosing between two datasources, which may be a necessary requirement. I've worked government projects in a country with three administratively independent regions and needing to switch data source can be a necessary implementation that possibly should not be exposed to the caller.
    – Flater
    Mar 6 at 21:30
  • Both forms of ID would be valid. One could be an internal database surrogate ID and the other could be a SSN + country code. Some countries might have overlapping SSN, so the country code would be necessary to do a unique lookup. In any case the example is constructed. The question is about superfluous arguments. I could have used foo and bar as variable names, but I thought it would be easier to relate to with something which was closer to a real world example. My real case is much more complicated involving state machines, which would only make the question unfocused. Mar 6 at 21:33
  • @AndréChristofferAndersen If you had to deal with two different kinds of id you would not mix them up in the same method, you would create separate methods for them. Mar 6 at 22:32
  • Not if they can be treated identical by 99% of your code.
    – gnasher729
    Mar 7 at 5:46
  • @MartinMaat If the source data (or the overall codebase) uses the identifiers interchangeably, having a single method makes sense. If it is known in advance which ID is being used, I agree with your comment.
    – Flater
    Mar 7 at 8:32

Not the answer you're looking for? Browse other questions tagged or ask your own question.