0
try:
    msg_json = json.loads(message_string)
    if "task" in msg_json:
        job_type = msg_json["task"]
        return (job_type, msg_json)
    logger.error(
        "Could not parse message: must provide 'task' property",
        extra={"message_string": message_string},
    )
    return empty
except Exception:
    logger.exception(
        "Error parsing JSON message. Did you accidentally double-escape it?",
        extra={"message_string": message_string},
    )
    return empty

I have this code where i am trying to load some JSON formatted message string. After looking back at this piece of code i feel i maybe using try and catch in the wrong way and i was looking for suggestions as i am new to python and there may be cleaner approaches. There is no bug here but this is more for me to learn the "Cleaner" approach. As such i am open to all suggestions that explain the cleaner and more correct approach.

Dinero
  • 1,070
  • 2
  • 19
  • 44
  • This may be more appropriate for [Code Review](https://codereview.stackexchange.com/). – chash Jun 03 '20 at 18:29
  • 3
    Avoid using generic `except` clauses like `except Exception:` because they can hide unexpected errors. Instead use `except Exception as exc:` and display or log the string value of `exc` so you have a clue as to what went wrong. – martineau Jun 03 '20 at 18:32
  • 1
    If you are just trying to catch a parsing error then you could put it around `msg_json = json.loads(message_string)` only. You should try to catch specific Exceptions instead of All Exceptions. – wwii Jun 03 '20 at 18:33

1 Answers1

1

You could handle both of your error cases in catch blocks, which makes your "happy path" code a bit cleaner and neatly groups all the error handling in one place:

try:
    msg_json = json.loads(message_string)
    return (msg_json["task"], msg_json)
except KeyError:
    logger.error(
        "Could not parse message: must provide 'task' property",
        extra={"message_string": message_string},
    )
    return empty
except Exception:
    logger.exception(
        "Error parsing JSON message. Did you accidentally double-escape it?",
        extra={"message_string": message_string},
    )
    return empty
Samwise
  • 68,105
  • 3
  • 30
  • 44
  • This is awesome and is there any best practice that suggests we should leave logic out of the "try" clause – Dinero Jun 03 '20 at 18:32
  • Any logic that you don't want to catch those exceptions from should not go in the `try`. Since you want to catch exceptions from both `json.loads` and the `['task']` lookup, and you want to always return an empty result rather than raising to the caller, it makes sense to have both of those in a `try`. – Samwise Jun 03 '20 at 18:34
  • There's kind of a larger best practice question about whether it's better to return an empty result than to raise, but that depends on the needs and expectations of the caller. In most cases I think it's better to raise, but that depends on there being any possibility that the caller can do something better than just ignoring it. – Samwise Jun 03 '20 at 18:35