Skip to Content

New vs Old ABAP, Elegance vs Readability, etc...

I thought I would continue the thread from the comments of Is there any elegant way to rewrite below code snippet in 740/750? where we started talking about readability and elegance of code. Earlier today I tried out a piece of code which definitely is not possible pre-7.40.

What the code does is that it creates a new instance of zcl_material if either the material in question changes or there is no existing instance.

  IF zmy_helper=>conversion_exit_internal( i_matnr ) <>
      COND matnr( WHEN ob_mat IS NOT BOUND THEN abap_true 
                  ELSE ob_mat->matnr ).
    ob_mat = NEW zcl_material( ).
  ENDIF.

Here is how I would have written that in the past.

DATA:
  conv_matnr TYPE matnr.

CALL FUNCTION 'CONVERSION_EXIT_MATN1_INPUT'
  EXPORTING
    input        = i_matnr
  IMPORTING
    output       = conv_matnr
  EXCEPTIONS
    length_error = 1
    OTHERS       = 2.
IF sy-subrc <> 0.
  MESSAGE ID sy-msgid TYPE sy-msgty NUMBER sy-msgno
             WITH sy-msgv1 sy-msgv2 sy-msgv3 sy-msgv4.
ENDIF.

IF ob_mat IS NOT BOUND.
  CREATE OBJECT ob_mat.
ELSE.
  IF ob_mat->matnr <> conv_matnr.
    CLEAR ob_mat.
    CREATE OBJECT ob_mat.
  ENDIF.
ENDIF

The first example is compact and I understood it immediately when I wrote it. But, I am looking at it going - what if something doesn't work just right? It's going to be a @#$% of a time to figure out later. For instance points to anyone who can say why I used an abap_true if the ob_mat is not bound.

I'll open the floor to all who want to bash either style of programming.

3
Show 29
* Please Login or Register to Comment on or Follow discussions.
avatar image
Feb 01, 2017 at 09:09 PM edited Feb 01, 2017 at 09:13 PM
348

If anyone has a better place for this, let me know and I will move it. I originally moved it because it was starting to go off topic for the original question. This is more of a discussion than a Question so I didn't think it belonged there. This is not blogging material, and does quite fit here in the world of something I want to read with my Coffee... this is more of a "technical discussion".

0 Share

For instance points to anyone who can say why I used an abap_true if the ob_mat is not bound.

If the method zmy_helper=>conversion_exit_internal( i_matnr ) raised an exception you wouldn't have needed that abap_true.

0 Share

If the method zmy_helper=>conversion_exit_internal( i_matnr ) raised an exception

Sorry, not it... I'll leave my unreadable code up for a while longer for anyone else who wants to venture a guess before I tell you what it does!

0 Share

I think this has something to do with this "If ELSE is not specified, the result is the initial value of the data type type"

Refer.: https://help.sap.com/abapdocu_740/en/abenconditional_expression_cond.htm

Tbh, without knowing what is happening inside zmy_helper=>conversion_exit_internal( i_matnr ) i cannot comment :-/

0 Share

Here is the sequence of events that led to the abap_true.

I first had an abap_false there and what was happening was... if a blank material was sent initially, ob_mat is not bound AND the i_matnr are both INITIAL. So, in that case the IF block was not being executed. The abap_true threw something into the field so that it will get into the block. Elegant? NOT!

I ended up rewriting to...

  IF ob_mat IS NOT BOUND OR
     i_matnr IS INITIAL OR
     zmy_helper=>conversion_exit_internal( i_matnr ) <> ob_mat->matnr.
    ob_mat = NEW zcl_material( ).
    "...do something else...
  ENDIF.
0 Share

if a blank material was sent initially

That's why i said that if your helper method raised an exception(for a blank material), you wouldn't have to code for the ABAP_TRUE!

0 Share

Ah! If that is what you were going with, yes, you are correct - just have to wrap the whole IF block with a TRY...CATCH and then repeat the IF block code in the catch... or write another method. Quickly looses it's elegance!

My conversion routines do not raise any errors, if it can't do it, it just returns the input. Internally, it is a "universal" conversion routine. It determines the conversion exit to use, runs it and returns the result.

0 Share

How about this construct?

Helper Class:

"! Helper Exceptions
CLASS lcx_helper_error DEFINITION INHERITING FROM cx_static_check.
ENDCLASS.
"! Helper Class
CLASS lcl_helper DEFINITION CREATE PRIVATE.
  PUBLIC SECTION.
    CLASS-METHODS conversion_exit_internal
      IMPORTING
        i_matnr        TYPE matnr
      RETURNING
        VALUE(r_value) TYPE matnr
      RAISING
        lcx_helper_error.
  PROTECTED SECTION.
  PRIVATE SECTION.
