Changes between Version 18 and Version 19 of DeveloperGuidelines/CodeConventions


Ignore:
Timestamp:
06/21/11 14:08:56 (10 years ago)
Author:
Mike A
Comment:

added notes about exception handling

Legend:

Unmodified
Added
Removed
Modified
  • DeveloperGuidelines/CodeConventions

    v18 v19  
    5252   * [http://pychecker.sourceforge.net/ PyChecker] recommended in [http://code.google.com/p/soc/wiki/PythonStyleGuide PythonStyleGuide]
    5353
     54
     55== Exceptions ==
     56
     57* Do not use naked excepts. Yes there are lots already in the code base. Do not make the situation worse. They are being gradually fixed.
     58 * If you do use a naked except, the exception should be re-raised.
     59 * database errors are the typical tricky case as we don't want to create dependencies on database modules that aren't used. You can catch BaseException and check the exception class name, or you can provide and reuse standard tuples of exception classes in the configuration that map to the same generic database error.
     60* try blocks should be minimised, so that there is less chance of catching the same exception type from a place that wasn't expected, (and then handling it incorrectly). To do this, move code before the exception raising line before the try block, and code after the exception raising line into an else block.
     61* It's good practice to clean up resources in finally blocks.
     62* avoid raising errors inside exception handlers.
     63* don't ignore exceptions. At the very least, log them, or raise them in debug mode.
     64 
     65Instead of this:
     66{{{
     67try:
     68    # this could raise an error we aren't prepared for
     69    file_path = get_file_path()
     70    f = file(file_path, 'w')
     71    # this could also raise an exception
     72    f.write('Please send the helicopter to %s' % disaster_location)
     73    f.flush()
     74except:
     75    pass
     76    # Argh! by ignoring the exception, now lots of things could be messed
     77    # up and the rest of the code could break in strange ways.
     78    # We will have a hard time figuring out the reason as the original
     79    # exception is now lost.
     80    # Worse, maybe the user doesn't notice and people die on a mountain.
     81}}}
     82
     83Do this:
     84{{{
     85# now we see any errors here separately
     86file_path = get_file_path()
     87try:
     88    # the try block is minimised
     89    f = file(file_path, 'w')
     90except IOError, exception:
     91    # e.g. a permissions violation
     92    if deployment_settings.debug:
     93        # re-raise in development
     94        raise
     95    else:
     96        # logging is good
     97        logging.error("%s: %s" % (file_path, exception))
     98
     99        # email reports are great as the developer doesn't have
     100        # to log into the machine to read them.
     101        # this mustn't raise exceptions (tricky)
     102        send_error_email_to_devs(exception, locals(), globals(), request)
     103
     104    if not exception_has_been_totally_handled_and_no_further_consequences():
     105        # let the user know so they can take action if possible.
     106        raise HTTPError(500, "Cannot queue your message because: %s" % exception)
     107else:
     108    f.write('Please send the helicopter to %s' % disaster_location)
     109    f.flush()
     110finally:
     111    # finalise resources
     112    f.close()
     113}}}
     114 
    54115----
    55116DeveloperGuidelines