Clean ABAP: Writing Maintainable Code

" BAD: Unclear abbreviations
DATA: lv_c TYPE i,
      lv_t TYPE string,
      lt_d TYPE TABLE OF mara.

" GOOD: Meaningful names
DATA: lv_customer_count TYPE i,
      lv_travel_status TYPE string,
      lt_materials TYPE TABLE OF mara.

" BAD: Unclear what the method does
METHODS process.
METHODS do_it.
METHODS handle.

" GOOD: Verb + object describes the action
METHODS calculate_total_price.
METHODS validate_customer_data.
METHODS send_confirmation_email.

" BAD: No question recognizable
DATA lv_status TYPE abap_bool.
METHODS check_customer.

" GOOD: Questions with is_, has_, can_
DATA lv_is_active TYPE abap_bool.
DATA lv_has_items TYPE abap_bool.
DATA lv_can_be_deleted TYPE abap_bool.

METHODS is_customer_valid RETURNING VALUE(rv_valid) TYPE abap_bool.
METHODS has_open_orders RETURNING VALUE(rv_has_orders) TYPE abap_bool.

" BAD: Unclear responsibility
CLASS zcl_helper DEFINITION.
CLASS zcl_utils DEFINITION.
CLASS zcl_manager DEFINITION.

" GOOD: Clear responsibility in the name
CLASS zcl_invoice_calculator DEFINITION.
CLASS zcl_email_sender DEFINITION.
CLASS zcl_customer_validator DEFINITION.

" Interfaces with IF_ prefix
INTERFACE zif_payment_provider DEFINITION.
INTERFACE zif_logger DEFINITION.

" BAD: Method does too much
METHOD process_order.
  " 1. Validation (50 lines)
  IF customer_id IS INITIAL.
    " ... error handling
  ENDIF.
  " ... further validations

  " 2. Price calculation (80 lines)
  SELECT * FROM pricing_conditions...
  LOOP AT lt_items...
    " ... complex calculation
  ENDLOOP.

  " 3. Database update (40 lines)
  MODIFY ztable FROM TABLE lt_data.
  " ... further updates

  " 4. Send email (30 lines)
  " ... email logic
ENDMETHOD.


" GOOD: Split into focused methods
METHOD process_order.
  validate_order( is_order ).
  DATA(lv_total) = calculate_total_price( is_order-items ).
  save_order( is_order ).
  send_order_confirmation( is_order ).
ENDMETHOD.

METHOD validate_order.
  IF is_order-customer_id IS INITIAL.
    RAISE EXCEPTION TYPE zcx_invalid_order
      EXPORTING textid = zcx_invalid_order=>customer_missing.
  ENDIF.
  " ... further validations
ENDMETHOD.

METHOD calculate_total_price.
  rv_total = REDUCE #(
    INIT sum = 0
    FOR item IN it_items
    NEXT sum = sum + item-quantity * item-price
  ).
ENDMETHOD.

" BAD: Too many parameters
METHODS create_invoice
  IMPORTING
    iv_customer_id   TYPE kunnr
    iv_customer_name TYPE string
    iv_street        TYPE string
    iv_city          TYPE string
    iv_postal_code   TYPE string
    iv_country       TYPE land1
    iv_invoice_date  TYPE dats
    iv_due_date      TYPE dats
    iv_currency      TYPE waers
    iv_payment_terms TYPE string.

" GOOD: Use structure
TYPES: BEGIN OF ty_invoice_data,
         customer    TYPE ty_customer,
         invoice     TYPE ty_invoice_header,
         items       TYPE tt_invoice_items,
       END OF ty_invoice_data.

METHODS create_invoice
  IMPORTING
    is_invoice_data TYPE ty_invoice_data.

" BAD: EXPORTING for single values
METHODS get_customer_name
  IMPORTING iv_customer_id TYPE kunnr
  EXPORTING ev_name        TYPE string.

" Cumbersome call
get_customer_name(
  EXPORTING iv_customer_id = lv_id
  IMPORTING ev_name = lv_name
).

