Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of ASSERT #35

Open
pokrakam opened this issue May 10, 2019 · 4 comments
Open

Use of ASSERT #35

pokrakam opened this issue May 10, 2019 · 4 comments

Comments

@pokrakam
Copy link
Contributor

pokrakam commented May 10, 2019

I agree with the principle of Dump for totally unrecoverable situations

However, I much prefer using ASSERTs. Yet they are not mentioned anywhere in the guide?

IF foo IS INITIAL. 
  MESSAGE x666(general).   "Should never happen!
ENDIF.

vs.

ASSERT foo IS NOT INITIAL. 

I must confess I also (mis?)use ASSERT 1 = 2. to force a dump. I don't use MESSAGE because it belongs to the classic dynpro world and the doco is pretty clear about not using it, and even explicitly advises against the above example:
https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/index.htm?file=abenmessages_guidl.htm

Sometimes I'll use ASSERT only to avoid the compiler complaining about an uncaught exception. There are occasions I want an exception to dump, but to keep the syntax check happy I have to do a clumsy catch-and-dump. A typical use case is:

TRY.
    data(guid) = cl_system_guid=>create_uuid_32_static( ). 
  CATCH cx_uuid_error.
    "We have bigger problems if this failed
    ASSERT 1 = 2.  
ENDTRY.

Thoughts? Should some recommendations on ASSERT be included in the guide?

@gbrfarkas
Copy link

Agreed, personally, every time I would comment "should never happen", it becomes an ASSERT.

Using the ASSERT ID .... CONDITION ... variant might be better though as that enables you turn assertions on/off in different environments.

@thomham
Copy link
Contributor

thomham commented May 14, 2019

If you want to write fault tolerant software where the user might not have access to the backend, then you want to transport an error message to the frontend. Also, you can not test ASSERT statement with unit tests.

@pokrakam
Copy link
Contributor Author

@thomham by that logic you are equally suggesting that message type X should also not be used and the whole guideline should be removed. However I think it makes sense, and the author placed good emphasis on using it with great care.

We should raise exceptions for everything we can realistically expect to go wrong, but it's unrealistic to catch every possible thing that can go wrong. Dumps/asserts are for that grey area in between: highly unlikely situations that don't warrant explicit handling but where it would be dangerous to continue.
I also use them for programming errors - e.g. to stop someone from calling a method in an incorrect way; the type of situations that should be caught by the most basic testing.

Regarding ASSERT and unit test, I've grappled with this before, see Is it possible to test ASSERT in ABAP Unit?

Anyway, the point of this issue was to suggest that an assertion is better than message type X receommended by the guideline. And since you raise the point, here's what the responses look like in a web environment:

500 SAP Internal Server Error
ERROR: The current application has triggered a termination with a short dump. (termination: RABAX_STATE)

vs.

500 SAP Internal Server Error
ERROR: The ASSERT condition was violated. (termination: RABAX_STATE)

From a user perspective, I would think that the ASSERT variant is marginally less frustrating as it provides that tiny bit more explanation that something went wrong.

@thomham
Copy link
Contributor

thomham commented Sep 24, 2019

Here some more arguments which came out of another discussion about ASSERT and how and if to include it into the clean code document:


  • The two ASSERT variants - with and without ID - behave completely different and should therefore be looked at differently. While the plain ASSERT triggers a runtime exception which can not be handled, the ASSERT ID variant can be switched off via configuration. The latter feels more like "cranking up the log level to debug".

  • It's hard to unit-test an ASSERT. If the condition is okay, the assertion goes unnoticed. If it fails, it results in a runtime error that cannot be managed by the unit test framework, such that it is not possible to write a unit test that explicitly provokes this failure.
    You can create a test for the private method called by the ASSERT to make sure the detection works, to avoid false positives or false negatives. For a simple ASSERT sy-subrc = 0. I think we can spare automatic tests. If it should have been a <>, you will find out immediately anyway.

  • From a readability perspective, ASSERT can serve as "executable comments" that tell the reader a little more about the program's current expected state.

  • ASSERT seem to fit better to places where my code calls something else, to validate that what happened in those calls matches my expectations, and if not, to uncover programming errors on my side. On the other side, any behavioural change in the called functionality might require de-central rework in all usages. I would prefer having exactly these checks as unit tests on the called method.

  • They seem less fit to validate a method's input at the method's start, as the code is likely unable or unwilling to recognise whether the input is internal or external or comes from a program or human user. For these pieces, exceptions seem to be the better choice, for example because they allow the caller to catch the exception, adjust the input, and repeat the call.


For the use of assertions the guidance given by Code Complete (chapter 8.2 in the second edition) is the one I like best:

Use error-handling code for conditions you expect to occur; use assertions for conditions that should never occur
Use assertions to document and verify preconditions and postconditions

