Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Before I try to make this work, I wondered if anyone had tried this, whether it was a good idea or not, etc.

I find myself doing this a lot:

if some_result is None:
    try:
        raise ResultException
    except ResultException:
        logger.exception(error_msg)
        raise ResultException(error_msg)

I would rather try this:

if some_result is None:
    with ResultException:
        logger.exception(error_msg)

Where the __exit__() statement would always raise the exception in the context.

Would this be clearer? Is this possible?

I only ask because I imagine someone else has tried this, or at least attempted it. But I don't seem to find any common patterns for accomplishing this after doing a couple of Google searches.

UPDATE

Here's some more information on this particular issue.

I have a method that goes to a database and determines what the user's id is via their email. The problem is that if the user isn't found, it'll return None. This is okay for most calls, but when it's incorporated in higher-level methods, this can happen:

def change_some_setting(setting, email):
    user_id = user_id_by_email(email) # None
    _change_some_setting(setting, id)

Now when _change_some_setting attempts to change the setting for a user who's id is None, we get a very vague ProgrammingException from the database library.

This helps prevent that. Is there a better way?

share|improve this question
Why not log the exception when you actually handle it? The traceback will show where it's from. – Janne Karila Apr 11 at 19:56
As far as this example is concerned, this is handling it. It catches result sets that are empty, logs the complaint, and then bails out of execution. – Droogans Apr 11 at 20:09
Can you show the code that catches the exception? – Winston Ewert♦ Apr 11 at 20:56
The fix is that user_id_by_email should raise an exception, as @JanneKarila said in his answer (however creating a wrapper is only acceptable if you really can't change user_id_by_email). – Quentin Pradet May 2 at 7:13

2 Answers

up vote 1 down vote

This:

if some_result is None:
    with ResultException:
        logger.exception(error_msg)

would seem to be equivalent (for most cases) to:

if some_result is None:
    logger.exception(error_msg)
    raise ResultException

And so using a context manager as you describe it would not seem to be helpful. That's probably why you can't find any instances of it. But this would not actually work because when you call logger.exception, you need to be in an exception handler.

You could do:

def raise_logged(exception):
    try:
         raise exception
    except: 
         logging.exception(str(exception))
         raise exception

Then use it like

 if some_result is None:
       raise_logged( ResultException("not so good") )

However, throwing the exception just to catch it and log it is a bit yucky. Do you really need to log the exception as this point? Usually, we log exceptions when they are caught.

share|improve this answer
I updated my question with more specific information about my problem. – Droogans Apr 11 at 20:52
up vote 1 down vote

Your issue is that a function returns None when raising an exception would be more useful to you. I would create a wrapper for the function for the sole purpose of raising the exception. Then catch the exception normally where it makes sense to you, and log it from there.

def checked_user_id_by_email(email):
    result = user_id_by_email(email)   
    if result is None:
        raise LookupError("User not found by email '%s'" % email)
    return result

You might use a decorator to wrap your functions like this.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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