Improvement for the exceptions section

[germanysources]

I write some improvements for:

• naming exception classes
• catching exceptions in legacy code dependencies
• control flow (fail-fast and don't alter behavior)
• document problem domain terms

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[HrFlorianHoffmann]

While I agree with the content of this section, I wonder whether it makes sense in a clean code guide. This is more a question of communication and documentation between the customers, product owners, solution managers, and architects, than the actual code. Do you have specific examples or incidents in mind where this has been neglected and led to misunderstandings? From my experience, we usually find more than enough explanations in the design documentation, architecture concept documents, scope documents, backlog items, and the application help to get along. SAP even has its own tool SAP Term for defining business terminology.

[germanysources]

Removed this section.

[pokrakam]

Hi,

While some of it makes sense, I am not convinced by this change in the Guide either.

The part about naming exceptions is already explained in the example Use sub-classes to enable callers to distinguish error situations and naming is discussed quite a lot in this guide. Perhaps a small comment in the subclass section could be added, or changing the title to "Use appropriately named subclasses...", but a second example is redundant IMHO.

The legacy code dependencies part is more controversial for me. I think it would make a nice blog on SCN for some discussion. To me the scenario seems very narrow in scope, and whether to catch or to let dump is very context-driven and subjective. Even in the example scenario I would still catch it. Leaving it out as the example suggests looks like someone forgot, therefore it should get a comment, which goes against Clean Code's "Express yourself in code not comments" principle.

I would handle this type of situation in a couple of ways.
Catching others generically makes it clear I don't care what went wrong. I could then raise a slightly informative (possibly dynamic) exception to provide at least some info:

CALL FUNCTION 'BAL_LOG_MSG_ADD'
   EXPORTING
     i_log_handle = log_handle
     i_s_msg = message_to_be_logged
   EXCEPTIONS
     others = 9.
  IF sy-subrc <> 0. 
    RAISE lcx_log_not_initialized.
  ENDIF.

(in real life I would use a RAISE method that also reads the SY message information for more clarity).

Alternatively go by Dump for totally unrecoverable situations, which I believe is the intent of your example, making it also feel a bit redundant for me.

[rdibbern]

Suggested change

	" The bass-class for all booking faults
	" The base-class for all booking faults

[sautermi0]

Just as a small addition to what @pokrakam already wrote:
When implementing code, you can never be sure who will be using it in which context at some point in time. I would argue that a wrapper of legacy functionality can't decide what's managable, unrecoverable, etc.

So to refer to Dump for totally unrecoverable situations and your example: Just because you can't deal with a non-existing log, it doesn't mean none of your consumers can deal with it and try again. And you should at least give them a chance to try, IMHO.

[sautermi0]

I'll close this pull request because:

• While the idea of avoiding side effects is noble, just recommending a ROLLBACK WORK in case of exceptions is not a good idea. Whether a ROLLBACK WORK is a good idea or not is depending on the context (used frameworks, different consumers, ...). Additionally, this would distribute the concern of transaction handling to different places which is not a good idea...
• Regarding meaningful names: We consider this covered in the section names-section and Use sub-classes to enable callers to distinguish error situations of the guide.
• Regarding not catching legacy exceptions and accepting a short dump: This is not a good idea because a wrapper of legacy functionality can't decide what's managable, unrecoverable, etc. and short dumps make the consumers unable to react.