Community
Application Development and Automation Discussions
Join the discussions or start your own on all things application development, including tools and APIs, programming models, and keeping your skills sharp.
cancel
Turn on suggestions
Auto-suggest helps you quickly narrow down your search results by suggesting possible matches as you type.
Showing results for 
Search instead for 
Did you mean: 
Read only

Improve ABAP Code

former_member799034
Explorer

‎2022 Apr 25 11:22 AM

0 Likes
2,498

Hi,

Experts, Is there any better way to do the following coding? The main purpose of the following code is to distribute total stock to different periods from front to back, Every period has already calculated the total receipt quantity.

thanks!

*Define

*-----------------------------------------------------------------
DEFINE MOVE_SUB.
d_menge_i = d_menge_i_sub. clear d_menge_i_sub.
END-OF-DEFINITION.

DEFINE SUB_M_D.
d_menge_i_sub = d_menge_i - &1.
END-OF-DEFINITION.

DEFINE MOVE_M_D.
&1 = d_menge_i.

END-OF-DEFINITION.

D_MENGE_I = D_MENGE_I_E - D_MENGE_I_1. " total stock - first month receipt quanity

IF D_MENGE_I <= 0. "1M
IT_DATA-MENGE_1 = D_MENGE_I_E.
ELSE.
IT_DATA-MENGE_1 = D_MENGE_I_1.
SUB_M_D D_MENGE_I_2.
IF D_MENGE_I_SUB <= 0. "1-2M
MOVE_M_D IT_DATA-MENGE_2.
ELSE.
IT_DATA-MENGE_2 = D_MENGE_I_2.
MOVE_SUB.
SUB_M_D D_MENGE_I_3.
IF D_MENGE_I_SUB <= 0. "2-3M
MOVE_M_D IT_DATA-MENGE_3.
ELSE.
IT_DATA-MENGE_3 = D_MENGE_I_3.
MOVE_SUB.
SUB_M_D D_MENGE_I_4.
IF D_MENGE_I_SUB <= 0. "4-6M
MOVE_M_D IT_DATA-MENGE_4.
ELSE.
IT_DATA-MENGE_4 = D_MENGE_I_4.
MOVE_SUB.
SUB_M_D D_MENGE_I_51.
IF D_MENGE_I_SUB <= 0. "7M
MOVE_M_D IT_DATA-MENGE_51.
ELSE.
IT_DATA-MENGE_51 = D_MENGE_I_51.
MOVE_SUB.
SUB_M_D D_MENGE_I_52.
IF D_MENGE_I_SUB <= 0. "8M
MOVE_M_D IT_DATA-MENGE_52.
ELSE.
IT_DATA-MENGE_52 = D_MENGE_I_52.
MOVE_SUB.
SUB_M_D D_MENGE_I_53.
IF D_MENGE_I_SUB <= 0. "9M
MOVE_M_D IT_DATA-MENGE_53.
ELSE.
IT_DATA-MENGE_53 = D_MENGE_I_53.
MOVE_SUB.
SUB_M_D D_MENGE_I_54.
IF D_MENGE_I_SUB <= 0. "10M
MOVE_M_D IT_DATA-MENGE_54.
ELSE.
IT_DATA-MENGE_54 = D_MENGE_I_54.
MOVE_SUB.
SUB_M_D D_MENGE_I_55.
IF D_MENGE_I_SUB <= 0. "11M
MOVE_M_D IT_DATA-MENGE_55.
ELSE.
IT_DATA-MENGE_55 = D_MENGE_I_55.
MOVE_SUB.
SUB_M_D D_MENGE_I_56.
IF D_MENGE_I_SUB <= 0. "12M
MOVE_M_D IT_DATA-MENGE_56.
ELSE.
IT_DATA-MENGE_56 = D_MENGE_I_56.
MOVE_SUB.
SUB_M_D D_MENGE_I_6.
IF D_MENGE_I_SUB <= 0. "1-2Y
MOVE_M_D IT_DATA-MENGE_6.
ELSE.
IT_DATA-MENGE_6 = D_MENGE_I_6.
MOVE_SUB.
SUB_M_D D_MENGE_I_7.
IF D_MENGE_I_SUB <= 0. "2-3Y
MOVE_M_D IT_DATA-MENGE_7.
ELSE. ">3Y
IT_DATA-MENGE_7 = D_MENGE_I_7.
IT_DATA-MENGE_8 = D_MENGE_I_SUB.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.
ENDIF.

