Why ABAP exception handling isn’t like playing catch with your dog
I’ve been cleaning up some legacy ABAP code lately, and a reoccurring theme is poor exception handling. In this post, I’ll explain why ABAP exception handling shouldn’t be treated like a game of catch with your dog. I’ll discuss some anti-patterns I’ve come across in the legacy code and give a few suggestions on how to improve your exception handling.
CX_ROOT looks something like this:
TRY. zcl_my_class=>my_method( ). CATCH cx_root INTO DATA(lx_root). " do something, like logging the error; " or even worse, do nothing ENDTRY.
CX_ROOT is an abstract class and the super class of all exception classes. Catching
CX_ROOT thus catches every possible exception that could be raised. This is a very broad approach to exception handling, which I’m not a big fan of. When you play catch with your dog, you always want the dog to catch all the balls. This is not an appropriate approach to ABAP exception handling and was the major inspiration for writing this blog post.
I think that exception handling should be as narrow and specific as possible. Hopefully
zcl_my_class=>my_method( ) only raises one specific exception, and the caller should only catch this exception.
I find it hard to imagine scenarios where it makes sense to catch every possible exception, especially when the CATCH clause is empty. Why would you want your code to continue executing when an unknown exception has occurred?
Catching an exception just to raise it again
I’ve come across code which looks something like this:
TRY. zcl_my_class=>my_method( ). CATCH cx_my_exception INTO DATA(lx_my_exception). RAISE EXCEPTION lx_my_exception. ENDTRY.
To catch an exception just to raise the same exception again makes no sense. It is much better to make sure that the exception is part of the method signature and just let the exception bubble up without catching it.
The only scenario I can think of where it would make sense to catch the exception to then raise it again is the scenario where the exception is logged first:
TRY. zcl_my_class=>my_method( ). CATCH cx_my_exception INTO DATA(lx_my_exception). zcl_my_log_class=>write_to_log( lx_my_exception ). RAISE EXCEPTION lx_my_exception. ENDTRY.
Not having the exception in the method signature
I’ve seen code where exceptions are raised, which are not part of the method signature. This makes the code fragile since the callers of the method don’t know which exceptions to expect and thus don’t see the need to implement proper exception handling on their end.
Raising several different types of exceptions from the same method
I’ve seen methods with several exceptions in the method signature. The caller of the method would then have to catch several exceptions:
TRY. zcl_my_class=>my_method( ). CATCH cx_my_exception1. ... CATCH cx_my_exception2. ... ENDTRY.
If a method raises several exceptions, it is probably a sign of the method doing several things. Consider Clean ABAP: Do one thing, do it well, do it only.
zcl_my_class=>my_method( ) raises several exceptions, a refactoring should be done (see Clean ABAP: Throw one type of exception).
Catch is empty
Another scenario I’ve seen is that the exception is caught, but not acted upon, especially with an entertaining comment about how this should never happen:
TRY. zcl_my_class=>my_method( ). CATCH cx_my_exception INTO DATA(lx_my_exception). " do nothing, since this should never happen... ENDTRY.
Maybe there is a scenario where the exception shouldn’t be acted upon, but in that case, I would recommend to:
- Add a comment explaining why it is not necessary to act upon the exception.
- Not catch the exception into an object instance, when it is not being used.
TRY. zcl_my_class=>my_method( ). CATCH cx_my_exception. " No action required since ... ENDTRY.
The TRY-CATCH block is several hundred lines big
A violation of the recommendation Clean ABAP: Keep methods small is to write huge blocks of code and then surrounding these huge blocks of code with a TRY-CATCH. Please keep your methods small, and the exception handling will be much easier to follow.
The anti-patterns discussed above don’t make up an exhaustive list of poor exception handling but illustrate some common pitfalls which should be avoided. If you have more examples, please feel free to add them in the comments section.
Happy exception handling!
This blog post first appeared on the Developer Voyage blog at https://www.developervoyage.com/2020/03/27/why-abap-exception-handling-isnt-like-playing-catch-with-your-dog.html
I have seen an issue where an exception was thrown "resolved" with
I did raise merry heck about it.
I have used catch cx_root once. This was in a BW extractor getting the training data (LSO) for each of 70000+ employees from the HR module on an ECC system. It was necessary, because we could not abort the whole extraction due to one error. The errors could arise from the data and there was no feasible way of error checking every possible error that could be thrown by the standard function modules and methods before trying to get the data.
The catch was written to ensure that the appropriate people were notified if an error happened, with as much detail as possible about what the error was.
Thanks! I understand that you got upset with the first scenario you describe. And thanks for providing the second example, where it actually was necessary to catch cx_root. It sounds like you implemented the best possible solution under the circumstances.
Good blog, just two thoughts:
For some rare, unrecoverable cases it could make sense to intentionally generate a dump via an exception.
Good points! Thanks!
What do you think about "the trouble with checked exceptions"? I think that's one area where C# diverted from Java (and it seems ABAP followed Java with CX_STATIC_CHECK, while CX_NO_CHECK would be C# style ?!).
In that context: What's your opinion of deriving from CX_NO_CHECK ? I tend to put such a catch at the highest level (e.g. around the call of a class method from a report) and only catch specific exceptions deeper inside the class(es), in case I know how to deal with the error.
I don't know whether this is a question towards me or Johan, but I'll give my two cents anyway. 🙂
First of all thanks, that was a good read. They made really good arguments why C# opted for non-checked exceptions.
Personally, I never liked CX_STATIC_CHECK, because based on my experience, most of the time you can ensure from client side that a specific exception will not occur. If you are not requesting a non-existing column from an ALV list in your code, you will certainly not receive the corresponding exception. So why should you pollute the code with a useless handler?
So far, I used DYNAMIC_CHECK based exceptions mainly which is somewhere in the middle between STATIC_CHECK and NO_CHECK. But recently I wrote a complex application and I faced the issue that new error types had to be added at each level of the call hierarchy. So at the end I went just declaring my "root" exception class in the signature of every method.
In a different scenario, I called a standard API which threw a locally defined exception deriving from NO_CHECK (intentionally), which was actually triggered during some circumstances. I really liked the approach that I can process it on client side and I didn't feel the design unclean at all.
All in all, I'm starting to lean towards CX_NO_CHECK. I am willing to give up the list of possible exceptions in exchange of the constant signature adjustment.
They also gave a good point on the handler part. I tend to agree that exception handling is usually not type-based, that is, you have one generic handler regardless of the specific error where you recover and clean-up. (BTW I think ABAP really needs a FINALLY block, which would be more useful than CLEANUP)
Thanks, Gabor. Actually it was directed to both of you and/or the whole community.
Exactly this "adapting signatures along the call hierarchy" makes it tedious, and also simply putting then CX_ROOT everywhere is not better than CX_NO_CHECK.
So I'm glad that you seem to agree with that approach. I was always unsure because the documentation more or less discourages from using CX_NO_CHECK. I'm sure (or even hope) that there are other opinions, so I'm looking forward to reading them.
Maybe there's is also a "inbetween" solution, e.g. class internally use CX_NO_CHECK (for easier signatures) but for Public methods provide CX_STATIC_CHECK or CX_DYNAMIC_CHECK to ensure that the caller handles potential errors. But also that is maybe not perfect because it depends on the size/scope of classes and they should be small (single responsibility principle) and then you end up not much better.
So maybe only for the "public" classes that might be called from outside? But a concept of "package" classes (where then CX_NO_CHECK would be sufficient according to that assumption) doesn't exist in ABAP. Of course, you could simply assume, which classes are really used publicly (but also not nice).
Or putting CX_STATIC_CHECK or CX_DYNAMIC_CHECK only for errors where you know the caller can and should specifically react? (but probably contradicts to the fact that a class shouldn't really care about the caller) And CX_NO_CHECK for everything where anyway it will only end up in showing a message or putting a log entry?
One possible solution might be using hierarchical (inherited) exception classes, so that the external exceptions would be "generic" ones, like ZCX_INTERNAL_ERROR, ZCX_UPDATE_FAILED and the internal (actually thrown) ones would be ZCX_GUI_NOT_AVAILABLE, ZCX_NO_MEMORY, ZCX_FILE_READ, ZCX_FILE_WRITE (which would inherit from the generic ones).
Digging a litter deeper in the topic I found that in newer releases it is possible to declare exceptions inheriting from CX_NO_CHECK in signature of methods.
So maybe a coherent approach would be to use NO_CHECK all the time and for public API's list all the exceptions that can "reach the surface".
Regarding package and API classes: you can define package interfaces to define the list of object that should/can be used from your development package. (It only yields a syntax error if package checks are active in the system but still provides an overview of "allowed" objects).
Very good hint and link. Thanks. Still a way to go till that release for us, but good to know. Yes, that would or could then be the best compromise.
Thanks also for the package interface hint, need to learn about that.
Hi Johan. Thanks for this great blog. In connection with exception handling, I want to add a hint to a nice statement: retry. In some situations it's really helpful.
Thanks for this important post.
For the matter of fact, I'm a big fan of not catching unexpected exceptions instead of catching/logging them. From my experience, a dump will be reported (and fixed) quickly, while a nice message/log can be ignored for ages without anyone is being informed about the problem.
Thanks! I fully agree.
Exception handling is difficult, my main requirement is that it should be readable, class based exceptions are mandatory for bigger developments where we have the benefits, but in other cases I'm in favor of KISS.
What is everyone's thought on raising class based exceptions within a method and catching it in the method itself again?
I've raised a classed based exception within a method to catch it in the method itself again in some cases. I don't think it is generally wrong to do so.