Criminal exceptions

Monday 29 March 2010 — This is close to 15 years old. Be careful.

We’re using socks.py to provide SOCKS proxying in some code at work, and it works great until it doesn’t. Then, unfortunately, the author didn’t try very hard to help us out.

Recently we got this exception:

GeneralProxyError: (5, "bad input")

Looking into the code, here’s where it raises that error:

if (type(destpair) in (list,tuple)==False) or (len(destpair)<2) or (type(destpair[0])!=str) or (type(destpair[1])!=int):
    raise GeneralProxyError((5,_generalerrors[5]))

This is criminal: here is input validation, all of which focuses on a single variable, and when raising the exception, it doesn’t include the value of the variable! Ugh.

Error handling is no different than the rest of your product: you need to put yourself in your customer’s shoes and think about what they’ll need. Then give it to them. Simple.

Comments

Amy Griffis 8:24 PM on 29 Mar 2010

Ugh is right!

Dj Gilcrease 12:51 AM on 31 Mar 2010

I hate errors like that since it prevents me from catching only expected ones. Plus that code is just wrong in so many ways

should be written as

if not isinstance(destpair, (list, tuple)) and len(destpair) != 2 and\
  not isinstance(destpair[0], basestring) and\
  not isinstance(despair[1], int):
    raise GeneralProxyError((5,
        _generalerrors[5] + '\ndestpair myst be a list or tuple of length two '\
        'with the first being a string and the second being and int\n'\
        'destpair = %s' % (str(destpair),)))

Ned Batchelder 6:11 AM on 31 Mar 2010

@Dj: you are right about (most of) your changes. isinstance is better than type()==, and basestr instead of str would actually have solved my problem. I was getting this exception because the first element was a Unicode string.

I have to disagree with one of your changes though: You use %s in your format string, and explicitly convert destpair to str. This is redundant, but worse, it can throw away information. %r is a better choice in cases like this, since it more accurately conveys the information about what destpair is. In my case, where destpair[0] was Unicode, your code wouldn't have shown me what was wrong, since the %s would explicitly discard the important difference!

Criminal exceptions

Comments

Add a comment: