1
\$\begingroup\$

There is a lib code, trying to parse an Element Tree object. If exception happens, it either returns an empty dict of dict or a partially constructed object of such type. In this case, caller needs to parse the results to see if parsing is correctly handled or not. Or in other words, the returned dict is not deterministic. How to solve this issue? Or if this is an issue?

def parse_ET(self, ETObj):
    if ETObj == None: return None
    dict_of_dict = collections.defaultdict(dict)
    try:
        for doc in ETObj.iter("result"):
            id = doc.attrib.get("id")
            for elem in doc.iter("attrib"):
                dict_of_dict[id].setdefault(elem.attrib.get("name"), elem.text)
    except Exception, ex:
        logging.exception("%s:%s" % (self.__class__, str(ex)))
    finally:
        return dict_of_docs
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
def parse_ET(self, ETObj):

Python style guide recommends lowercase_with_underscores for both function and parameter names

    if ETObj == None: return None

Use is None to check for None. However, consider whether you really want to support None as a parameter. Why would someone pass None to this function? Do they really want a None in return?

    dict_of_dict = collections.defaultdict(dict)

Avoid names that describe the datastructure. Choose names that describe what you are putting in it.

    try:
        for doc in ETObj.iter("result"):
            id = doc.attrib.get("id")
            for elem in doc.iter("attrib"):
                dict_of_dict[id].setdefault(elem.attrib.get("name"), elem.text)

Can there be duplicate "id" and "name"? If not you don't need to use defaultdict or setdefault

    except Exception, ex:

You should almost never catch generic exceptions. Only catch the actual exceptions you want.

        logging.exception("%s:%s" % (self.__class__, str(ex)))

Don't log errors and try to continue on as if nothing has happened. If your function fails, it should raise an exception. In this case, you probably shouldn't even catch the exception.

    finally:
        return dict_of_docs

finally is for cleanup tasks. Under no circumstances should you putting a return in it.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks Winston. If I don't want to catch the exception, the best way is to remove try...except? or just raise? \$\endgroup\$ Commented May 1, 2013 at 18:04
  • \$\begingroup\$ What's the best way to force argument not None? <br>>>> def func(a): ... print a \$\endgroup\$ Commented May 1, 2013 at 18:04
  • \$\begingroup\$ @user24622, if the caller caused the error, then you should raise a new error which tells the caller what they did wrong. If its a bug in your code, just don't catch it. \$\endgroup\$ Commented May 1, 2013 at 18:14
  • \$\begingroup\$ @user24622, as for forcing not None, don't bother. You'll get an fairly obvious error in any case, so you don't gain anything by checking. You can't assume that its safe to pass None into things, and so you don't need to let your callers assume that. \$\endgroup\$ Commented May 1, 2013 at 18:15

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.