First check input parameters then continue with function logic

[sap]

First check input parameters then continue with function logic

[HrFlorianHoffmann]

The intention is to encourage people to focus on what the method actually wants to achieve, meaning the positive success branch or "happy path". We often see methods where that happy path makes up only a very small portion of the method's code, and the vast majority of lines deals with exceptional or error cases.

For example, a short validation of input in the beginning makes for a healthy method:

METHOD append_xs.
  CHECK input > 0 AND string IS NOT INITIAL.
  DATA(remainder) = input.
  WHILE remainder > 0. 
    result = result && `X`. 
    remainder = remainder - 1. 
  ENDWHILE.
ENDMETHOD.

But methods that need to check a lot of prerequisites before they actually do something don't look so good:

METHOD write_to_file.
  IF filename IS INITIAL.
    RAISE EXCEPTION gimme_a_file_name.
  ENDIF.
  IF filename NS '.txt'.
    RAISE EXCEPTION unsupported_extension.
  ENDIF.
  IF file_exists( file_name ).
    RAISE EXCEPTION sorry_file_already_exists.
  ENDIF.
  " etc.
  DATA(file) = open_file_for_output( file_name ).
  file->write( content ).
ENDMETHOD.

[jfilak]

I do understand your point. However, I still found your example perfectly readable and easy to debug because you don't need to navigate to another method to just to find out what are requirements. You might argue that the requirements should be written in documentation but that's not often the case.

Anyways, your example wouldn't work for me, if the logic validating input parameters wasn't so simple.

[pokrakam]

If it helps, the idea is pretty much directly from Clean Code (chapter 3 if you have a copy).
Methods should do one thing only, and error handling is one thing. Another rule is to only deal in one layer of abstraction within a method.

Sure the example is perfectly readable, but in reality the reader is usually interested in one or the other 'thing', rarely both (due to different layers of abstraction!): A reader typically arrives at a method because s/he wants to know what it does (function) or why it didn't do what it should (exceptions). So it's about making it even easier to read by packaging the less relevant stuff into a well-named method.

[jfilak]

Alright, closing this issue.

I will just add my recommendation to our own fork of this this document because:

• If I go to a function to see what it does, I also want to see what it requires to actually do it (and the need to look at the checks at another location distracts me).
• I do not consider the input parameters validation a thing, if it is in the form of if not ok, then report invalid arg.
• It helps reviewers to spot missing checks because of locality.
• I think we should try to avoid unnecessary levels of abstraction (e.g. creating a new function just hide few lines seems to be an overkill because a smart text editor can fold that block) - think about debugging.
• Function names like validate do not provide the caller with any hint on what it actually does.

[pokrakam]

If I may still respond, reading your first and last points I suspect that the trick you're missing is that method names are hugely important, almost more so than the encapsulation itself. Your last point is valid in that the example in this guide doesn't follow this theory very well, validate( ) is vague.

If the example is written as:

METHOD append_xs.
  check_must_be_positive( input ). 
  DATA(remainder) = input.
  ...
ENDMETHOD. 

Then in 90% of cases you won't need to open the method, fold blocks or anything. Instead you can just read one line instead of five. Ideally the method names should match the requirement terminology too, then it becomes much easier to relate a piece of logic with its requirement and can actually improve the situation described in your third point.

Debugging is also a good point: When I reach such a validation or exception handling method I can decide whether to F5 into it or F7 over it, so it's my choice and actually becomes a time saver.

But of course these are just guidelines, what's also important is that teams agree and follows the same set of rules that work best for them. All I can suggest is to actually try it. Doing things in practice may surprise you; I was horrified at the idea of dropping variable prefixes some years ago, but decided to try it out anyway and became a convert surprisingly quickly and never looked back.

[jfilak]

Well, then you end up in with method names such as file_name_with_ext_txt_which_not_exist (unfortunately, you can use on 30 characters for function name in ABAP) or you have 3 methods file_name_not_initial, file_name_ext_is_txt, and file_does_not_exist. But you should use the prefix raise_if_... because the purpose of those methods is to raise an exception when the conditions are not met and the methods must raise an exception with generic message like The file foo.txt already exists (because functions are often re-used) but I would rather raise a customized message like It is not possible to write the results in to the file foo.txt because it already exists (of course, you can customize the message by adding a try catch block but that looks awful too and actually breaks the rule to focus on one thing - BTW do you also propose to have a method handling errors appearing in the logic?)

[pokrakam]

There will be compromises, I'd probably name the method check_valid_name_and_not_exist( file ). I don't use raise_if_, it's the first time I've heard of this convention.

Regarding your question of whether to split out error handling, yes why not. Maybe not for a one-liner, but it can simplify it, especially if there is repetition (IF contains_error( bapi_messages ). ).

Just a few days ago I had to change some very old pre-OO legacy code that used a custom message dynpro. Every error or message involved 4-10 lines of code to invoke it.

Refactoring into exception classes would be too disruptive. The usual IF sy-subrc... is at least three statements. I like the elegance of ABAP Unit's assert classes to do it all in one, so I followed the same pattern and packaged the error handling into things like:

SELECT ....
  WHERE ...
lcl_check_subrc=>fail_if_not_zero( 'Data not found'(123) ).

Functional people read code too, so this makes it easy for the non-developers compared to a hungarian-prefixed global variable assignment fest. Do readers care that we need to save the current cursor position? No, the majority just want to know that it fails if no data is found, so that's what's written.

Talking of unit test asserts, I even like hiding them behind easier named methods when it gets repetitive. See Writing simple, readable unit tests

I can recommend getting a copy of Clean Code, it's worth a read even if you don't agree with all of it. It is hard to do well but worth it in my experience.

[jfilak]

That's it -> Maybe not for a one-liner ... It depends. This issue was more about proposing the structure for the simple cases where I consider the following as anti-pattern:

METHOD append_xs.
  IF input > 0.
    DATA(remainder) = input.
    WHILE remainder > 0.
      result = result && `X`.
      remainder = remainder - 1.
    ENDWHILE.
  ELSEIF input = 0.
    RAISE EXCEPTION /dirty/sorry_cant_do( ).
  ELSE.
    RAISE EXCEPTION cx_sy_illegal_argument( ).
  ENDIF.
ENDFUCTION.

but it is OK if you rewrite into the following form:

METHOD append_xs.
  IF input < 0.
    RAISE EXCEPTION cx_sy_illegal_argument( ).
  ELSEIF input = 0.
    RAISE EXCEPTION /dirty/sorry_cant_do( ).
  ELSE. " input > 0

  DATA(remainder) = input.
  WHILE remainder > 0.
    result = result && `X`.
    remainder = remainder - 1.
  ENDWHILE.
ENDFUNCTION.

where you can easily factor out into the checks into a function called raise_if_negative_or_zero when you find out you can use them somewhere else.

Anyways, the recommendation in general is OK, I didn't want to replace it (I didn't express myself correctly and perhaps I misled the audience). However, I don't think we need to create a new validation function for each if/check statement and I want to recommend the latter form because the former form is harder to get.