Tyrsis / SE-Community-Mod-API

Space Engineers Community Modding API
GNU Lesser General Public License v3.0
23 stars 11 forks source link

Please stop using catch-all exception handlers #55

Closed dodexahedron closed 9 years ago

dodexahedron commented 9 years ago

I know the VAST majority of them are from legacy inherited code from the project, but I have a request for anything anyone else writes for this project: unless it can't be helped, please don't write any more catch( Exception ex ) blocks (also known as catch-all exception blocks. The reason being if you don't know what kind of exception you're dealing with, you really don't have any business reacting to it. Using catch-all exception handlers can mask things that you didn't expect and let the program continue running, even though it is in a bad state. If you let the exception bubble up to the top level, it will crash the program and we can deal with the specific issue on a case-by-case basis, instead, based on the stack trace we get.

This is a general "best practice" in c# programming, and I suggest we follow it, since it doesn't really require that much effort.

Tyrsis commented 9 years ago

While this is true, when you're dealing with obfuscation, it's impossible not to do this for one very important reason: each week something may break, and unless you're testing every single obfuscation every single time, you're going to miss it and crash the server. Then you're stuck waiting for an update. I do this so that people don't have to wait for me to update the server right away. Catching errors and showing them allows things to function until a fix can be found. Not catching will kill the server, which in my opinion is way worse.

While I can appreciate "best practices" sometimes they do not work in specific situations, this is one of them.

dodexahedron commented 9 years ago

Fair enough. For what it's worth, I agree in some cases. However, when there's only one possible exception thrown by a piece of code, or when all exceptions are already caught by callees, there's literally no point in an additional catch-all, since it'll never be hit anyway. Most of what I've changed, in that regard, is exactly that, where I've made such changes.