CodeSOD: An Exceptional Junior

When “dragoncoder047” was but a junior developer, without very much experience at all, they were tasked with building error handling in a Python Flask web application.

Now, they were a junior, and tossed into the problem without much preparation, or much supervision, and just told to “make it work”. So they did. With this disaster:

def _is_exception(obj):
if type(obj) is TypeError: return True
try:
raise obj
except BaseException as e:
if type(e) is TypeError and str(e) == “exceptions must derive from BaseException”:
return False
else:
return True
raise Exception(“unreachable”)

First, we compare type(obj) is TypeError. Is the type of obj TypeError? This is the wrong way to compare types, in general, as it doesn’t respect inheritance- isinstance(obj, TypeError) would be more appropriate, but then again, *this is an exception type. The try/except statement can filter by types for us, if we just throw the exception…

Which is exactly what we do next. We raise obj inside of a try/except. Then we catch it, as a BaseException, the parent of all exceptions. Then, once again, we check to see if the exception is a TypeError and then also the message of the exception. Why?

Because this is the exception raised when you throw something that isn’t actually an exception class. You’re not allowed to raise “an error occurred”, you must raise SomeExceptionTypeWithBaseExceptionAsAnAncestor(“an error occurred”).

And that’s why they did the TypeError check above. If the incoming exception is already a TypeError, then we know it’s an exception. In that case, we return True. If it’s not a TypeError we throw it, knowing that throwing a non-exception object results in a TypeError with a specific message. So we can return False in that case. Otherwise, we can return True.

Finally, we add one last exception throw to please the linter- despite the fact that we know that this will definitely return a value, the linter doesn’t, because this code is wrong on so many levels. So we ensure that it never reaches the end of the function without a return.

All of this could be replaced with isinstance(obj, BaseException). But even if we did, we’re left with the question of: why do we need to do this at all? Using isinstance at all is a code smell in 90% of cases, and handling objects that are exception objects outside of some sort of try/catch block implies that we’ve gone way off somewhere.

Well, our submitter explains why:

The kicker is that the only place this function is used is in one single view function in a larger Flask web server. If the argument called “path” is actually an exception object, we render and return the error page template, else we do some other stuff with the actual path that was requested. And the reason that check is necessary is because I installed the same view function as both a request handler and an error handler — instead of just doing the obvious thing and just writing a dedicated error handler!

It’s a case of baby’s first code-reuse creating bizarre code to try and glue unrelated functions into the same view. Sometimes the view shows actual data, sometimes it shows errors. So we have to do this test on the model to see if it’s actual data, or an error.

As with many of our confessions, the developer’s heart was in the right place. They wanted to re-use code. They simply didn’t know how. And with no one giving them the support they needed, we got… this.

Source: Read More