People working with me, know how much I do truly stress in regards to code quality.
Ok, it’s not the worst, I don’t even remember what the worst was, but I recall a lot of time-variant bugs caused by issues like the one in the snippet below. Problems like this can really upset you, but let’s start from the beginning, here’s the snippet (sorry for the pic, but wordpress is a pita — also see my devil mouse pointer in action).
Oh Good Lord! I spent 10 minutes wondering from where I should start discussing this code. I decided to start from the most visible issues. This code almost breaks any coder rule and looks like shit. I won’t tell you from where I took it, but trust me, it’s code that has been used for some time somewhere.
First of all: the scope of the code (“hey, it’s alpha software! we know it”) DOES NOT justify you at all! This code is clearly shit, warm and soft cow dung I’d say!
Issue 1: catching a base exception class just because you don’t know what your code could do means that you don’t know what you are doing. To write reliable code, you need to make your code crash! Or force it to work outside your mindset-boundaries. If things can go wrong, they will go wrong. Costs of maintaining such code will grow with the time and with the amount of functions using your library. What happens if at some point, results stops to be a list of lists (or tuples) [(1,2,3), (4,5,6)]? You will never know that, because the IndexError exception is going to be caught by the generic “except:” statement you wrote.
There are only two exceptions to this rule:
- If you want to catch any exception, then execute some logging function and eventually raise (throw) the exception that you are handling. In case you cannot use the “finally” statement.
- If the library you’re using sucks. It happens with some standard Python modules, like xml.doc.minidom. I had to use that utterly broken way of coding too, but under very controlled circumstances.
Issue 2: the code between try/except statements is too long. You may think I’m still talking about “Issue 1” but I’m not. In general, you should not try to handle a lot of code inside it. For the reason that code mutates over time, requirements change, architecture changes and you could end up stuffing more code inside it and cannot afford to try to reduce the exposure to a particular exception, that should always handle a very specific case. Besides, using a lot of code inside try/finally while making sure to free resources in any case (file objects/descriptors and other critical things like file or thread locks) often due to security requirements is generally accepted (ehehe, at least by me).
Issue 3/4/5/infinite: it’s clear that the guy has no Python knowledge nor he/she spent time reading some Python books or the library documentation in regards to dictionaries. The second for statement could be one-lined by writing: ready = items.values() — or, with Python 3.x: ready = list(items.values()).
Playing with list indexes off lists not directly generated from the very same code snippet is just looking for troubles. An IndexError can accidentally happen and in the case above, nobody will notice it.
There are many other mistakes in that code, and I am frankly bored of talking about it. I spent one year trying to educate people I was working with about such errors, It’s just like somebody doesn’t know how C strlen() or strncpy() works in certain, corner-case circumstances (like omitting the final 0x00 [blame WordPress] if the buffer is not big enough, etc).
Hoping to have educated at least today.