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

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

Answer 3

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)