1

I've read various posts that polymorphism should be used instead of isinstance, and I agree that makes sense when the use of isinstance is checking the subtypes of a class to determine what to do. However, is the use of isinstance acceptable when checking the type of an object in order to do something unrelated to the object itself?

For exammple:

last_little_err = last_big_err = None

for thing in things:
    try:
        thing.dothing()
    except (LittleError, BigError) as err:
        logger.error(err, extra={'thingObj': thing.name})
        if isinstance(err, LittleError):
            last_little_err = err
        else:
            last_big_err = err
    else:
        thing.success = True

if last_big_err is not None:
    raise SuperBigError('A big thing error occurred') from last_big_err
elif last_little_err is not None:
    display_error_warning(last_little_err)

I believe using isinstance here is fine since I am not defining what the objects should do based on their type but rather checking their type to determine which variable should store it for later use. Is my reasoning valid?

4
  • This is kinda strange, why do you want to catch this error and save it to a variable? If things[0].dothing() fails, your loop will keep going and keep trying to dothing() on the rest of the things. Is this international? – Alexander Mar 8 at 21:14
  • @Alexander Yes. These are "pass-fail" actions that do not impact the execution of the application, but each should at least be attempted. Any of them can fail, and should be logged, with only the last failure being captured and displayed; handled differently if a known large failure (the raise). – madeslurpy Mar 8 at 21:18
  • And you only ever care about a single error, even if there are many? – Alexander Mar 8 at 21:35
  • @Alexander Correct. Its for logging purposes only. The system tries to repair some things, if they fail thats fine (unless its a big known failure). – madeslurpy Mar 9 at 1:07
5

This seems like a case when ruthlessly eliminating code duplication causes other problems. If you cannot treat LittleError and BigError the same, do not handle them the same way. The if statement in the except block is a big red flag that these exception types should be handled differently. They do have similar elements, that is both should set the success flag to True. But this would be better just catching both exception types on their own with a duplicate call to set the success flag:

try:
    thing.dothing()
except LittleError as err1:
    logger.error(err1, extra={'thingObj': thing.name})
    last_little_err = err1
except BigError as err2
    logger.error(err2, extra={'thingObj': thing.name})
    last_big_err = err2
else:
    thing.success = True

The code repetition is minimal. The code is easy to read. No need to infer the exception type. I find eliminating code duplication inside exception handlers to be more difficult for precisely the reason you posted this question. I've learned a little code duplication in exception handlers goes a long way towards making exception handling a little clearer. The more obvious you make your failure handling code, the better off your application will be.

13
  • 3
    @madeslurpy: DRY is a guideline, not a law. Abide by guidelines when they make your life easier. DRY does not make exception handling easier, so don't feel like you need to abide by DRY. – Greg Burghardt Mar 8 at 20:31
  • 1
    @jonrsharpe But is using isinstance() an abstraction? I'm not pulling anything out to its own method, there is no passing of anything or altering of arguments as described in that article. – madeslurpy Mar 9 at 1:08
  • 1
    @madeslurpy, if you want to make those exception handlers DRY, refactor the common parts into a new function and call that from the exception handlers. – Bart van Ingen Schenau Mar 9 at 8:40
  • 1
    @madeslurpy, YES. DRY is all about having the same logic only in one place, so that you only have to update one place if a modification is needed (due to a bug or a change request). – Bart van Ingen Schenau Mar 9 at 13:40
  • 1
    @madeslurpy, A single function call (logging or otherwise) is not worth it to abstract further. With logging, there is even the chance that it is not really duplicated but only appears to be. It is possible that at some point in the future you want to log slightly different things for BigError and LittleError so you can differentiate the two when looking at the logs. – Bart van Ingen Schenau Mar 9 at 14:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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