Application that processes XML based on configuration

Question

I used CodeClimate to evaluate my code and marked that the following piece of code appeared twice in my source:

 except IOError as e:
 log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
 app_exit(1)

The justification is that the amount of duplication mass is 33, which is above default limit of 32.

How would you solve this problem? Would you disable the check? Would you wrap the 2 lines in a new method? Something else?

The duplicate part in question is in main:

#!/usr/bin/env python
import logging as log
def get_cfg(xmlfile):
 # gets configuration from file which 
 # has the same name as XML
 # but with different extension.
 pass
def get_root(xmlfile):
 pass
def app_exit(status):
 log.info('Exiting...')
 exit(status)
def setup_logging(args):
 if args.verbose == 2:
 log.basicConfig(level=log.DEBUG)
 elif args.verbose == 1:
 log.basicConfig(level=log.INFO)
 else:
 log.basicConfig(level=log.WARN)
def main():
 import argparse
 parser = argparse.ArgumentParser()
 parser.add_argument('-f', '--file', help='XML file to be parsed')
 parser.add_argument("-v", "--verbose", action="count",
 help="increase output verbosity")
 args = parser.parse_args()
 setup_logging(args)
 xmlfile = args.file
 log.info('Getting root element from XML {}.'.format(xmlfile))
 try:
 root = get_root(xmlfile)
 except ET.ParseError as e:
 log.error("Error while parsing {0}: {1}".format(xmlfile, e))
 app_exit(1)
 except IOError as e:
 log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
 app_exit(1)
 log.info('Getting configuration of XML {}.'.format(xmlfile))
 try:
 cfg = get_cfg(xmlfile)
 except IOError as e:
 log.error("I/O error({0}): {1}".format(e.errno, e.strerror))
 app_exit(1)
 # processing of XML using configuration
if __name__ == '__main__':
 main()

Answer 1

Pardon my lack of Python knowledge, but I think a function something like this might work:

def fatal_error(message):
 log.error(message)
 app_exit(1)

And now your catch cases look more like this:

try:
 root = get_root(xmlfile)
except ET.ParseError as e:
 fatal_error("Error while parsing {0}: {1}".format(xmlfile, e))
except IOError as e:
 fatal_error("I/O error({0}): {1}".format(e.errno, e.strerror))
log.info('Getting configuration of XML {}.'.format(xmlfile))
try:
 cfg = get_cfg(xmlfile)
except IOError as e:
 fatal_error("I/O error({0}): {1}".format(e.errno, e.strerror))

Answer 2

Looking how to avoid duplication error I missed how simpler nhgrif's solution is. Eventually I used the following method that accepts any number of arguments and prints them before exiting.

def fatal_error(*messages):
 for m in messages:
 log.error(m)
 app_exit(1)

Answer 3

Avoid duplicate identical error messages

One of the main points to log error messages is to help you debug when it eventually fails. And if at the point in time your code is getting slightly large there are a few points I find vital in an error message:

• Uniquely identifiable – Based on the non-varying text of the error message you should be able to locate the precise code line where the error was triggered.

  Having duplicate error message, which was your original request, does clearly not have an identifiable location.

• Detailed information helping to reconstruct error – Just stating "IOError" is the worst kind, as you know you have a bug/feature somewhere related to IO, but you have no information. Your code is a little bit better as it gives the error code and string, but still it doesn't help you identify what you were trying to do at the time.

  In your case I would add information on the operation and which xml file you were processing.

• Using unique error codes – You return 1 in both cases, if this was an unique number you could possibly supply your potential customers with work arounds when they encounter a given error code. When they are equal you only know that they have an issue, and it's much harder to provide good help and work arounds.

In your case I think I would have something similar to the following blocks:

except IOError as e:
 log.error("IOError when locating root element - File: {0}, Code: {1.errno}, Message: {1.strerror}".format(xmlfile, e))
 app_exit(1001)
... 
except IOError as e:
 log.error("IOError when reading configuration - File: {0}, Code: {1.errno}, Message: {1.strerror}".format(xmlfile, e))
 app_exit(1002)

Contrary to the other answers I would keep the app_exit() next to the error handling, as it clearly indicates that your program will terminate at this point, and that is useful information to have.

I would however consider to join the two exceptions, and potentially expand the error message with the type of exception caught. Note that the best practice to capture two or more exceptions is like the following:

except (ET.ParseError, IOError) as e:
 pass

• kon psych: You mention some good points. I was expecting the error message to capture the file name but it doesn't. About the last point the exceptions may not be mergeable in a message that is differentiated only by error type. I will consider it though.