Caves of Qud

Caves of Qud

Harmony Injector
HawkleyFox  [developer] 5 Aug, 2019 @ 5:17am
Do Not Swallow Exceptions in `HarmonyShim.Harmony.Patch`
So, due to the Better Ability Bar mod no longer working with the latest version of Qud, I've opened up the code again to fix it.

I recall your comments about how Better Ability Bar's use of a `try-catch` block in its initializer was responsible for breaking functionality in Hover Info. I naturally disregarded this, since it should not be possible for the mere presence of a `try-catch` to affect program flow in any way, assuming no exceptions are actually raised in the `try` block.

But then I recalled that other part you said, "i already have try catch block in Patch". I've finally taken a look at this code and see that it is an empty `catch` block matching `System.Exception`.

So, what is probably happening is that you're hiding a critical problem with my program from me. By swallowing the exception thrown when a patch fails, my program will merrily continue to execute, never knowing that its program state is completely, utterly broken.

If patch `A` fails, but patch `B` succeeds, and patch `B` relies on information that patch `A` was meant to extract from the game-state (which does happen in Better Ability Bar), then what we have is undefined behavior being introduced into my program. I have no idea what it's going to do if only some, but not all, of the patches get applied.

It probably won't be good. And this is why my initializer has that `try-catch` block and why I did not remove it, as you suggested.

The purpose of exceptions are to inform the caller of a procedure that it was unable to complete the task it was designed to do. If you discard those exceptions and provide no other means in your public API to indicate that it failed, then the caller has no awareness of the failure-state and no possible way to handle it gracefully.

Exceptions should be treated as a part of your public API, same as the arity, parameter types, and return types of your methods; they're essentially a different kind of "return value" that a method can produce to indicate unexpected failures to the caller. Hiding failures that you do not handle and resolve, such that your method can still succeed in its designed task, is incredibly irresponsible.

I recommend changing the code in `HarmonyShim.Harmony.Patch` in one of the following ways:
  • Remove the `try-catch` block and allow exceptions raised by the call to `Injector.Shared.Patch` to fall-through to the caller.
  • Catch the exception, but throw a different exception from the `catch` block in its place; this is handy if you feel the need to hide private information the original exception provides from the caller or to provide alternative, simplified information about the problem to the caller. You may also wish to include the original exception as an `InnerException[docs.microsoft.com]` in this new exception to further aid debugging, if possible.

And, in the future, please consider very carefully what you're doing when you use an empty `catch` block. While this does still have its uses, it is considered an anti-pattern[devblogs.microsoft.com] by the majority of programmers for a damn good reason. If you know of any other places in Harmony injector's public API where you do this, consider refactoring them as well.
< >
Showing 1-1 of 1 comments
namkazt  [developer] 14 Aug, 2019 @ 1:42am 
ok good call. i will update it
< >
Showing 1-1 of 1 comments
Per page: 1530 50