Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question

1 Answer 1

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.

share|improve this answer
    
Thanks Winston. If I don't want to catch the exception, the best way is to remove try...except? or just raise? –  user24622 May 1 '13 at 18:04
    
What's the best way to force argument not None? <br>>>> def func(a): ... print a –  user24622 May 1 '13 at 18:04
    
@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. –  Winston Ewert May 1 '13 at 18:14
    
@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. –  Winston Ewert May 1 '13 at 18:15

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.