" GOOD: RETURNING for single values
METHODS get_customer_name
  IMPORTING iv_customer_id TYPE kunnr
  RETURNING VALUE(rv_name) TYPE string.

" Elegant call
DATA(lv_name) = get_customer_name( lv_customer_id ).

" BAD: Double negation
IF NOT is_invalid = abap_true.
  " ...
ENDIF.

IF NOT has_no_items( ).
  " ...
ENDIF.

" GOOD: Positive formulation
IF is_valid = abap_true.
  " ...
ENDIF.

IF has_items( ).
  " ...
ENDIF.

" BAD: Deep nesting
METHOD calculate_discount.
  IF customer IS NOT INITIAL.
    IF customer-is_active = abap_true.
      IF order-total > 1000.
        IF customer-loyalty_level >= 3.
          rv_discount = 15.
        ELSE.
          rv_discount = 10.
        ENDIF.
      ELSE.
        rv_discount = 5.
      ENDIF.
    ENDIF.
  ENDIF.
ENDMETHOD.

" GOOD: Guard clauses (early returns)
METHOD calculate_discount.
  " Check preconditions
  IF customer IS INITIAL.
    RETURN.
  ENDIF.

  IF customer-is_active = abap_false.
    RETURN.
  ENDIF.

  " Main logic - flat and readable
  IF order-total > 1000 AND customer-loyalty_level >= 3.
    rv_discount = 15.
    RETURN.
  ENDIF.

  IF order-total > 1000.
    rv_discount = 10.
    RETURN.
  ENDIF.

  rv_discount = 5.
ENDMETHOD.

" BAD: Long IF-ELSEIF chain
IF lv_status = 'N'.
  lv_text = 'New'.
ELSEIF lv_status = 'P'.
  lv_text = 'In Progress'.
ELSEIF lv_status = 'C'.
  lv_text = 'Completed'.
ELSEIF lv_status = 'X'.
  lv_text = 'Cancelled'.
ENDIF.

" GOOD: CASE is clearer
lv_text = SWITCH #( lv_status
  WHEN 'N' THEN 'New'
  WHEN 'P' THEN 'In Progress'
  WHEN 'C' THEN 'Completed'
  WHEN 'X' THEN 'Cancelled'
  ELSE 'Unknown'
).

" BAD: Return code based error handling
METHODS validate_order
  IMPORTING is_order TYPE ty_order
  EXPORTING ev_error TYPE string
  RETURNING VALUE(rv_success) TYPE abap_bool.

" Call with complicated check
IF validate_order(
     EXPORTING is_order = ls_order
     IMPORTING ev_error = lv_error
   ) = abap_false.
  MESSAGE lv_error TYPE 'E'.
ENDIF.


" GOOD: Exception based error handling
METHODS validate_order
  IMPORTING is_order TYPE ty_order
  RAISING   zcx_order_validation.

" Call with TRY-CATCH
TRY.
    validate_order( ls_order ).
  CATCH zcx_order_validation INTO DATA(lx_error).
    MESSAGE lx_error TYPE 'E'.
ENDTRY.

" Custom exception class with message texts
CLASS zcx_order_validation DEFINITION
  INHERITING FROM cx_static_check
  CREATE PUBLIC.

  PUBLIC SECTION.
    INTERFACES if_t100_message.

    CONSTANTS:
      BEGIN OF customer_not_found,
        msgid TYPE symsgid VALUE 'ZORDER',
        msgno TYPE symsgno VALUE '001',
        attr1 TYPE scx_attrname VALUE 'CUSTOMER_ID',
        attr2 TYPE scx_attrname VALUE '',
        attr3 TYPE scx_attrname VALUE '',
        attr4 TYPE scx_attrname VALUE '',
      END OF customer_not_found,

      BEGIN OF invalid_quantity,
        msgid TYPE symsgid VALUE 'ZORDER',
        msgno TYPE symsgno VALUE '002',
        attr1 TYPE scx_attrname VALUE 'QUANTITY',
        attr2 TYPE scx_attrname VALUE '',
        attr3 TYPE scx_attrname VALUE '',
        attr4 TYPE scx_attrname VALUE '',
      END OF invalid_quantity.

    DATA customer_id TYPE kunnr READ-ONLY.
    DATA quantity TYPE i READ-ONLY.

    METHODS constructor
      IMPORTING
        textid      LIKE if_t100_message=>t100key OPTIONAL
        previous    TYPE REF TO cx_root OPTIONAL
        customer_id TYPE kunnr OPTIONAL
        quantity    TYPE i OPTIONAL.
ENDCLASS.

" BAD: Swallowing errors
TRY.
    do_something( ).
  CATCH cx_root.
    " Do nothing - error ignored!
ENDTRY.

" BAD: Treating all errors the same
CATCH cx_root INTO DATA(lx_error).
  WRITE: 'An error occurred'.

" GOOD: Specific error handling
TRY.
    send_email( ls_email ).
  CATCH zcx_email_invalid_address INTO DATA(lx_invalid).
    " Inform user - recoverable
    MESSAGE lx_invalid TYPE 'W'.
  CATCH zcx_email_server_error INTO DATA(lx_server).
    " Logging for admin - retry later
    log_error( lx_server ).
    RAISE EXCEPTION lx_server.
ENDTRY.

" BAD: Comment explains obvious code
" Add 1 to counter
ADD 1 TO lv_counter.

" Check if customer is active
IF customer-is_active = abap_true.

" Loop over all items
LOOP AT lt_items INTO DATA(ls_item).


" GOOD: Comment explains WHY, not WHAT
" Temporary fix until release 2.0 - see Issue #4711
lv_offset = lv_offset + 1.

" Archived customers are processed separately in batch job
IF customer-is_active = abap_true.

"! <p class="shorttext synchronized">Calculates the total price including discount</p>
"!
"! @parameter it_items | <p class="shorttext synchronized">Order items</p>
"! @parameter iv_discount_percent | <p class="shorttext synchronized">Discount in percent (0-100)</p>
"! @parameter rv_total | <p class="shorttext synchronized">Total price after discount</p>
"! @raising zcx_calculation | <p class="shorttext synchronized">For invalid input values</p>
METHODS calculate_total
  IMPORTING
    it_items            TYPE tt_order_items
    iv_discount_percent TYPE i DEFAULT 0
  RETURNING
    VALUE(rv_total)     TYPE netwr
  RAISING
    zcx_calculation.

" BAD: Direct dependency - not testable
CLASS zcl_order_processor DEFINITION.
  PUBLIC SECTION.
    METHODS process_order
      IMPORTING is_order TYPE ty_order.
ENDCLASS.

CLASS zcl_order_processor IMPLEMENTATION.
  METHOD process_order.
    " Direct instantiation - hard to test
    DATA(lo_email_sender) = NEW zcl_email_sender( ).
    lo_email_sender->send( ... ).

    " Direct DB access - test modifies real data
    MODIFY ztable FROM ls_data.
  ENDMETHOD.
ENDCLASS.


" GOOD: Dependency Injection - testable
CLASS zcl_order_processor DEFINITION.
  PUBLIC SECTION.
    METHODS constructor
      IMPORTING
        io_email_sender TYPE REF TO zif_email_sender
        io_repository   TYPE REF TO zif_order_repository.

    METHODS process_order
      IMPORTING is_order TYPE ty_order.

  PRIVATE SECTION.
    DATA mo_email_sender TYPE REF TO zif_email_sender.
    DATA mo_repository   TYPE REF TO zif_order_repository.
ENDCLASS.

CLASS zcl_order_processor IMPLEMENTATION.
  METHOD constructor.
    mo_email_sender = io_email_sender.
    mo_repository   = io_repository.
  ENDMETHOD.

  METHOD process_order.
    " Uses injected dependencies
    mo_repository->save( is_order ).
    mo_email_sender->send_confirmation( is_order ).
  ENDMETHOD.
ENDCLASS.

" Test with mock objects
CLASS ltc_order_processor DEFINITION
  FOR TESTING
  DURATION SHORT
  RISK LEVEL HARMLESS.

  PRIVATE SECTION.
    DATA mo_cut TYPE REF TO zcl_order_processor.  " Class Under Test
    DATA mo_email_mock TYPE REF TO ltd_email_sender.
    DATA mo_repo_mock TYPE REF TO ltd_order_repository.

    METHODS setup.
    METHODS test_process_sends_email FOR TESTING.
ENDCLASS.

CLASS ltc_order_processor IMPLEMENTATION.
  METHOD setup.
    " Create test doubles
    mo_email_mock = NEW ltd_email_sender( ).
    mo_repo_mock = NEW ltd_order_repository( ).

    " Instantiate class under test with mocks
    mo_cut = NEW zcl_order_processor(
      io_email_sender = mo_email_mock
      io_repository   = mo_repo_mock
    ).
  ENDMETHOD.

  METHOD test_process_sends_email.
    " Given
    DATA(ls_order) = VALUE ty_order(
      order_id = '12345'
      customer_email = '[email protected]'
    ).

    " When
    mo_cut->process_order( ls_order ).

    " Then
    cl_abap_unit_assert=>assert_true(
      act = mo_email_mock->was_send_called
      msg = 'Email should be sent'
    ).
  ENDMETHOD.
ENDCLASS.

" Test double for email sender
CLASS ltd_email_sender DEFINITION FOR TESTING.
  PUBLIC SECTION.
    INTERFACES zif_email_sender.
    DATA was_send_called TYPE abap_bool.
ENDCLASS.

CLASS ltd_email_sender IMPLEMENTATION.
  METHOD zif_email_sender~send_confirmation.
    was_send_called = abap_true.
  ENDMETHOD.
ENDCLASS.

METHOD calc.
  DATA: lt_d TYPE TABLE OF mara,
        lv_t TYPE p DECIMALS 2,
        lv_c TYPE i.

  SELECT * FROM mara INTO TABLE lt_d
    WHERE matnr IN @s_matnr.

  LOOP AT lt_d INTO DATA(ls_d).
    IF ls_d-mtart = 'FERT' OR ls_d-mtart = 'HALB'.
      SELECT SINGLE netpr FROM a]
        INTO @DATA(lv_p)
        WHERE matnr = @ls_d-matnr.
      IF sy-subrc = 0.
        lv_t = lv_t + lv_p.
        lv_c = lv_c + 1.
      ENDIF.
    ENDIF.
  ENDLOOP.

  IF lv_c > 0.
    rv_result = lv_t / lv_c.
  ENDIF.
ENDMETHOD.

"! <p class="shorttext synchronized">Calculates average price for finished goods</p>
"!
"! @parameter it_material_range | <p class="shorttext synchronized">Material number selection</p>
"! @parameter rv_average_price | <p class="shorttext synchronized">Average price</p>
METHOD calculate_average_price_for_finished_goods.
  DATA(lt_materials) = get_finished_goods( it_material_range ).

  IF lt_materials IS INITIAL.
    RETURN.
  ENDIF.

  DATA(lt_prices) = get_prices_for_materials( lt_materials ).

  rv_average_price = calculate_average( lt_prices ).
ENDMETHOD.


METHOD get_finished_goods.
  SELECT matnr
    FROM mara
    WHERE matnr IN @it_material_range
      AND mtart IN ('FERT', 'HALB')
    INTO TABLE @rt_materials.
ENDMETHOD.


METHOD get_prices_for_materials.
  SELECT matnr, netpr
    FROM a]
    FOR ALL ENTRIES IN @it_materials
    WHERE matnr = @it_materials-matnr
    INTO TABLE @rt_prices.
ENDMETHOD.


METHOD calculate_average.
  CHECK it_prices IS NOT INITIAL.

  DATA(lv_total) = REDUCE netwr(
    INIT sum = CONV netwr( 0 )
    FOR price IN it_prices
    NEXT sum = sum + price-netpr
  ).

  rv_average = lv_total / lines( it_prices ).
ENDMETHOD.