APPEND IT_DATA.

9 REPLIES 9
Read only

Jeansy
Active Contributor

‎2022 Apr 25 11:31 AM

2,394

Hi,

please use the CODE-function in your posting, that makes code much better readable.

I already see the definition of macros. I would not use macros anymore as they are deprecated.

Kind regards
Jens

Read only

former_member799034
Explorer

‎2022 Apr 25 11:40 AM

0 Likes
2,394 

*Define
*-----------------------------------------------------------------
DEFINE MOVE_SUB.
  d_menge_i = d_menge_i_sub. clear d_menge_i_sub.
END-OF-DEFINITION.

DEFINE SUB_M_D.
  d_menge_i_sub = d_menge_i - &1.
END-OF-DEFINITION.

DEFINE MOVE_M_D.
  &1 = d_menge_i.
END-OF-DEFINITION.

      D_MENGE_I = D_MENGE_I_E - D_MENGE_I_1.  "Total stock - first month receipt quanity

      IF D_MENGE_I <= 0.                            "1M
        IT_DATA-MENGE_1 = D_MENGE_I_E.
      ELSE.
        IT_DATA-MENGE_1 = D_MENGE_I_1.

        SUB_M_D D_MENGE_I_2.
        IF D_MENGE_I_SUB <= 0.                          "1-2M
          MOVE_M_D IT_DATA-MENGE_2.
        ELSE.
          IT_DATA-MENGE_2 = D_MENGE_I_2.
          MOVE_SUB.
          SUB_M_D D_MENGE_I_3.
          IF D_MENGE_I_SUB <= 0.                        "2-3M
            MOVE_M_D IT_DATA-MENGE_3.
          ELSE.
            IT_DATA-MENGE_3 = D_MENGE_I_3.
            MOVE_SUB.
            SUB_M_D D_MENGE_I_4.
            IF D_MENGE_I_SUB <= 0.                      "4-6M
              MOVE_M_D IT_DATA-MENGE_4.
            ELSE.
              IT_DATA-MENGE_4 = D_MENGE_I_4.
              MOVE_SUB.
              SUB_M_D D_MENGE_I_51.
              IF D_MENGE_I_SUB <= 0.                      "7M
                MOVE_M_D IT_DATA-MENGE_51.
              ELSE.
                IT_DATA-MENGE_51 = D_MENGE_I_51.
                MOVE_SUB.
                SUB_M_D D_MENGE_I_52.
                IF D_MENGE_I_SUB <= 0.                      "8M
                  MOVE_M_D IT_DATA-MENGE_52.
                ELSE.
                  IT_DATA-MENGE_52 = D_MENGE_I_52.
                  MOVE_SUB.
                  SUB_M_D D_MENGE_I_53.
                  IF D_MENGE_I_SUB <= 0.                      "9M
                    MOVE_M_D IT_DATA-MENGE_53.
                  ELSE.
                    IT_DATA-MENGE_53 = D_MENGE_I_53.
                    MOVE_SUB.
                    SUB_M_D D_MENGE_I_54.
                    IF D_MENGE_I_SUB <= 0.                      "10M
                      MOVE_M_D IT_DATA-MENGE_54.
                    ELSE.
                      IT_DATA-MENGE_54 = D_MENGE_I_54.
                      MOVE_SUB.
                      SUB_M_D D_MENGE_I_55.
                      IF D_MENGE_I_SUB <= 0.                      "11M
                        MOVE_M_D IT_DATA-MENGE_55.
                      ELSE.
                        IT_DATA-MENGE_55 = D_MENGE_I_55.
                        MOVE_SUB.
                        SUB_M_D D_MENGE_I_56.
                        IF D_MENGE_I_SUB <= 0.                      "12M
                          MOVE_M_D IT_DATA-MENGE_56.
                        ELSE.
                          IT_DATA-MENGE_56 = D_MENGE_I_56.
                          MOVE_SUB.
                          SUB_M_D D_MENGE_I_6.
*                          IF D_MENGE_I_SUB <= 0 .                 "7-12M
*                            MOVE_M_D IT_DATA-MENGE_5.
*                          ELSE.
*                            IT_DATA-MENGE_5 = D_MENGE_I_5.
*                            MOVE_SUB.
*                            SUB_M_D D_MENGE_I_6.
                          IF D_MENGE_I_SUB <= 0.                  "1-2Y
                            MOVE_M_D IT_DATA-MENGE_6.
                          ELSE.
                            IT_DATA-MENGE_6 = D_MENGE_I_6.
                            MOVE_SUB.
                            SUB_M_D D_MENGE_I_7.
                            IF D_MENGE_I_SUB <= 0.                "2-3Y
                              MOVE_M_D IT_DATA-MENGE_7.
                            ELSE.                                 ">3Y
                              IT_DATA-MENGE_7 = D_MENGE_I_7.
                              IT_DATA-MENGE_8 = D_MENGE_I_SUB.
                            ENDIF.
                          ENDIF.
                        ENDIF.
                      ENDIF.
*                      ENDIF.
                    ENDIF.
                  ENDIF.
                ENDIF.
              ENDIF.
            ENDIF.
          ENDIF.
        ENDIF.
      ENDIF.

      IT_DATA-MENGE_5 = IT_DATA-MENGE_51 + IT_DATA-MENGE_52 +
                        IT_DATA-MENGE_53 + IT_DATA-MENGE_54 +
                        IT_DATA-MENGE_55 + IT_DATA-MENGE_56.
      APPEND IT_DATA.
Read only

FredericGirod
Active Contributor

‎2022 Apr 25 12:16 PM

2,394

as jzaehringer, macro is deprecated

use Oo, with a simple recursive method

You should also use ABAP Unit to test your code

Read only

former_member799034
Explorer

‎2022 Apr 25 1:01 PM

0 Likes
2,394

Well, these codes may have a long history, It's done ten years ago, Recently my customer has some new business request， So when I start to work and I look at this long long code， I thought maybe I can make a little bit improve，make it looks more clear， But I don't want to rewrite all code.

Read only

FredericGirod
Active Contributor

‎2022 Apr 25 1:16 PM

2,394

https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#how-to-refactor-legacy-code

Follow the boy scout rule to your daily work routine: always leave the code you edit a little cleaner than you found it. Don't obsess with this by sinking hours into "cleaning the campsite", just spend a couple of minutes extra and observe how the improvements accumulate over time.

You have no choice, to keep a cleaner place, you have to fully rewrite it.

Read only

matt
Active Contributor

‎2022 Apr 25 1:16 PM

2,394
william.tse
  1. Before making adding the new logic, get the code into a test harness. I.e. write ABAP Unit Tests and make sure it passes them all.
  2. Replace the macros with calls to methods - make sure all your unit tests pass
  3. Rename the variables into something meaningful d_menge_i_e and d_menge_i_1 are not terribly meaningful
  4. Rewrite the code so that what it does is readily understandable. Run the unit tests again and fix until it passes them all.
  5. Add a new unit test for the new requirement.
  6. Write and fix until the new test is passed by the code.

Bit the bullet. Rewrite the code. It's awful - expensive and shoddy. You've got a ten year technical debt there. If you pay off the debt now, then you'll be able to do the next change far more quickly and easily. Or your successor will.

Read only

FredericGirod
Active Contributor

‎2022 Apr 25 1:28 PM

2,394

if you could post little bit more of this code, we could help you for ABAP Unit (could scary little bit the first time you see it, but it will give you great Abap power 😉 )

Read only

Sandra_Rossi
Active Contributor

‎2022 Apr 25 3:25 PM

2,394

I agree with Matthew, the code is awful, I can't understand what it does, use meaningful names, etc. (do what Matthew suggested).

Read only

former_member799034
Explorer

‎2022 Apr 26 2:32 AM

0 Likes
2,394

OK, I will try it, thanks for your advice