2

I'm starting to notice this pattern throughout some of my code:

try:
    some_func()
except FirstException as err:  # known possible err
    # log err
    notify_user(err)
except SecondException as err:  # known possible err
    # log err
    notify_user(err)
except Exception as err:  # unexpected err to prevent crash
    # log err
    notify_user(err)
finally:
    # close logs

It should be noted that some_func does not explicitly raise these exceptions, but calls other methods that may propagate them back up the call stack. While its clean and clear that each exception caught will get its own logging (which presumably has a unique message), they all call the same notify_user. Therefore, this structure is not DRY, so I thought:

try:
    some_func()
except Exception as err:  # catch all (presumably) exceptions
    if isinstance(err, FirstException):
        # log err
    elif isinstance(err, SecondException):
        # log err
    else:
        # log err
    notify_user(err)
finally:
    # close logs

While also clear that logging is specific to the desired error caught, its now DRY.

But doesn't this defeat the point of knowing know exception types by trying to catch all exceptions and using conditionals to identify which is caught? Broad exception catching seems "okay" in concept, but I'm concerned it could lead to some unforeseen consequences later.

6
  • Normally catching all exceptions could be a problem, but here you're doing that in either case. So the usual objections don't apply. I think either variant is fine. – amon Nov 9 '20 at 19:31
  • @amon Is it abnormal to have the "catch all exception" portion at the program root to prevent it from crashing? Say in a Java main method perhaps? – pstatix Nov 9 '20 at 20:26
  • 1
    A catch-all at the top level or at another failure boundary is very common and often absolutely necessary. But you should note that Python's exception hierarchy is a bit weird – the root is not Exception but BaseException. Whether you should catch those as well depends on the context. – amon Nov 9 '20 at 21:42
  • 1
    You could use multi except like described here: stackoverflow.com/questions/6470428/… – Minato Nov 10 '20 at 5:50
  • Do you have control over the exception hierarchy? If so you could institute a base class which can be caught and logged in a single catch here. – Kain0_0 Nov 10 '20 at 6:49
4

Catching a bare Exception is mostly bad practice, but this would solve your problem as I have understood it.

EXCEPTION_SPECIFIC_LOG_MSG = {
    FirstException: 'first exception',
    SecondException: 'second exception',
    Exception: 'bare exception'
}

try:
    some_func()
except (FirstException, SecondException, Exception) as e:
    print(EXCEPTION_SPECIFIC_LOG_MSG[type(e)])
    notify_user(e)
2
  • I agree that catching the bare Exception is generally a bad practice, but this is at the root of the program (analogous to Java's main), so the bare catch is there to prevent a program crashing and gracefully notifying the user (along with crash dumping). – pstatix Nov 10 '20 at 12:37
  • Note also that this has very little to do with "DRY". Dry is important when critical logic is involved. You don't want to have your business repeated all over the code. Handling exceptions is a very corner case that might justify blocks like the one you dislike. On the other hand, to make these blocks DRY you would have to encapsulate these blocks into a sort of handler for further reuse. But @Minato answer is, IMO, a cleaner and elegant way to solve your main concern. I would get rid of the map and I would just print e.message. If any. – Laiv Nov 10 '20 at 13:33
1

Your solution looks ok to me. Alternatively (not necessarily "better"), here is a DRY version without using isinstance:

lasterr=None
try:
    some_func()
except FirstException as err:  # known possible err
    # log err
    lasterr=err
except SecondException as err:  # known possible err
    # log err
    lasterr=err
except Exception as err:  # unexpected err to prevent crash
    # log err
    lasterr=err
finally:
   if lasterr is not None:
      notify_user(err)
    # close logs

Of course, the statement lasterr=err is repeated here, but since this does not contain any real "logic", this does not count as a DRY violation. Those statements are usually not as likely to change as a function call notify_user(err), so the goal of DRY to reduce the number of places where changes will happen is fulfilled.

You did not show us the logging code, but it might be also a good idea not to do the real logging at all places where # log err occurs, only remember the specific logging message in a variable, and do the logging in the finally section as well.

4
  • I had this at one point but that the boolean variable addition and reuse was still DRY. Normally I don't catch the generic Exception, but this is at the program root and would prefer to handle it gracefully rather than crash. Is that an anti-pattern or fairly common? – pstatix Nov 9 '20 at 21:21
  • @pstatix: "boolean variable addition"? No idea what you mean. And to your second question (which is independent from the alternative implementations): handling generic exceptions gracefully at the program's root is a fairly common strategy for many systems. If it is suitable or not depends on the context, what kind of program you are writing, what consequences a bug might have and how information about bugs or unexpected exceptions can be communicated to a developer when they occur. (Just noted amon told you the same - I fully agree to what he wrote). – Doc Brown Nov 9 '20 at 21:56
  • Sorry, poor wording, "with the addition of a variable (i.e. lasterr)", at first I read that you were setting it to True, not err. – pstatix Nov 9 '20 at 22:35
  • Why would logging at the # log err not be best? – pstatix Nov 10 '20 at 12:40

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.