Like the executable comments. Your reader (vacation substitute, development support) can identify that you considered the situation and their next job is to find out whether your assumption was faulty or code running before yours is.

For highly robust code, assert and then handle the error anyway


I see a difference between assertions and exceptions, for example, when it comes to preconditions of a method. I stick to this specific topic. With an assertion I describe a contract any caller needs to adhere to. Violation of the contract is similar to a programming error. (The 'inconsistency' some colleagues mentioned above.) If the signature of a method is clear enough and unambiguous then there is no need for additional assertions. Exceptions are the result of input validations (the value is provided by a user or another system or derived from those input) when the caller needs to be informed about invalid input in that specific case. Not sure if the difference becomes clear: one identifies wrong 'calls' of a method in the sense of wrong programming, the other wrong input. The problem often is that the provider of a method cannot really distinguish between the two. Therefore, exceptions are used in most cases.

xtough pushed a commit to xtough/styleguides that referenced this issue Mar 29, 2021
* update activity diagram

* upper case OK
xtough added a commit that referenced this issue Apr 1, 2021
* Creative commons license

it's not code, anyway

* Create README.md

initial title

* Scaffolding (#5)

* scaffolding

* remove non existing logo

* update title

* README: additional info for onboarding (#6)

* scaffolding #2 (#7)

* scaffolding #2

* some first intro content

+ added VSCode build task

* first intro content

* Inital diagram

Finally got ditaa to work. Plantuml activity diagrams are apparently not supported. Not really convinced yet of the text-based diagrams
approach.

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* change font to sans-serif (#9)

* introduction, minor changes (#10)

* some more notes (#12)

* some more notes

* some words regarding impact

* mention abapgit restrictions/design

* update scenarios

* add note regarding steampunk

* first attempt at best practices

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* small formatting issues (#16)

* minor changes (#17)

* updates (#23)

* update deps

* updates

* upd

* closes #15

* upd

* small additions and corrections

+ some official text on (g)CTS

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* move the static analysis part into the tooling chapter (#25)

* move the static analysis part into the tooling chapter

* add short abaplint description

* show 3 toc levels

* updates

* upd

* small corrections and enhancements on tooling

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* Update README.md (#31)

* Update README.md

* Update README.md

* fix typos

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* add title page logo (#33)

* add title page logo

* add page numbers

* Build timestamp

* recommend spellchecker
* custom dictionary

* add this document section (#32)

* add this document section

* introduction, sections etc.

* remove heading, move related work to new chapter

* add related work to index

* moved to abstract

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* front page hacks (#34)

* update links (#36)

* update activity diagram (#35)

* update activity diagram

* upper case OK

* polish scenarios (#37)

* should fix #26 , #24 , #14
 * needs some more polishing of example for #21

* publish pdf to latest tag (#38)

* Update build.yml

* Create publish.yml

* Update build.yml

* Update build.yml

* Update publish.yml

* dummy, testing (#39)

* rename release asset (#40)

* mention latest release pdf (#43)

* minor changes (#41)

* link Gerrit

* upd

* add todos

* upd

* minor

* upd

* update abaplint section

* some diagrams

* more diagrams

* I love diagrams

* upd

* remove "to"

* happy -> happen

* Update code_review.adoc (#45)

``todo, wording? remove "qualified" ? 

Done.

`` "gain reputation" -> "developers can actively become more knowledgeable about the codebase" something like that

No need to overdo it.  Meritocracy is all about reputation. I don't see how anyone could take offence in that.

* update url (#46)

* related work: add link (#47)

* workaround page numbering problem (#52)

* adjust headings (#51)

* update README.md with automatic build information (#48)

* update README.md with automatic build information

* Update README.md

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* abapgit examples (#53)

* wip: abapgit examples

* abapgit chapter updates

* split setup steps in two

* scenario updates (#49)

* scenario updates

* upd

* upd

* clarify

* update deps

* minor changes (#54)

* name in italics

* additional abaplint links

* abapGit add note

* add link

* Move gCTS example to separate section (#56)

* fixes from Mike (#58)

* README: add "Thanks To" seciton (#57)

* check links in build (#59)

* check links in build

* test

* move structure

* add link to code review guide in main README.md

* prepare latest release links

release links should point to SAP/styleguides

* use main license

* parent license

* ignore build and node_modules

* Delete LICENSE

redundant to ../LICENSE

* Update .gitignore

moved build and node_modules to top level

* README, add AsciiDoc notes

* code review improvements

* motivation: getting different perspectives
* selection: mention central distribution by team leads

* add missing space

* Automated Review --> Checks

* disambiguation of terms

VCS, git platforms...

Co-authored-by: Lars Hvam <larshp@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants