Small refactoring towards cleaner code: keep the IF small

[joachimrees1]

I want to share a small refactoring example with you.

For whatever reason, I have this code:

    IF mv_given_cat_for_selection IS NOT INITIAL.

      " take what is given.

      r_result = VALUE #( ( sign   = wmegc_sign_inclusive

                            option = wmegc_option_eq

                            low    = mv_given_cat_for_selection ) ).

    ELSE.

      " Take cat from first/any one line.

      r_result = VALUE #( ( sign   = 'I'

                            option = 'EQ'

                            low    = mt_component_data[ 1 ]-s_v2_items-cat  ) ).

    ENDIF.

Note:
- same thing, two wrintings?! Not so good.

sign   = 'I'                   "no, not so nice

sign   = wmegc_sign_inclusive  "yes, this I like to use. 

- but: might not even matter, as 2 of the 3 lines are duplicates anyway. So let's pull stuff out?!

But first: re-name, so we can understand what the result is:

    IF mv_given_cat_for_selection IS NOT INITIAL.

      " take what is given.

      rt_range_of_cat = VALUE #( ( sign   = wmegc_sign_inclusive

                                   option = wmegc_option_eq

                                   low    = mv_given_cat_for_selection ) ).

    ELSE.

      " Take cat from first/any one line.

      rt_range_of_cat = VALUE #( ( sign   = 'I'

                                   option = 'EQ'

                                   low    = mt_component_data[ 1 ]-s_v2_items-cat  ) ).

    ENDIF.

Note: although in each case we only insert a single line, the result is - has to be - table-like (it's a range / type rseloption ).
Now pull similar lines out of the IF:

    DATA ls_range_line LIKE LINE OF rt_range_of_cat.

    ls_range_line-sign   = wmegc_sign_inclusive.

    ls_range_line-option = wmegc_option_eq.

    IF mv_given_cat_for_selection IS NOT INITIAL.

      " take what is given.

      ls_range_line-low = mv_given_cat_for_selection.

    ELSE.

      " Take cat from first/any one line.

      ls_range_line-low = mt_component_data[ 1 ]-s_v2_items-cat.

    ENDIF.

    INSERT ls_range_line INTO TABLE rt_range_of_cat.

Pull the important part up as far as possible:

    DATA ls_range_line LIKE LINE OF rt_range_of_cat.

    IF mv_given_cat_for_selection IS NOT INITIAL.

      " take what is given.

      ls_range_line-low = mv_given_cat_for_selection.

    ELSE.

      " Take cat from first/any one line.

      ls_range_line-low = mt_component_data[ 1 ]-s_v2_items-cat.

    ENDIF.

    ls_range_line-sign   = wmegc_sign_inclusive.

    ls_range_line-option = wmegc_option_eq.

    INSERT ls_range_line INTO TABLE rt_range_of_cat.

And maybe we can now remove the comments:

    DATA ls_range_line LIKE LINE OF rt_range_of_cat.



    IF mv_given_cat_for_selection IS NOT INITIAL.

      ls_range_line-low = mv_given_cat_for_selection.

    ELSE.

      ls_range_line-low = mt_component_data[ 1 ]-s_v2_items-cat.

    ENDIF.

    ls_range_line-sign   = wmegc_sign_inclusive.

    ls_range_line-option = wmegc_option_eq.

    INSERT ls_range_line INTO TABLE rt_range_of_cat.

Just to make that clear: it's the whole method:

  METHOD hu_sel_build_range_for_cat.

    DATA ls_range_line LIKE LINE OF rt_range_of_cat.



    IF mv_given_cat_for_selection IS NOT INITIAL.

      ls_range_line-low = mv_given_cat_for_selection.

    ELSE.

      ls_range_line-low = mt_component_data[ 1 ]-s_v2_items-cat.

    ENDIF.

    ls_range_line-sign   = wmegc_sign_inclusive.

    ls_range_line-option = wmegc_option_eq.

    INSERT ls_range_line INTO TABLE rt_range_of_cat.

  ENDMETHOD.

Finally: in my codebase I cant do that, but I can do it for the blog: different naming / dropping prefixes:

  METHOD hu_sel_build_range_for_cat.

    DATA range_line LIKE LINE OF result_range_of_cat.



    IF given_cat_for_selection IS NOT INITIAL.

      range_line-low = given_cat_for_selection.

    ELSE.

      range_line-low = component_data[ 1 ]-s_v2_items-cat.

    ENDIF.

    range_line-sign   = wmegc_sign_inclusive.

    range_line-option = wmegc_option_eq.

    INSERT range_line INTO TABLE result_range_of_cat.

  ENDMETHOD.

What I note:
- went away from "new" ABAP back to very traditional one.
- didn't realy save on lines: 11 (with 2 commets) vs. 10 (without comments).
- main improvement was: keeping the IF part smal and dense.

What do you think? Was it an improvement?
How would you write that code?

best
Joachim

[pedro_oliveira02]

Hello,

Why not something like:

INSERT VALUE #( ... low = COND #( ... ) ) INTO TABLE ...

[joachimrees1]

No other reason than me never having used COND til now.

Thanks for the hint!

 

[tobiasz_h]

Hello,

I don't know why but I prefer the version of the code before refactoring. It is immediately clear what the result of a method is. But in general I agree with the topic.

[Ruthiel]

Thanks for the post 93b1c542d4984f0e9a75a57ce6030ff3 !

Let me share how I'd codify this scenario:

INSERT VALUE #( 

    sign   = cl_abap_range=>sign-including

    option = cl_abap_range=>option-equal

    low    = COND #( WHEN mv_given_cat_for_selection IS INITIAL

                     THEN VALUE #( component_data[ 1 ]-s_v2_items-cat OPTIONAL )

                     ELSE mv_given_cat_for_selection )

) INTO TABLE rt_range_of_cat.

