| I wrote this code, but I'm not sure what's wrong with it. Can you explain what's bad about it and how you'd improve it? --- To provide some context, this is the top-level entry point for all requests. It routes requests to code which does application-level things, like showing a code review or updating a task or whatever. When the application-level things go wrong, we try to display a pretty, human-readable exception page with a nicely formatted stack trace (if you're in developer mode), etc. This page relies on some infrastructure subsystems (like configuration, to check for developer mode, and static resources, to include JS and CSS, and rendering, to actually produce the HTML) working correctly. Normally, the exception is in application code and everything works fine. We produce a pretty page and send it back to the user. However, sometimes there's an error in one of these subsystems. If we start rendering the page and encounter a second exception during rendering, we fall back to a less-pretty page which has bare text and no dependencies on subsystems. Normally, this happens during development when you've made a mistake in changing one of the subsystems, and all JS inclusion or all configuration checks are failing. On this bare page, we want to retain the original exception (as well as show the followup exception) because it may be helpful in identifying and resolving the issue. With the obvious caveat that I wrote the code, this architecture seems sensible and appropriate to me, and appears to function correctly in practice. |
To answer your question, I'd start with why you are worried about exceptions in rendering your exceptions? Whatever you are using to render your exception should be so simple and stable that you shouldn't need to worry about it.
Also another question, from a fairly quick glance and I admit I didn't read through every last line of code, it appears as if all application execution is wrapped in a try/catch block, instead of exceptions being caught and handled closer to the potential points of failure is there a reason for this?
And one nitpick, because it was kind of a pain to follow some of the code but nested if/else statements five/six layers deep are generally kind of discouraged.
Thanks for taking time out of your weekend to respond and good luck to your team!