abap-code-review-helper

ABAP Coding Pitfalls

These requirements describe what the code must do. For reviews, report violations; for authorized changes, make the code meet the applicable requirements. Report important unknowns, not passed-check lists unless asked. Never claim checks or tests that were not performed.

Review safeguards

1. A finding must describe a reachable failure in the current code. A hypothetical future change is not a current defect.
2. Existing protections must be traced before reporting a missing check. An enclosing branch, earlier exit, full assignment, or earlier compatible sort can already satisfy the requirement.
3. A safe equivalent must be accepted. The code need not use the reviewer's preferred syntax.
4. A claim disproved during investigation must be removed from the findings and severity totals.
5. Quoted code, types, keys, and signatures must match inspected source. Missing information must remain unknown.
6. Security findings require an actual input/control path to a security-sensitive operation. Dynamic ABAP conditions are not automatically SQL injection.
7. Correctness defects, conditional risks, measured performance problems, and maintainability suggestions must be kept separate.

Examples of mistakes that pass syntax checks

These are examples, not the complete list. The later requirements also apply wherever relevant to the code.

• Old data must not be used after a failed read. After READ TABLE ... INTO or SELECT SINGLE, sy-subrc must be checked or saved before another operation can overwrite it. The result must be checked before the target is used. If no row was found, the target work area must not be used as a successful result: it may still hold data from the previous read. The return code must determine whether a row was found, not whether the work area is initial; a real row can contain blank or zero values.
• Temporary loop variables must be cleared or fully assigned before use on each iteration. A skipped assignment must not leave the previous row's value in use. Running totals and other values needed across iterations must not be cleared prematurely. Clearing is unnecessary when the value is always replaced completely before use.
• Internal-table inserts and updates must use the statement that matches the intended action.
  • INSERT ... INTO TABLE adds a row. MODIFY TABLE changes an existing row found by the primary table key; it must not be used to insert a missing row. MODIFY ... INDEX changes the row at that index.
  • Duplicate handling must match the actual statement and table keys. A duplicate can set a failure return code, raise an exception, or cause a dump. sy-subrc must be checked when the statement sets it; exceptions must be handled where required. This also applies to bulk inserts, constructors, and unique secondary keys.
  • INSERT must not be replaced with MODIFY just to hide duplicates. The business rule must determine whether a duplicate is rejected, skipped, merged, or updated.
• Database INSERT, UPDATE, and MODIFY must match the allowed create/change behavior.
  • INSERT is for create-only and UPDATE for change-only. Database MODIFY must be used only when insert-or-update by primary key is intended. Unlike internal-table MODIFY TABLE, it can insert a missing row.
  • Duplicate primary keys and conflicts with unique secondary indexes must be handled. Depending on the statement, these can return a failure code, raise an exception, or cause a dump. MODIFY can still fail on a unique secondary index.
  • A duplicate must not be ignored unless the business rule allows it. A preceding existence check is not enough: another session can insert the row before this session does.
• The table must be sorted correctly before BINARY SEARCH. On a standard table, the search fields must be the leading sort fields, in ascending order and in the same sequence as the search key. A descending sort or AS TEXT sort is not a substitute. Appends or changes must not break that order before the search. A suitable declared sorted or hashed key is preferable when it removes the need to maintain this order manually.
• An index-based parallel cursor must have the correct sort, start index, and exit check.
  • The tables must be ordered by the grouping fields as required by the algorithm. The first matching inner row's sy-tabix must be saved before another operation can overwrite it, and the inner loop must start FROM that saved index.
  • The inner loop must exit when any grouping field differs (NE/<>). Comparing only part of a composite group key or continuing into the next group is incorrect.
  • Duplicate keys in both tables must be handled. An inner group must remain available to any later outer row that also needs it. The last successful index must not be reused after a failed search.
• The FAE table must be checked for empty before the SELECT. SELECT ... FOR ALL ENTRIES IN @lt_keys must be guarded by IF lt_keys IS NOT INITIAL or an earlier return/skip when it is empty. With an empty FAE table, the entire explicit WHERE condition can be ignored, not just conditions using lt_keys. The query can then read every row allowed by implicit restrictions such as the current client. A skipped SELECT must not leave its previous result in use accidentally.
• An empty range in WHERE field IN @range_table means no restriction on that field." If empty input should return no rows, the SELECT must be skipped or an empty result returned explicitly.
• The number of keys passed through IN range must stay within the database's limits.
  • A large range in WHERE field IN @lt_range can exceed SQL statement-size or parameter limits and cause a database error or dump.
  • Large exact-key lists must use an approach suitable for the expected volume, such as FAE with a nonempty-table check, a supported join, or smaller key batches. Neither FAE nor a hardcoded batch size is safe at every size on every system.
  • The selection must keep its intended meaning. FAE removes duplicate result rows. Exclusions, intervals, and patterns must not be replaced blindly with equality checks or split into separate queries whose results are combined.
• SELECT SINGLE must not choose the latest or preferred row from several matches. It is suitable when the key is unique or any matching row is acceptable, including an existence check. Selecting the first, latest, or top-N rows must use ORDER BY with an extra sort field when needed to resolve ties. Database result order must not be relied on without ORDER BY.
• Database reads inside loops must be avoided unless their number and cost are justified. This includes SELECTs inside methods and function modules called by the loop. Reading needed rows together and looking them up by table key is preferable. Per-row reads may be appropriate for a justified buffered lookup or another bounded access pattern.
• SELECTs must retrieve only needed columns by default. SELECT * must not be the default: extra columns increase transfer and memory use. Whole-row reads are acceptable when the complete row is needed and the expected row count and row size are acceptable.
• A field symbol must be assigned before it is used. When assignment can fail or be skipped, access must be guarded by IF <row> IS ASSIGNED or an equivalent successful-assignment check. The unassigned case must skip, return, or raise an error before dereferencing; otherwise the code can dump with GETWA_NOT_ASSIGNED. Inside a successful LOOP ... ASSIGNING, another check is unnecessary unless later code unassigns the symbol or invalidates its row.
• The result of dynamic ASSIGN must be checked before the field symbol is used. sy-subrc must be checked or saved before another operation can overwrite it. A failed assignment can leave the field symbol pointing at an older object. IS ASSIGNED alone does not establish that the new assignment worked. When an old assignment must not survive, the symbol must be unassigned before the attempt. Failure must be handled before the symbol is used.
• Missing rows in table expressions must be handled. itab[ ... ] can raise CX_SY_ITAB_LINE_NOT_FOUND. Where absence is expected, that exception must be handled or an appropriate default used. A missing mandatory row must produce an error rather than silently becoming a blank value. Unrelated errors must not be hidden by a broad catch.
• A helper, exit, or BAdI must not commit the caller's work without permission from the calling application. COMMIT WORK can save unrelated pending changes made by the caller. It is allowed only where the application or framework explicitly permits that routine to end the transaction.
• Database changes must be protected against conflicting changes by other sessions. Between reading a row and acting on it, another session can change it. The code must use the appropriate lock, a single conditional database operation, or a version/value check that rejects a conflicting update. An existence check alone does not make concurrent creation safe.