Criminal exceptions

Monday 29 March 2010

We’re using 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.


Ugh is right!
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),)))
@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!

Add a comment:

Ignore this:
Leave this empty:
Name is required. Either email or web are required. Email won't be displayed and I won't spam you. Your web site won't be indexed by search engines.
Don't put anything here:
Leave this empty:
Comment text is Markdown.