[joachimrees1]

Thanks for your feedback! Yes, might well be that sometime, refactoring makes something worse. 🙂

[joachimrees1]

Awesome, thanks!

[matt]

That's how I'd do it... or...

r_result = VALUE #( (

    sign   = cl_abap_range=>sign-including

    option = cl_abap_range=>option-equal

    low    = COND #( WHEN mv_given_cat_for_selection IS INITIAL

                     THEN VALUE #( component_data[ 1 ]-s_v2_items-cat OPTIONAL

                     ELSE mv_given_cat_for_selection ) ).

One of the clean code recommendations (apart from not using Hungarian Notation 😄 ! ), is to use r_result for all your returning parameters of functional methods.

 

[olegbash599]

my proposal - no if and no conditional branches at all

 METHOD hu_sel_build_range_for_cat.

	append initial line to rt_range_of_cat assigning FIELD-SYMBOL(<fs_rng_line>).

	<fs_rng_line> = 'IEQ'.

	<fs_rng_line>-low = _first_not_initial( value #( 

			          ( mv_given_cat_for_selection ) 

				  ( value #( mt_component_data[ 1 ]-s_v2_items optional )-cat ) )

				  ).

  ENDMETHOD.

  

  method _first_not_initial.

	" importing it_list type standard table of hu_category / char4

	" returning value(rv_val) type hu_category / char4.

	loop at it_list assigning FIELD-SYMBOL(<fs>) where table_line is not initial.

		rv_val = <fs>.

		exit.

	endloop.

  endmethod.

[olegbash599]

r_result - is not a good practice from my point of view,

First of all: the word "result" humans usually use in the end of the action (or part of something long activity): John Doe worked hard last year and as a result he is now on Bentley.

Usually functional methods (and in the provided case) are not result of long action, but are calculators of intermediate helpful vars/objects.

The second it is nor short and it does not tell anything about type complexity.

the oldMerry: rv_val;rs_val;rt_val;ro_obj - seems more loudly speaking.

The third: why do we need r?  🙂 the shorter - the better: _v, _s, _t, _o.

...PS. It is just my opinion for your challenging 🙂

[fprokopiuk]

Aren't you missing parenthesis after OPTIONAL in COND statement?

[Ruthiel]

Yes! Indeed I missed one!
Already edited!

 

Thanks!

[matt]

That's fine. I'm just explaining Clean Code (and SAP Style) guidelines.

1. Result. What you describe is one use. Another is "I added 1 to 1 and the result is 2".
2. Using HN is against Clean Code for explaining type is against SAP Style Guidelines and Clean Code guidelines.
3. I use Hungarian Notation not to describe the data (t_things s_thing etc.), but to show use. So G_ for global, i_ for importing, e_ for exporting, x_ for changing (others use c_) and r_result for the returning parameter.

[olegbash599]

1. what if "I added 1 to 1 and the result is 2 and add 2 and the result is 4 and multiply 3 and the result 24 and divide by 2 and the result 12". What are the results or we have one result and others are just intermediate calculations? how it would be in the calculations via functional methods?


2. agree that HN is less helpful in really stable and fast IDE, because IDE could provide fast technical information about vars, but I do not agree that existing ABAP-IDEs are fast and stable. Most of the ABAP-types are in dictionary (which is on server-side) - so to read all technical info we need to do many client-servers requests in background. Other approach is in C++, Java/C# - when the typing is mostly locally and IDE really fast provides all technical information.
   When Mr. "Uncle Bob" Fowler  advised to avoid HN he means that IDE could provide fast and clear picture of technical view of variable and the programmer could concentrate on semantics, but ABAP is mostly server-side typing and IDE could not be so fast as for C++, Java or Python.
   So, avoiding HN in ABAP usually slowing down code understanding (readability); but in C++ it otherwise speeding up.
   Moreover in ABAP we have both data types: specific and generic. and with generic types (and with inline declaration) it could be a little horror avoiding HN.
   I am trying to follow all SAP-recommendations, but SAP/ABAP-heart is to be readable and changeable and be robust; and to achieve this HN should be still used. (PS. my opinion based on experience)


3. Good use. why x - for changing?
   However generic types and inline declarations make ABAP more similar with non-strict-typed languages (like JS, python) and therefore it makes sense to use data description in some cases.

[matt]

1. Just r_result - whether it's a table, a structure, a deep structure, or whatever

2. As you wish. I'm just stating SAP Guidelines and Clean Code.

3. x_ ... by habit. c_ could equally be used, but I use c_ for constants. Again I only use prefixes for use. Not for trying to describe type. Data description can be gained by F2 in Eclipse.

 

[olegbash599]

one comment about naming of vars and method.

Actually, you should use HungarianNotation (or it "child-notation") anyway: either in method names or in params name.

And of course in modern development (where we have different objects and frameworks) it is more recommended to use readability in method-names.

so let's see cases.

we need to get list of materials for planning.

methods read_list_of_matnr2plan

  exporting(r) type MATERIAL_LIST_T.

so, no issue if I avoid HU in parameters name, but my method name contains "list" and returning table type. But actually a bit of HU is using in method name (we can call it HU-child )

but here - no type in method name and no HU in parameters - and readability is decreasing

methods read_matnr4plan

  exporting(r) type MATERIAL_LIST_T.

So, typing of object compexity is good practice to use in method name, but anyway - good practice.

So, you could not talk it is good practice to avoid HU in parameters - because it is descreasing readability,

But if we provide clear/accurate method's name and keep it short, then: yes, we could definitely avoid HU in parameter's name.

[matt]

But if we provide clear/accurate methods' name and keep it short

This is the goal.

In your example, I'd never use READ_MATNR4PLAN, I'd use READ_MATERIAL_LIST_FOR_PLAN. Because that's what I'm reading and why. I'd only use MATNR or 4 if the name was longer than 30 characters.

Incidentally, the other reason I use r_result is because the code assistant does that when you ask it to create a method! 😉

 

[olegbash599]

usually, my goal is to solve business problem. readable/robust code is just means to achieve it. Everyone has its' own goals.

yes, in 1st case it was example how to use

read_list_of_matnr2plan

and, yes, you are right, ABAP has some specifications compare with some other popular language programming

• bad-slow IDE due to server-typing (it will be always slower comparing to local typing)
• limitations for 30symbols in methods' names
• many more

it is not bad, but it has.

as for code assistant you can configure as you like... i have rv_val (just because it shorted - I am trying to keep it short)

[matt]

r_val is shorter... 😉

Anyway, let me clarify.

But if we provide clear/accurate methods' name and keep it short

This is the goal, in order to achieve the wider goal of making code more robust and readable, in order to solve business problems.

 

[vonglan]

I think, the most important rule is: avoid copies. In this case: pull identical parts outside of the IF / ELSE parts.