CHECK vs RETURN, example

[larshp]

in https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#check-vs-return

the good example

METHOD read_customizing.
  IF keys IS NOT INITIAL.
    " do whatever needs doing
  ENDIF.
ENDMETHOD.

Might not be super good, especially it introduces nesting, https://twitter.com/JelenaAtLarge/status/1527775392790204416

I typically have code looking like

METHOD read_customizing.
  IF keys IS INITIAL.
    RAISE NEW...
  ENDIF.
    " do whatever needs doing
ENDMETHOD.

in the example it looks like a validation for input?

[ceedee666]

In my opinion which option to choose depends on the current situation.

• If an empty keys is a situation that should never happen I would also throw an exception.
• If an empty keys is a valid option throwing an exception is an anti pattern. In this case the version with a return should be used.

[ceedee666]

Therefore the beginning of the paragraph should also be clarified.

There is no consensus on whether you should use CHECK or RETURN to exit a method if the input doesn't meet expectations.

[Sachinart]

@ceedee666 Either way, keeping a single IF statement in method is not a good practice. And I support @larshp using IF for raising the exception or using CHECK/RETURN statement and the placeholder for the logic when keys are not initial.

[Jelena-P]

Both examples below are valid, which one to use depends on the scenario and specific requirements. This part is not the matter of cleanliness.

METHOD read_customizing.
  IF keys IS INITIAL.
    RAISE NEW...
  ENDIF.
    " do whatever needs doing
ENDMETHOD.

or

METHOD read_document.
  IF keys IS INITIAL.
    RETURN.
  ENDIF.
    " do whatever needs doing
ENDMETHOD.

The main point is the whole method should never be wrapped in IF... ENDIF. That's anti-clean. :)

[pokrakam]

If it really shouldn't happen (e.g. only through programmer error), then I'd even go as far as assert

There's another related (anti?)pattern:

if <someCondition>. 
  do stuff. 
  do things.
  ...
  more code.
  ...
else.
  raise exception ...
endif.

IMHO it is cleaner to invert the condition and do

if <notSomeCondition>. 
  raise exception ...
endif.
do stuff, party, whatever.

[ruthiel]

If it really shouldn't happen (e.g. only through programmer error), then I'd even go as far as assert

There's another related (anti?)pattern:

if <someCondition>. 
  do stuff. 
  do things.
  ...
  more code.
  ...
else.
  raise exception ...
endif.

IMHO it is cleaner to invert the condition and do

if <notSomeCondition>. 
  raise exception ...
endif.
do stuff, party, whatever.

I agree with this approach!

[Jelena-P]

That's what I usually do too - put the shortest part of IF / exception first. Much easier to read IMHO.

[rdibbern]

Closed with PR

[larshp]

I think this issue is still relevant, and it is common practice to follow the discussion in issues instead of pull requests

[larshp]

alternatively, I can follow the approach and roll back the changes in #277 with new changes?