ENDCLASS.
CLASS lcl_helper IMPLEMENTATION.
  METHOD conversion_exit_internal.
    CALL FUNCTION 'CONVERSION_EXIT_MATN1_INPUT'
      EXPORTING
        input        = i_matnr
      IMPORTING
        output       = r_value
      EXCEPTIONS
        length_error = 1
        OTHERS       = 2.
    IF sy-subrc <> 0.
      RAISE EXCEPTION TYPE lcx_helper_error.
    ENDIF.
  ENDMETHOD.
ENDCLASS.

The exception is raised analogue to the raising of the MESSAGE in the old code.

Material class:

"! Material Class
CLASS lcl_material DEFINITION CREATE PRIVATE.
  PUBLIC SECTION.
    CLASS-METHODS create
      IMPORTING
        i_matnr TYPE matnr.
    CLASS-DATA ob_mat TYPE REF TO lcl_material READ-ONLY.
  PROTECTED SECTION.
  PRIVATE SECTION.
    DATA: matnr TYPE matnr.
ENDCLASS.
CLASS lcl_material IMPLEMENTATION.
  METHOD create.
    TRY.
        ob_mat
          = COND #(
              LET int = lcl_helper=>conversion_exit_internal( i_matnr ) IN
              WHEN ob_mat IS NOT BOUND OR
                   ob_mat->matnr <> int
                THEN NEW #( )
            ).
      CATCH lcx_helper_error.
    ENDTRY.
  ENDMETHOD.
ENDCLASS.

My idea is to illustrate that the object creation for "ob_mat" can be completely put inside the COND operator and shouldn't look complex.

BR,

Suhas

PS - I don't know how the actual classes are built. So excuse me for making any assumptions!

0 Share

This will work for my purpose too based on the example I gave... I am going to rewrite my statement to make it more readable, but keep the IF block because I have another couple of variables to clear. I guess the only way to do that with everything in the structure is to call a method which does the NEW and CLEAR.

EDIT:

I tested and it worked a couple of iterations and then failed. You also need an ELSE ob_mat at the end of the condition, otherwise it will clear out ob_mat if it is bound and the material has not changed.

0 Share

otherwise it will clear out ob_mat if it is bound and the material has not changed

Thanks for correcting me. I have lost count of how many times i have fallen into this trap.

I have also mentioned it in one of my previous responses , "If ELSE is not specified, the result is the initial value of the data type type" :D

0 Share

This clearly would be a good discussion in the ABAP Development space, er tag, I mean. I get that it's not a Question, and maybe not a classic blog, but it might be worth posting there as a blog anyway. Except you've already got it running here, of course.

1 Share

I get where you are going with this....I just wish there was a "happy medium". I too tend to not like really condensed, chained coding. (I see it way overused in other languages!) Sure...it is easy and efficient and takes up less space, but it also can be confusing and "mask" what is really going on. On the other hand, I also don't like when a language takes 10-15 lines to do a very simple task....one that comes up often. I do like some of the new ABAP language/syntax functionality....some is just really cool and makes me go "FINALLY!!!" in many cases. But at the same time, I think it helps....especially those new to ABAP...to understand what it replaces and why. For us "old timers", it is like when we first learned the ABAP "CHECK" statement....which back then, there were many who said not to use it because it was "cheating" and actually a performance issue (because the compiler had to first change it to the ABAP equivalent "if...endif." and then compile....which I don't know if I ever believed). SOooooo......long answer/comment, short....TLDR; I like the newer "condensed", chaining code...but NOT when it is overused to the point of making understanding the logic a greater chore than it needs to be.

2 Share

100% agree with you :-)

but NOT when it is overused to the point of making understanding the logic a greater chore than it needs to be

Don't you think once you get the feel of it, it starts to get addictive :-)

0 Share

But that's the problem.....people get "addicted" to it and want to shoehorn it into everywhere....."look how clever I am with my one line of code!" haha

0 Share

Here's another way of putting it....

3 Share

Bad programmers worry about the code.

Good programmers worry about data structures and their relationships.

--Linus Torvalds

2 Share

Linus Torvalds - He's God !!!

0 Share

Clarity is king & Good programmers worry about...

I have been involved in the periphery of open source (I use Redmine for PM and often make internal mods to support my projects), and have noticed that open source code is often much clearer and well structured than most run of the mill ABAP. My personal opinion here is that in Open Source the audience is other programmers as well. In the SAP world, as long as it works, no one (well other than people on this discussion :D) cares about what's behind it.

I would be willing to Open Source some of my code that I think would be universally helpful if we had the right platform. In the end, with the right participation and feedback, it will make me a better too.

0 Share
Show more comments

Hello Raghu,

the first part of the classic code is a function taking

  • an input of type MATNR
  • returning an output of the algebraic type MATNR + EXCEPTION.

Your expression oriented variant mimic this with a MATNR + BOOLEAN = MATNR type.

The elegant refactoring would be to reduce the complexity of the IF block:

IF ob_mat IS NOT BOUND OR ob_mat->matnr <> conv_matnr. 
  CREATE OBJECT ob_mat. 
ENDIF 

Did I read your intent correctly?

JNN

1 Share

Hi JNN,

        ob_mat
          = COND #(
              LET int = lcl_helper=>conversion_exit_internal( i_matnr ) IN
              WHEN ob_mat IS NOT BOUND OR ob_mat->matnr <> int THEN NEW #( )
              ELSE ob_mat
            ).

Exactly what i had proposed (without the ELSE part ;-) )

What is complex about this expression? :P

- Suhas

0 Share

Hello Suhas,

yes it is, but you could reduce the WTFs/min :

CLASS-METHODS create IMPORTING i_matnr TYPE matnr RAISING lcx_helper_error.

  METHOD create.
      CHECK ob_mat IS NOT BOUND 
        OR ob_mat->matnr NE lcl_helper=>conversion_exit_internal( i_matnr ).
      ob_mat = NEW #( ).
  ENDMETHOD.

JNN

0 Share

That is basically what I ended up doing with one "new" addition.

IF ob_mat IS NOT BOUND OR
     i_matnr IS INITIAL OR
     zmy_helper=>conversion_exit_internal( i_matnr ) <> ob_mat->matnr.

The "new" thing being able to use functions (aka method return) and calculation expressions in the operand position of the IF clause. I am not sure when this changed... I am guessing some time between 3.1 and now :), but this is what has me even more excited than the COND construct. I remember when I started I switched from VB (v6 and before), VBScript, C++ and FoxPro to ABAP, it was like going backwards in time that I couldn't do a simple IF 1 + 1 = 2.

0 Share

OK.

I think the major improvement was Suhas' idea to extract the logic to a method with a meaningful name (create).

JNN

0 Share

I haven't had any experience with new constructs y'all are showing off here but what bugs me more is that 3 different names "mat", "matnr", and "material" are used in the example essentially for the same thing (if I understand it correctly). And even prefixes are inconsistent or at least I don't see much logic there.

Change the names, remove ELSE and it's much better already.

Suhas's version of COND looks "cleaner" to me, although the whole thing in the "old" version is still the easiest to understand to the simpletons like myself. :)

Good discussion though.

0 Share

How about:

material = COND #( WHEN material is not bound THEN NEW zcl_material( )
WHEN material->matnr <> |{ i_matnr ALPHA = IN }| THEN NEW zcl_material( )
ELSE material ).
1 Share

I did try this, but didn't like the way the THEN statement repeated. If I added an input to the constructor for example, I would have to touch both the occurrences of the NEW zcl_material( ).

On the other hand { i_matnr ALPHA = IN } is brilliant. I missed that one and instead used...

{ shift_left( l_variable ) WIDTH = length ALIGN = RIGHT PAD = '0' }

I have a request to any SAP folks who may be watching ( maybe? :) ), can we get a string expression that executes the related conversion exit based on the domain definition? So, the following for example would execute the CONVERSION_EXIT_MATN1_INPUT including any user exits that may be active?

{ i_matnr CONV = IN }
0 Share

Yes it's technically possible to write it into an expression with a single NEW, but I thought the repetition makes it easier to read than increasing the complexity by another factor with an OR.

If it needs changing at another time, it's easy enough to refactor the whole expression.

Personally I would have coded the entire logic into a static method and just had a call to zcl_material=>get_instance( material_number ).

That way the instance handling is in the class and your calling code is clean.

0 Share
Personally I would have coded the entire logic into a static method and just had a call to zcl_material=>get_instance( material_number ).

I really disagree with that approach, here is why. The zcl_material is a very generic class that can be used all over an inventory system for pretty much anything. The moment you start putting in specific instantiation logic - which in this case was for a very specific DYNPRO front end - I can't use it any where else in the system.

Of course, you could say...

1) Create a different get_instance_dynpro1 method for each program you call from. That would loose the elegance of write once use many times. Keeping it separate also helps with regression testing.

2) Or, you could create another class inheriting from the zcl_material just for the instantiation. I don't think that is a good use of inheritance for just doing a relatively simple IF statement.

And guess what? Even if you do put the logic in a method, you still need the same IF statement within it. At that point, you are just moving the code somewhere else. My question there would be, give me an elegant way of coding that IF statement within that static method of yours.

0 Share
10 |10000 characters needed characters left characters exceeded