Changes between Version 18 and Version 19 of DeveloperGuidelines/CodeConventions

Timestamp:

06/21/11 14:08:56 (14 years ago)

Author:

Mike A

Comment:

added notes about exception handling

DeveloperGuidelines/CodeConventions

@@ -52 +52 @@
    * [http://pychecker.sourceforge.net/ PyChecker] recommended in [http://code.google.com/p/soc/wiki/PythonStyleGuide PythonStyleGuide]
 
+
+== Exceptions ==
+
+* 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.
+ * If you do use a naked except, the exception should be re-raised.
+ * 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.
+* 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. 
+* It's good practice to clean up resources in finally blocks.
+* avoid raising errors inside exception handlers.
+* don't ignore exceptions. At the very least, log them, or raise them in debug mode.
+ 
+Instead of this:
+{{{
+try:
+    # this could raise an error we aren't prepared for
+    file_path = get_file_path()
+    f = file(file_path, 'w')
+    # this could also raise an exception
+    f.write('Please send the helicopter to %s' % disaster_location)
+    f.flush()
+except:
+    pass
+    # Argh! by ignoring the exception, now lots of things could be messed
+    # up and the rest of the code could break in strange ways.
+    # We will have a hard time figuring out the reason as the original 
+    # exception is now lost.
+    # Worse, maybe the user doesn't notice and people die on a mountain.
+}}}
+
+Do this:
+{{{
+# now we see any errors here separately
+file_path = get_file_path()
+try:
+    # the try block is minimised 
+    f = file(file_path, 'w')
+except IOError, exception:
+    # e.g. a permissions violation
+    if deployment_settings.debug:
+        # re-raise in development
+        raise
+    else:
+        # logging is good
+        logging.error("%s: %s" % (file_path, exception))
+
+        # email reports are great as the developer doesn't have 
+        # to log into the machine to read them.
+        # this mustn't raise exceptions (tricky)
+        send_error_email_to_devs(exception, locals(), globals(), request)
+
+    if not exception_has_been_totally_handled_and_no_further_consequences():
+        # let the user know so they can take action if possible.
+        raise HTTPError(500, "Cannot queue your message because: %s" % exception)
+else:
+    f.write('Please send the helicopter to %s' % disaster_location)
+    f.flush()
+finally:
+    # finalise resources
+    f.close()
+}}}
+ 
 ----
 DeveloperGuidelines