From 84b28c0e4a33c301f5e8eec31a5f480d467d463a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20J=C3=BCliger?= Date: Tue, 24 May 2022 11:12:58 +0000 Subject: [PATCH 1/5] Remove superflous pseudo comment --- src/checks/y_check_function.clas.abap | 1 - 1 file changed, 1 deletion(-) diff --git a/src/checks/y_check_function.clas.abap b/src/checks/y_check_function.clas.abap index 44206684..6b9a8c5d 100644 --- a/src/checks/y_check_function.clas.abap +++ b/src/checks/y_check_function.clas.abap @@ -17,7 +17,6 @@ CLASS y_check_function IMPLEMENTATION. METHOD constructor. super->constructor( ). - settings-pseudo_comment = '"#EC CI_FUNCTION' ##NO_TEXT. settings-disable_threshold_selection = abap_true. settings-threshold = 0. settings-documentation = |{ c_docs_path-checks }function-routine.md|. From b83826ab5b2f6317de4bc0be5ed53f37e4aaf24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20J=C3=BCliger?= Date: Mon, 8 Aug 2022 14:18:25 +0200 Subject: [PATCH 2/5] Improve documentation Particular focus: - consistent language - consistent capitalization (lots of unnecessary capitalization removed) - proper use of technical terms (e.g. "static" vs. "dynamic") --- docs/checks/function-routine.md | 20 ++++++------------ docs/checks/prefer-line-exists.md | 6 +++--- .../prefer-pragmas-to-pseudo-comments.md | 21 +++++++------------ 3 files changed, 17 insertions(+), 30 deletions(-) diff --git a/docs/checks/function-routine.md b/docs/checks/function-routine.md index 9f8b5283..ef2ffe41 100644 --- a/docs/checks/function-routine.md +++ b/docs/checks/function-routine.md @@ -2,30 +2,22 @@ ## FUNCTION Routine Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the usage of FUNCTION Modules (procedural programming) since with the release of Object-Oriented ABAP this syntax became obsolete. +This check searches for the usage of function modules since non-RFC-enabled function modules became obsolete with the release of object-oriented ABAP. ### How does the check work? -This check searches for Function Modules within a function group. However, since Remote Function Calls (RFC) can only be executed via FUNCTION MODULES, these ones (Function Modules with RFC enabled) will not be caught by this check. - -Note: This check does not search for the `CALL FUNCTION` statement within your source code (e.g. in a method or program). +When a function group is checked, this check will determine the RFC status of all function modules inside it and report a finding for every function module that is not RFC-enabled. ### How to solve the issue? -Use classes and methods instead (Object Oriented ABAP). +Use classes and methods instead as these are the intended tool for modularization in object-oriented ABAP. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_FUNCTION` which should be placed after the FUNCTION statement: - -```abap -FUNCTION my_function. "#EC CI_FUNCTION - " Function content -ENDFUNCTION. -``` +There are no pseudo comments for this check since pseudo comments cannot be used in the definition part of a function module. ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-object-orientation-to-procedural-programming) +* [Clean ABAP - Prefer object orientation to procedural programming](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-object-orientation-to-procedural-programming) diff --git a/docs/checks/prefer-line-exists.md b/docs/checks/prefer-line-exists.md index 497ddd40..f043f4d9 100644 --- a/docs/checks/prefer-line-exists.md +++ b/docs/checks/prefer-line-exists.md @@ -2,9 +2,9 @@ ## Prefer LINE_EXISTS or LINE_INDEX to READ TABLE or LOOP AT -### What is the Intent of the Check? +### What is the intent of the check? -Prefer `LINE_EXISTS` or `LINE_INDEX` over `READ TABLE` or `LOOP AT` as they avoid needlessly longer statements. +Prefer `LINE_EXISTS` or `LINE_INDEX` over `READ TABLE` or `LOOP AT` statements as they express the intent of the table access more specifically. ### How to solve the issue? @@ -54,4 +54,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-line_exists-to-read-table-or-loop-at) +* [Clean ABAP - Prefer `line_exists` to `READ TABLE` or `LOOP AT`s](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-line_exists-to-read-table-or-loop-at) diff --git a/docs/checks/prefer-pragmas-to-pseudo-comments.md b/docs/checks/prefer-pragmas-to-pseudo-comments.md index bae177e4..e1139edc 100644 --- a/docs/checks/prefer-pragmas-to-pseudo-comments.md +++ b/docs/checks/prefer-pragmas-to-pseudo-comments.md @@ -2,38 +2,33 @@ ## Prefer Pragmas to Pseudo Comments -### What is the Intent of the Check? +### What is the intent of the check? -Based on the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-pragmas-to-pseudo-comments): -> Prefer pragmas to pseudo comments to suppress irrelevant warnings and errors identified by the ATC. Pseudo comments have mostly become obsolete and have been replaced by pragmas - -:bulb: Only the Pseudo Comments and Pragmas available in the `SLIN_DESC` table are in scope. - -:wan: Code Pal does not support Pragmas. +In cases where pragmas are possible (i.e. for findings from the ABAP syntax check and from the Extended Program Check (SLIN)), they should be preferred over pseudo comments as they are more precise with respect to the statement they refer to since they are placed inside of the statement instead of after it. ### How to solve the issue? -Change the `"#EC ` (Pseudo Comment) to `##` (Pragma). +Change the `"#EC ` of the pseudo comment to `##` and move the new pragma inside of the statement it refers to if the pseudo comment was placed after the terminating period. ### What to do in case of exception? -This Check cannot be exempt. +This check has no associated pseudo comment or pragma because pseudo comments can only refer to statements, not to other comments. ### Example Before the check: ```abap - DATA a TYPE string. "#EC NEEDED + DATA a TYPE string. "#EC NEEDED ``` After the check: ```abap - DATA a TYPE string. ##NEEDED + DATA a TYPE string ##NEEDED. ``` ### Further Readings & Knowledge -* [Clean ABAP: Prefer pragmas to pseudo comments](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-pragmas-to-pseudo-comments) -* [ABAP - Keyword Documentation: Pseudo Comments for the Extended Program Check](https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/abenpseudo_comment_slin.htm) +* [Clean ABAP - Prefer pragmas to pseudo comments](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-pragmas-to-pseudo-comments) +* [ABAP Keyword Documentation: Pseudo Comments for the Extended Program Check](https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/abenpseudo_comment_slin.htm) From fe6af1af7309ea21cca02857b5d875bbf1dc0231 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20J=C3=BCliger?= Date: Mon, 8 Aug 2022 14:18:41 +0200 Subject: [PATCH 3/5] Improve documentation (2nd part) --- docs/checks/avoid-default-key.md | 31 +++++----- docs/checks/boolean-input-parameter.md | 6 +- docs/checks/call-method-usage.md | 13 +++-- docs/checks/chain-declaration-usage.md | 33 ++++++----- docs/checks/check-in-loop.md | 29 +++++++--- docs/checks/check-statement-position.md | 32 ++++++---- docs/checks/collect.md | 16 +++-- docs/checks/comment-position.md | 20 ++++--- docs/checks/comment-type.md | 10 ++-- docs/checks/comment-usage.md | 15 ++--- docs/checks/constants-interface.md | 61 ++++++++++++-------- docs/checks/cx-root-usage.md | 31 +++------- docs/checks/cyclomatic-complexity.md | 21 +++---- docs/checks/db-access-in-ut.md | 24 +++----- docs/checks/deprecated-classes.md | 15 ++--- docs/checks/deprecated-key-word.md | 37 +++++++----- docs/checks/empty-catch.md | 8 ++- docs/checks/empty-if-branches.md | 9 ++- docs/checks/empty-procedure.md | 13 +++-- docs/checks/equals-sign-chaining.md | 15 ++--- docs/checks/external-call-in-ut.md | 9 ++- docs/checks/form-routine.md | 10 ++-- docs/checks/interface-in-class.md | 10 ++-- docs/checks/magic-number.md | 19 +++--- docs/checks/maximum-nesting-depth.md | 10 ++-- docs/checks/message-easy-to-find.md | 9 ++- docs/checks/message-translation.md | 16 +++-- docs/checks/method-output-parameter.md | 4 +- docs/checks/method-return-bool.md | 10 ++-- docs/checks/non-class-exception.md | 21 ++++--- docs/checks/number-attributes.md | 12 ++-- docs/checks/number-events.md | 12 ++-- docs/checks/number-executable-statements.md | 26 ++++++--- docs/checks/number-interfaces.md | 12 ++-- docs/checks/number-methods.md | 12 ++-- docs/checks/number-output-parameter.md | 15 ++++- docs/checks/number-public-attributes.md | 14 +++-- docs/checks/omit-optional-exporting.md | 19 +++--- docs/checks/optional-parameters.md | 10 ++-- docs/checks/prefer-case-to-elseif.md | 18 +++--- docs/checks/prefer-is-not-to-not-is.md | 5 +- docs/checks/prefer-new-to-create-object.md | 9 ++- docs/checks/prefer-returning-to-exporting.md | 7 +-- docs/checks/pseudo-comment-usage.md | 8 +-- docs/checks/receiving-usage.md | 13 +++-- docs/checks/returning-name.md | 9 ++- docs/checks/scope-of-variable.md | 22 ++----- docs/checks/self-reference.md | 8 +-- docs/checks/sub-assign-read-table.md | 10 ++-- docs/checks/test-seam-usage.md | 5 +- docs/checks/text-assembly.md | 4 +- docs/checks/unit-test-coverages.md | 8 +-- docs/checks/unit_test_assert.md | 11 ++-- pages/how-to-install.md | 8 --- 54 files changed, 439 insertions(+), 395 deletions(-) diff --git a/docs/checks/avoid-default-key.md b/docs/checks/avoid-default-key.md index a97653a9..06abd111 100644 --- a/docs/checks/avoid-default-key.md +++ b/docs/checks/avoid-default-key.md @@ -1,27 +1,26 @@ [code pal for ABAP](../../README.md) > [Documentation](../check_documentation.md) > [Avoid DEFAULT KEY](avoid-default-key.md) -## Avoid DEFAULT KEY +## Avoid default table keys -### What is the Intent of the Check? +### What is the intent of the check? -> Default keys are often only added to get the newer functional statements working. The keys themselves in fact are usually superfluous and waste resources for nothing. They can even lead to obscure mistakes because they ignore numeric data types. The `SORT` and `DELETE ADJACENT` statements without explicit field list will resort to the primary key of the internal table, which in case of usage of `DEFAULT KEY` can lead to very unexpected results when having e.g. numeric fields as component of the key, in particular in combination with `READ TABLE ... BINARY` etc. +Default table keys (declared by the `WITH DEFAULT KEY` clause in a table definition) can lead to obscure mistakes because they ignore numeric data types and they make it the intent of the key unclear. `SORT` and `DELETE ADJACENT` statements without explicit field list will resort to the primary key of the internal table, which in the case of default keys can lead to unexpected results, in particular in combination with `READ TABLE ... BINARY SEARCH` statements. -Source: [Clean ABAP - Avoid DEFAULT KEY](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-default-key). - -Therefore, this check searches for the internal table definitions which forces the use of the default key. +Therefore, this check searches for internal table definitions that declare a default table key. ### How to solve the issue? -> Either specify the key components explicitly -> ```abap -> DATA itab1 TYPE STANDARD TABLE OF row_type WITH NON-UNIQUE KEY comp1 comp2. -> ``` -> or resort to `EMPTY KEY` if you don't need a key at all. -> ```abap -> DATA itab1 TYPE STANDARD TABLE OF row_type WITH EMPTY KEY. -> ``` +Either specify the key components explicitly +```abap +DATA itab1 TYPE STANDARD TABLE OF row_type WITH NON-UNIQUE KEY comp1 comp2. +``` +or resort to `EMPTY KEY` if you don't need a key at all. +```abap +DATA itab1 TYPE STANDARD TABLE OF row_type WITH EMPTY KEY. +``` +If you declare an explicit key, also consider whether you actually should use a sorted or hashed key. -Source: [Clean ABAP - Avoid DEFAULT KEY](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-default-key). +See also: [Clean ABAP - Avoid DEFAULT KEY](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-default-key). ### What to do in case of exception? @@ -34,7 +33,7 @@ CLASS-DATA itab1 TYPE STANDARD TABLE OF row_type WITH DEFAULT KEY. "#EC DEFAULT_ DATA itab1 TYPE STANDARD TABLE OF row_type WITH DEFAULT KEY. "#EC DEFAULT_KEY ``` ```abap -TYPES: BEGIN OF type1, ' ) +TYPES: BEGIN OF type1, non_unique TYPE STANDARD TABLE OF row_type WITH NON-UNIQUE KEY object, default_key TYPE STANDARD TABLE OF row_type WITH DEFAULT KEY, "#EC DEFAULT_KEY' empty_key TYPE STANDARD TABLE OF row_type WITH EMPTY KEY, diff --git a/docs/checks/boolean-input-parameter.md b/docs/checks/boolean-input-parameter.md index 3fd5462e..43c193ac 100644 --- a/docs/checks/boolean-input-parameter.md +++ b/docs/checks/boolean-input-parameter.md @@ -2,11 +2,11 @@ ## Boolean Input Parameter -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the usage of boolean input parameters in a method signature. These parameters, could be an indicator of bad design where the Single Responsibility Principle is not followed (the method might be doing several things instead of a single thing). +This check searches for boolean input parameters in method signatures. These parameters could be an indicator that the [single-responsibility principle (SRP)](https://en.wikipedia.org/wiki/Single-responsibility_principle) is not followed since the method might be doing several things at once. -REMARK: Setter methods using boolean input variables are acceptable. +Setter methods using boolean input variables are acceptable when the variable being set is a boolean. ### How to solve the issue? diff --git a/docs/checks/call-method-usage.md b/docs/checks/call-method-usage.md index 84750010..0ac83121 100644 --- a/docs/checks/call-method-usage.md +++ b/docs/checks/call-method-usage.md @@ -2,17 +2,17 @@ ## CALL METHOD Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check verifies the usage of the `CALL METHOD` ABAP statement. It is preferable to call a method dynamically instead of via this statement: CALL METHOD. In other words, prefer a functional instead of a procedural call. +This check searches for `CALL METHOD` statements. Static occurrences, i.e. those where the called method is known statically at compile-time, of this statement are obsolete and should be replaced by their functional equivalents. ### How does the check work? -It checks the usage of `CALL METHOD` statement in the code. +The check finds `CALL METHOD` statements that do not contain dynamic parts. Dynamic parts are indicated by the method being called being specified by a literal or character-like variable in parentheses or the usage of the additions `PARAMETER-TABLE` or `EXCEPTION-TABLE`. ### How to solve the issue? -Change the long method calls using `CALL METHOD` statement to short method calls using parenthesis notation (dynamic call). +Change the method calls using `CALL METHOD` to functional method calls. ### What to do in case of exception? @@ -28,14 +28,15 @@ Before the check: ```abap DATA(class) = NEW object( ). -CALL METHOD class->method. +CALL METHOD class->method + EXPORTING param = var. ``` After the check: ```abap DATA(class) = NEW object( ). -class->method( ). +class->method( var ). ``` ### Further Readings & Knowledge diff --git a/docs/checks/chain-declaration-usage.md b/docs/checks/chain-declaration-usage.md index 2d173e0c..37f8119d 100644 --- a/docs/checks/chain-declaration-usage.md +++ b/docs/checks/chain-declaration-usage.md @@ -2,13 +2,13 @@ ## Chain Declaration Usage -### What is the Intent of the Check? +### What is the intent of the check? -This check verifies the usage of chain up-front declarations. +This check searches for chained up-front declarations of variables. Chaining visually implies a logical grouping that is often not actually present and variables often should rather be declared inline at their first point of usage. ### How to solve the issue? -Change the chain up-front declarations to inline declarations. +Change the chained up-front declarations to inline declarations. ### What to do in case of exception? @@ -18,7 +18,7 @@ In exceptional cases, you can suppress this finding by using the pseudo comment DATA: "#EC CHAIN_DECL_USAG string TYPE string, json TYPE REF TO cl_abap_json, - client LIKE sy-mandt. + client LIKE sy-mandt. ``` ```abap @@ -26,7 +26,7 @@ In exceptional cases, you can suppress this finding by using the pseudo comment name TYPE string, json TYPE REF TO cl_abap_json. ``` - +or ```abap CONSTANTS: "#EC CHAIN_DECL_USAG min_age TYPE i VALUE 18, @@ -42,23 +42,30 @@ Before the check: string TYPE string, json TYPE REF TO cl_abap_json, client LIKE sy-mandt. + string = `Hello world`. + create object json + exporting iv_json = string. + client = sy-mandt. ``` After the check: ```abap - DATA string TYPE string. - DATA json TYPE REF TO cl_abap_json. - DATA client LIKE sy-mandt. + data(string) = `Hello world`. + data(json) = new cl_abap_json( string ). + data(client) = sy-mandt. ``` -Or even (which looks neat - but it won't be enforced): +or ```abap -DATA var1 TYPE a. -DATA var2 TYPE string. -DATA my_var3 TYPE int. -DATA a TYPE c. + DATA string TYPE string. + DATA json TYPE REF TO cl_abap_json. + DATA client LIKE sy-mandt. + string = `Hello world`. + create object json + exporting iv_json = string. + client = sy-mandt. ``` ### Further Readings & Knowledge diff --git a/docs/checks/check-in-loop.md b/docs/checks/check-in-loop.md index 16a0c292..76dd4214 100644 --- a/docs/checks/check-in-loop.md +++ b/docs/checks/check-in-loop.md @@ -1,16 +1,21 @@ [code pal for ABAP](../../README.md) > [Documentation](../check_documentation.md) > [CHECK in LOOP](check-in-loop.md) -## CHECK in LOOP +## Check in Loop -### What is the Intent of the Check? -This check verifies if a `CHECK` statement is being used inside of a `LOOP` structure. A CHECK within a LOOP, ends the current iteration and proceeds to the next one. This behavior might lead to some confusion like: Does it end the method processing or does it exit the loop? +### What is the intent of the check? +This check searches for `CHECK` statements inside iterations (such as `LOOP...ENDLOOP` or `DO...ENDDO`). A `CHECK` within a loop ends the current instance of the iteration and proceeds to the next one. Since `CHECK` statements that are not inside a loop leave the current modularization unit, e.g. inside a method act like a conditional `RETURN` statement, it can be confusing to determine the effect of any given `CHECK` statement. ### How to solve the issue? -Prefer using `CONTINUE` (within an IF-Statement) instead of using the CHECK Statement in this case. Since the keyword `CONTINUE` can only be used in LOOPS, the intention is then automatic clear to everyone reading the code. -Keep also in mind, that other Keywords like: `EXIT` or `RETURN` are also more explicit than `CHECK`. +Replace the `CHECK` statement by its equivalent `IF` statement +```abap +IF not condition. + CONTINUE. +ENDIF. +``` +and take care to choose a form of the logical expression that is easy to interpret. Since the keyword `CONTINUE` can only be used in loops, the intent of continuing to the next instance of the iteration is now unambiguous. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CHECK_IN_LOOP` which should be placed after the `CHECK` Statement itself: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CHECK_IN_LOOP` which should be placed after the `CHECK` statement itself: ```abap LOOP AT tadir ASSIGNING FIELD-SYMBOL(). @@ -23,6 +28,7 @@ Before the check: ```abap LOOP AT tadir ASSIGNING FIELD-SYMBOL(). CHECK -delflag = abap_true. + "some more code... ENDLOOP. ``` @@ -32,9 +38,18 @@ LOOP AT tadir ASSIGNING FIELD-SYMBOL(). IF -delflag = abap_false. CONTINUE. ENDIF. + "some more code... +ENDLOOP. +``` + +Note that in this example, the cleanest way of expressing the intent behind the code would be to remove the statement inside the loop completely and replace it by a `WHERE` clause in the `LOOP` statement: + +```abap +LOOP AT tadir ASSIGNING FIELD-SYMBOL() WHERE delflag = abap_false. + "some more code... ENDLOOP. ``` ### Further Readings & Knowledge - [Clean ABAP: Avoid CHECK in other positions](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-check-in-other-positions) -- [Exiting Loops -> Check](https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/abapcheck_loop.htm) +- [Exiting Loops -> Check](https://help.sap.com/doc/abapdocu_756_index_htm/7.56/en-US/abapcheck_loop.htm) diff --git a/docs/checks/check-statement-position.md b/docs/checks/check-statement-position.md index ab06eee0..da2c1674 100644 --- a/docs/checks/check-statement-position.md +++ b/docs/checks/check-statement-position.md @@ -2,19 +2,20 @@ ## CHECK Statement Position Check -### What is the Intent of the Check? -This check verifies whether the `CHECK` statement is the very first statement within a method, function-module or form-routine. +### What is the intent of the check? +This check searches for `CHECK` statements that are not the first statement within a method, function module or form subroutine since the statement behaves differently in different positions and may lead to unclear, unexpected effects. -The [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-check-in-other-positions) says: -> Do not use `CHECK` outside of the initialization section of a method. The statement behaves differently in different positions and may lead to unclear, unexpected effects. +### How does the check work -REMARKS: -1. CHECK statement inside of LOOPs will be disregard by check (for that, refer to: CHECK_IN_LOOP). -2. The usage of CLEAR statement prior to CHECK statement is considered to be a bad coding practice! This is actually a workaround in bad designed code (against OO scope principles). -3. DATA declarations (DATA / FIELD-SYMBOLS when not dynamic declared – inline declarations), might also come before the keyword CHECK. +When the check finds a `CHECK` statement, it will report a finding unless one of the following circumstances applies: + +1. The statement occurs within a loop - see [Check in Loop](check-in-loop.md) for a check dealing with these statements. +2. The only statement before the `CHECK` statement are variable declarations. However, see the check for [chained declarations](chain-declaration-usage.md) for better practices when declaring variables. + +While it might sometimes seem necessary to have a `CLEAR` statement for exporting parameters in front of any `CHECK` statements, a need for both of these statements indicates a method that should be refactored to be less confusing, so the check still reports a finding for these cases. ### How to solve the issue? -The `CHECK` statement shall be the first statement of a method. If it is not possible, try to substitute this keyword with an IF-statement instead. +Either move the `CHECK` statement to be the first statement of the method or replace it with its equivalent `IF` statement. ### What to do in case of exception? In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CHECK_POSITION` which has to be placed after the `CHECK` statement: @@ -45,7 +46,17 @@ METHOD example. ... ENDMETHOD. ``` -OR +or + +```abap +METHOD example. + ... + IF sy-mandt = 000. + ... + ENDIF. +ENDMETHOD. +``` +or ```abap METHOD example. @@ -54,5 +65,6 @@ METHOD example. ENDMETHOD. ``` +Note how the second option expresses most clearly the intent both syntactically and visually - the rest of the method (the part indented inside the `IF` statement) is to be executed if the condition is true. ### Further Readings & Knowledge - [Clean ABAP: Avoid CHECK in other positions (Clean ABAP)](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#avoid-check-in-other-positions) diff --git a/docs/checks/collect.md b/docs/checks/collect.md index a4bcd119..2297f6b5 100644 --- a/docs/checks/collect.md +++ b/docs/checks/collect.md @@ -2,31 +2,29 @@ ## COLLECT restriction -### What is the Intent of the Check? +### What is the intent of the check? The [ABAP - Keyword Documentation](https://help.sap.com/doc/abapdocu_755_index_htm/7.55/en-US/abencollect_guidl.htm?file=abencollect_guidl.htm) says: > **Rule** > > Do not fill standard tables with collections of lines > -> Only use the statement COLLECT for hashed tables or sorted tables with a unique key. Do not use it any more for standard tables. +> Only use the statement `COLLECT` for hashed tables or sorted tables with a unique key. Do not use it any more for standard tables. > > **Details** > -> The statement COLLECT is based on unique entries with respect to the primary key and stable key administration. This means that not all categories of internal tables are suitable for COLLECT: +> The statement `COLLECT` is based on unique entries with respect to the primary key and stable key administration. This means that not all categories of internal tables are suitable for `COLLECT`: > -> * If the statement COLLECT is applied to a standard table, this table first needs its own internal hash administration. Change operations on the table can invalidate this temporary hash administration. After a change operation of this type, the following COLLECT statements must resort to a linear search, which can affect performance considerably. The primary key of a standard table is also never unique. -> * COLLECT can be used for sorted tables and hashed tables without any problems since these, unlike standard tables, always have a separate, stable key administration that can be utilized by COLLECT. COLLECT can work properly for sorted tables only if the primary key is unique. If a sorted table has a non-unique key, only COLLECT can be used to fill the table, which is difficult to guarantee. In hashed tables, the key values are always unique. +> * If the statement `COLLECT` is applied to a standard table, this table first needs its own internal hash administration. Change operations on the table can invalidate this temporary hash administration. After a change operation of this type, the following COLLECT statements must resort to a linear search, which can affect performance considerably. The primary key of a standard table is also never unique. +> * `COLLECT` can be used for sorted tables and hashed tables without any problems since these, unlike standard tables, always have a separate, stable key administration that can be utilized by COLLECT. `COLLECT` can work properly for sorted tables only if the primary key is unique. If a sorted table has a non-unique key, only COLLECT can be used to fill the table, which is difficult to guarantee. In hashed tables, the key values are always unique. ### How does the check work? -It searches for `COLLECT` in: -* Internal tables typed as `SORTED TABLE` with `NON-UNIQUE KEY`; -* Internal tables typed as `STANDARD TABLE`; +The check searches for `COLLECT` statements whose argument are internal tables typed as `SORTED TABLE ... WITH NON-UNIQUE KEY` or `STANDARD TABLE`. ### How to solve the issue? -Change the internal table to `SORTED` or `HASHED` as recommended, or perform the collection manually. +Change the internal table to be a sorted or hashed table as recommended by the ABAP keyword documentation, or perform the collection manually. ### What to do in case of exception? diff --git a/docs/checks/comment-position.md b/docs/checks/comment-position.md index 7a961784..9c1e1b39 100644 --- a/docs/checks/comment-position.md +++ b/docs/checks/comment-position.md @@ -2,16 +2,18 @@ ## Comment Position -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for "Quote comments" which are not indented along with the statements they belong to. +This check finds comments starting with `"` that are not indented at the same level as the statements that follow them. Comments should generally refer to the code that follows them, and hence should be indented at the same level. + +The check does not report findings for comments that are in-line comments or are the only "code" inside an empty branch of a conditional statement. ### How to solve the issue? -You should indent the comments along with the statements they are commenting. +You should indent comments along with the statements they are commenting. ### What to do in case of exception? -There is no exception for this check since it works as an indicator only. Thus, it is also not possible to suppress its findings. +There are no pseudo comments for this check since you cannot put pseudo comments on the same line as an actual comment. ### Example @@ -22,17 +24,17 @@ Before the check: output = calculate_result( input ). ``` -```abap - output = calculate_result( input ). " delegate pattern -``` - After the check: ```abap " delegate pattern output = calculate_result( input ). ``` +or +```abap + output = calculate_result( input ). " delegate pattern +``` ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#put-comments-before-the-statement-they-relate-to) +* [Clean ABAP - Put comments before the statement they relate to](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#put-comments-before-the-statement-they-relate-to) diff --git a/docs/checks/comment-type.md b/docs/checks/comment-type.md index 4e0d60d0..c2534e79 100644 --- a/docs/checks/comment-type.md +++ b/docs/checks/comment-type.md @@ -2,16 +2,16 @@ ## Comment Type -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for comments in the code marked up with `"` instead of with `*`. Comments marked up with an asterisk tend to be indented to weird/uncontrolled places. +This check searches for comments starting with an asterisk `*`. These tend to not align well with the rest of the code, especially when the surrounding code changes indentation level, and should be replaced by comments starting with a quotation mark `"`. ### How to solve the issue? -You should replace the comment sign from `*` to `"`. +You should replace the `*` by `"`. ### What to do in case of exception? -There is no exception for this check since it works as an indicator only. Thus, it is also not possible to suppress its findings. +There are no pseudo comments for this check since you cannot put pseudo comments on the same line as an actual comment. ### Example @@ -39,4 +39,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP: Comment sign](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#comment-with--not-with-) +* [Clean ABAP - Comment with ", not with *](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#comment-with--not-with-) diff --git a/docs/checks/comment-usage.md b/docs/checks/comment-usage.md index 6b529124..406c254e 100644 --- a/docs/checks/comment-usage.md +++ b/docs/checks/comment-usage.md @@ -2,22 +2,23 @@ ## Comment Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for comments in the code. Clean Code principles do not forbid you to comment your code - but it encourages you to exploit better means, meaningful names and resort to comments only if that fails. Express yourself in code, not in comments. +This check counts the number of comments in your code. While Clean Code principles do not prohibit commenting as such, they encourage us to use better means to express ourselves in our code whenever possible, and so an excessive number of comments can be an indication that this guideline is not followed or misunderstood in a particular piece of code. ### How does the check work? -This check calculates an indicator; precisely, the percentage of comments in relation to the absolute number of real statements (productive code). +The check counts the number of comments in a compilation unit and then emits a finding that tells you the ratio of comments to productive statements in that code. ### How to solve the issue? -Remove unimportant and/or unnecessary comments. +Perhaps there is no issue and every comment in the code is actually meaningful and necessary. Do not treat findings from this check as issues that need to be "solved" by deleting potentially valuable comments, but consider them an invitation to think about your comment practices and find ways to incorporate the information they convey directly into the code itself. -### What to do in case of exception? +Of course, if you do decide that some comments are unnecessary, delete them. -There is no exception for this check since it works as an indicator only. Thus, it is also not possible to suppress its findings. +### What to do in case of exception? +There are no pseudo comments for this check since it reports a metric that refers to a complete compilation unit. There is no single line in the code it would refer to, so there is no meaningful location where a pseudo comment suppressing it could be placed. ### Further Readings & Knowledge -* [Clean ABAP: Less is more](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#express-yourself-in-code-not-in-comments) +* [Clean ABAP - Express yourself in code, not comments](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#express-yourself-in-code-not-in-comments) diff --git a/docs/checks/constants-interface.md b/docs/checks/constants-interface.md index 9d1b489a..29ac132b 100644 --- a/docs/checks/constants-interface.md +++ b/docs/checks/constants-interface.md @@ -2,10 +2,41 @@ ## Constants Interface Check -### What is the Intent of the Check? +### What is the intent of the check? -This check intends to avoid the creation and usage of an interface object for merely defining constants. -You should always prefer [enumeration classes](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-enumeration-classes-to-constants-interfaces) to constants interfaces. +This check is meant to prevent the creation of interfaces whose sole purpose is to define constants. You should usually prefer [enumeration classes](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-enumeration-classes-to-constants-interfaces) to interfaces that only declare constants. + +### How does the check work? + +The check searches for interfaces containing only constants. +### How to solve the issue? + +Use enumeration classes instead. + +### What to do in case of exception? + +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CONS_INTF` which should be placed right after the class definition header: + +```abap +INTERFACE interface_name. "#EC CONS_INTF + CONSTANTS two TYPE i VALUE 2. +ENDINTERFACE. +``` + +### Example + +Before the check: + +```abap +INTERFACE /dirty/message_severity. + CONSTANTS: + error TYPE symsgty VALUE 'E', + warning TYPE symsgty VALUE 'W', + notification TYPE symsgty VALUE 'N'. +ENDINTERFACE. +``` + +After the check: ```abap CLASS /clean/message_severity DEFINITION PUBLIC ABSTRACT FINAL. @@ -16,7 +47,7 @@ CLASS /clean/message_severity DEFINITION PUBLIC ABSTRACT FINAL. notification TYPE symsgty VALUE 'N'. ENDCLASS. ``` -or even +or ```abap CLASS /clean/message_severity DEFINITION PUBLIC CREATE PRIVATE FINAL. PUBLIC SECTION. @@ -28,26 +59,6 @@ CLASS /clean/message_severity DEFINITION PUBLIC CREATE PRIVATE FINAL. ENDCLASS. ``` -### How does the check work? - -The "Constant Interface" check searches for interfaces containing only constants. - -### How to solve the issue? - -Use enumeration classes instead. - -### What to do in case of exception? - -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CONS_INTF` which should be placed right after the class definition header: - -### Example - -```abap -INTERFACE interface_name. "#EC CONS_INTF - CONSTANTS two TYPE i VALUE 2. -ENDINTERFACE. -``` - ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-enumeration-classes-to-constants-interfaces) +* [Clean ABAP - Prefer enumeration classes to interfaces with constants](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-enumeration-classes-to-constants-interfaces) diff --git a/docs/checks/cx-root-usage.md b/docs/checks/cx-root-usage.md index 49bcdf51..d3a59eb4 100644 --- a/docs/checks/cx-root-usage.md +++ b/docs/checks/cx-root-usage.md @@ -2,36 +2,18 @@ ## CX_ROOT Usage Check -### What is the Intent of the Check? -This check searches for direct "CX_ROOT" exceptions being used in the code (e.g.: In a TRY-CATCH block). +### What is the intent of the check? -### How does the check work? - -It search for the "direct" usage of CX_ROOT exceptions like: - -Explicitly: -```abap -TRY. - cls=>meth( ). -CATCH cx_root. - cl_demo_output=>display( 'Catching exception' ). -ENDTRY. -``` - -Implicitly: -```abap -CLASS cx_my_exception DEFINITION INHERITING FROM cx_root. -ENDCLASS. -``` +This check searches for exceptions of type `CX_ROOT` (and not one of its subtypes) being directly used in the `CATCH` clause of a `TRY...CATCH` block. Catching all exceptions is often a stop-gap solution during prototyping that should be removed before the code goes productive. ### How to solve the issue? -The solution is to use well defined and specific class-based exceptions. +Since static and dynamic exceptions must be explicitly declared in method signatures if a method can throw them, you can always explicitly list the types of these exceptions in your `CATCH` clause instead. If you want to really catch even the *unexpected* exceptions, adding `CX_NO_CHECK` as a caught type is equivalent to using `CX_ROOT` and expresses your intent more clearly. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `“#EC NEED_CX_ROOT` which should be placed after the CATCH statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `“#EC NEED_CX_ROOT` which should be placed after the `CATCH` statement: ```abap TRY. @@ -41,7 +23,8 @@ CATCH cx_root. "#EC NEED_CX_ROOT ENDTRY. ``` -### Further Readings & Knowledge +An identical check is part of the Extended Program Check (SLIN) delivered by SAP (and existed long before this Code Pal check). Use either the SLIN check *or* this one, since otherwise you will get two different findings for the same issue, and the pragma that suppresses the SLIN check will not suppress this one, and vice versa the pseudo comment that suppresses this check will not suppress the SLIN check. -* [Clean ABAP: Using class based exceptions](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#use-class-based-exceptions) +### Further readings + - [Clean ABAP - Exceptions](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#exceptions) \ No newline at end of file diff --git a/docs/checks/cyclomatic-complexity.md b/docs/checks/cyclomatic-complexity.md index e32305e7..aca2191d 100644 --- a/docs/checks/cyclomatic-complexity.md +++ b/docs/checks/cyclomatic-complexity.md @@ -2,24 +2,21 @@ ## Cyclomatic Complexity Check -### What is the Intent of the Check? +### What is the intent of the check? -This check measures the complexity of your code based on a control flow graph. It counts the number of linearly-independent possible paths through the source code. +This check measures the [cyclomatic complexity](https://en.wikipedia.org/wiki/Cyclomatic_complexity) of your code based on a control flow graph. It counts the number of independent possible paths through the source code. -A high value in cyclomatic complexity is an indicator that the source code might have a difficult readability. Thus, maintaining and extending this complex code is also difficult. In addition, the risk of introducing bugs (regression) is higher in code with high cyclomatic complexity. +A high value in cyclomatic complexity is an indicator that the source code is rather complex and might be difficult to read and understand. Thus, maintaining and extending this complex code might also be difficult, in particular since the high complexity means that there are many different paths through the code that are very likely to not all be well-tested. ### How does the check work? -In this implementation, the number of binary decision points "b" (for instance: IF-statements) is counted. +In this implementation, the number of binary decision points `b` is counted as follows: -The cyclomatic complexity M = b + 1 is calculated as follows: + - Every `IF`, `ELSEIF`, `CHECK` and `ASSERT` statement is a decision point. + - Within a `CASE` structure every `WHEN` branch is a decision point, unless it is the `WHEN OTHERS` branch. + - Loops (`LOOP`, `DO`, `WHILE`) are decision points since they might be executed at least once or not at all. `DO` loops are only decision points when they have a `TIMES` addition, since otherwise they cannot branch. -Every IF and ELSEIF are counted as decisions ("b") - but not the "AND" and "OR" and other logical operands within them. -Within a CASE statement all WHEN tokens are counted ("b"). -LOOP, DO and WHILE are all counted ("b") and also CHECK within a loop as this is a conditional short circuit of the loop (avoid using CHECK, try to use an IF-Statement instead). -FORM, METHOD and FUNCTION are counted ("b") as these are separate sections of code with distinct entry and exit points. -Within SQL statements a WHERE clause is also a decision point ("b") and is therefore counted in SELECT, MODIFY, UPDATE and DELETE statements, however, these statements are not counted if there is no WHERE clause. Similarly a KEY clause in a READ statement is counted ("b"). -A coding block without decision points (for instance: just some statements of DATA declaration or some APPENDs in tables) will have at least M = 1. + The cyclomatic complexity of the code is `b+1`. ### How to solve the issue? @@ -29,8 +26,6 @@ Modularize the functionality into smaller blocks. This reduces the cyclomatic co In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_CYCLO` which should be placed after the `ENDMETHOD` statement: -#### Example - ```abap METHOD method_name. " Method content diff --git a/docs/checks/db-access-in-ut.md b/docs/checks/db-access-in-ut.md index 7b68919c..4e7dc976 100644 --- a/docs/checks/db-access-in-ut.md +++ b/docs/checks/db-access-in-ut.md @@ -2,17 +2,17 @@ ## Database Access within Unit-Tests Check -### What is the Intent of the Check? +### What is the intent of the check? -The Check identifies database operations within the test classes. +The check searches for database operations inside ABAP Unit test classes that do not use an SQL test double framework to mock the database. Unit tests should usually be isolated from the actual database. ### How does the check work? -The Check behaves differently depending on the test class classification (`RISK LEVEL`) and automatically exempts the finding if a known test framework is find. +The check behaves differently depending on the test class's risk level: #### 1. `RISK LEVEL HARMLESS` or not set -The Check reports: +The check reports: * `SELECT` * `INSERT` * `UPDATE` @@ -24,23 +24,15 @@ The Check reports: #### 2. `RISK LEVEL DANGEROUS` or `RISK LEVEL CRITICAL` -The Check reports: +The check reports: * `UPDATE` * `MODIFY` * `DELETE` * `ALTER` -In short, the Check allows: -* `SELECT` -* `INSERT` -* `COMMIT` -* `ROLLBACK` - #### 3. Test Frameworks -The Check identifies if the test class uses one of the following objects internally and exempts the finding automatically. - -The relevant objects for the automatic exemption are: +If an instance of one of the following types associated with a test double framework is used in the test class, no findings are reported for database accesses: * `IF_OSQL_TEST_ENVIRONMENT` * `CL_OSQL_TEST_ENVIRONMENT` * `IF_CDS_TEST_ENVIRONMENT` @@ -48,14 +40,14 @@ The relevant objects for the automatic exemption are: ### How to solve the issue? -Please, isolate the database dependency using one of the below frameworks: +Isolate the database dependency using one of the test double frameworks for SQL and CDS accesses: * [ABAP SQL Test Double Framework](https://help.sap.com/viewer/c238d694b825421f940829321ffa326a/1809.000/en-US/8562b437073d4b9c93078c45f7a64f21.html) * [ABAP CDS Test Double Framework](https://help.sap.com/viewer/5371047f1273405bb46725a417f95433/Cloud/en-US/8562b437073d4b9c93078c45f7a64f21.html) ### What to do in case of exception? In special cases, it is possible to suppress a finding by using the pseudo comment `"#EC DB_ACCESS_UT`. -The pseudo comment must be placed right after the DB access statement. +The pseudo comment must be placed right after the statement that accesses the database. ```ABAP SELECT * FROM tadir INTO TABLE @DATA(entries). "#EC DB_ACCESS_UT diff --git a/docs/checks/deprecated-classes.md b/docs/checks/deprecated-classes.md index 3b578a68..4a866c69 100644 --- a/docs/checks/deprecated-classes.md +++ b/docs/checks/deprecated-classes.md @@ -2,23 +2,20 @@ ## Deprecated Classes -### What is the Intent of the Check? +### What is the intent of the check? -This check points out deprecated classes which should be replaced by newer objects. - -You can check the list of currently supported objects in the `constructor` of the `y_check_deprecated_classes` class. +This check searches for the usage of deprecated objects which should be replaced by newer objects. ### How does the check work? -This check searches for the usage of deprecated classes. For instance: +The check searches for occurrences of the following objects: -```abap -DATA aunit TYPE REF TO cl_aunit_assert. -``` +* `CL_AUNIT_ASSERT` +* `IF_AUNIT_CONSTANTS` ### How to solve the issue? -Reference non deprecated/newer objects instead. For the above example, a corrected code would look like: +Reference non-deprecated/newer objects instead. For the above example, a corrected code would look like: ```abap DATA aunit TYPE REF TO cl_abap_unit_assert. diff --git a/docs/checks/deprecated-key-word.md b/docs/checks/deprecated-key-word.md index bd3dfda8..86f30ead 100644 --- a/docs/checks/deprecated-key-word.md +++ b/docs/checks/deprecated-key-word.md @@ -2,28 +2,20 @@ ## Deprecated Key Word Check -### What is the Intent of the Check? +### What is the intent of the check? -This check points out old syntax which should be replaced with newer notations instead. +This check searches for obsolete syntax elements which should be replaced with newer syntax elements instead. ### How does the check work? -This check searches for deprecated key words like: `MOVE` and `TRANSLATE` and suggests its replacement to news notations/functions. +This check searches for the statements starting with the following keywords: -```ABAP -" MOVE 'A' TO variable. -DATA(variable) = 'A'. - -" TRANSLATE lowercase TO UPPER CASE. -DATA(uppercase) = to_upper( lowercase ). - -``` - -REMARK: The check will be continuously enhanced with other deprecated ABAP Keywords. +* `MOVE` +* `TRANSLATE` ### How to solve the issue? -Use the newer notations instead. +Please `MOVE` by a normal assignment statement using `=` and `TRANSLATE` by an equivalent functional expression using [string functions}(https://help.sap.com/doc/abapdocu_755_index_htm/7.55/en-US/abenprocess_functions.htm). ### What to do in case of exception? @@ -34,3 +26,20 @@ MOVE …. "#EC DEPRECATED_KEY TRANSLATE …. "#EC DEPRECATED_KEY ``` + +### Example + + +Before the check: + +```abap +MOVE 'A' TO variable. +TRANSLATE lowercase TO UPPER CASE. +``` + +After the check: + +```abap +DATA(variable) = 'A'. +DATA(uppercase) = to_upper( lowercase ). +``` diff --git a/docs/checks/empty-catch.md b/docs/checks/empty-catch.md index 33be338d..e2672030 100644 --- a/docs/checks/empty-catch.md +++ b/docs/checks/empty-catch.md @@ -2,17 +2,19 @@ ## Empty Catch -### What is the Intent of the Check? +### What is the intent of the check? This check searches for empty `CATCH` blocks. ### How to solve the issue? -Fill the `CATCH` block with an exception handling. +Perform meaningful exception handling in the `CATCH` block. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_CATCH` or `"#EC NO_HANDLER` which should to be placed after the opening statement of the empty `CATCH`: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_CATCH` or `"#EC NO_HANDLER` which should to be placed after the opening statement of the empty `CATCH`. + +Note that this check is the same as a check in the Extended Program Check (SLIN) delivered by SAP. That check accepts a pragma `##NO_HANDLER` for suppressing its findings that Code Pal cannot evaluate (pragmas are inaccessible to ordinary Code Inspector checks). We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. ```abap TRY. diff --git a/docs/checks/empty-if-branches.md b/docs/checks/empty-if-branches.md index 3d5af261..253aa0f7 100644 --- a/docs/checks/empty-if-branches.md +++ b/docs/checks/empty-if-branches.md @@ -2,9 +2,9 @@ ## Empty IF-Branch Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for empty `IF` statements or branches. +This check searches for empty branches of `IF` statements. ### How to solve the issue? @@ -12,7 +12,10 @@ Fill the empty `IF` structure with code or remove it by refactoring the conditio ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_IF_BRANCH` which should be placed after the opening statement of the empty `IF` branch; or, in case of nested `IF`statements, in the deepest structure within the branch: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_IF_BRANCH` which should be placed after the opening statement of the empty `IF` branch; or, in case of nested `IF`statements, in the deepest structure within the branch. + +Note that this check is the same as a check in the Extended Program Check (SLIN) delivered by SAP. That check accepts a pragma for suppressing its findings that Code Pal cannot evaluate (pragmas are inaccessible to ordinary Code Inspector checks). We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. + ```abap IF name = ''. diff --git a/docs/checks/empty-procedure.md b/docs/checks/empty-procedure.md index 1d922a7a..0538bf3a 100644 --- a/docs/checks/empty-procedure.md +++ b/docs/checks/empty-procedure.md @@ -2,21 +2,24 @@ ## Empty Procedure Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for empty methods, forms and modules. +This check searches for empty modularization units - methods, forms and function modules - since empty procedures usually fulfill no purpose and can be deleted. ### How does the check work? -The check highlights all empty code statements. Comments and Pragmas are also classified as "empty". +A modularization unit counts as empty when it contains no ABAP statements. Comments and pragmas do not count as ABAP statements. ### How to solve the issue? -Remove the empty code blocks if they are not needed. +Delete the empty modularization unit unless its existence is functionally necessary. The only case where such empty procedures are needed is usually when implementing interface methods or redefining inheriting methods to explicitly do nothing. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_PROCEDURE` which should be placed right after the `ENDMETHOD` statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC EMPTY_PROCEDURE` which should be placed right after the `ENDMETHOD` statement. + +Note that this check is the same as a check in the Extended Program Check (SLIN) delivered by SAP. That check accepts a pragma `##NEEDED` for suppressing its findings that Code Pal cannot evaluate (pragmas are inaccessible to ordinary Code Inspector checks). We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. + ```abap METHOD method_name. diff --git a/docs/checks/equals-sign-chaining.md b/docs/checks/equals-sign-chaining.md index 83edc3db..3218a184 100644 --- a/docs/checks/equals-sign-chaining.md +++ b/docs/checks/equals-sign-chaining.md @@ -2,9 +2,9 @@ ## Equals Sign Chaining -### What is the Intent of the Check? +### What is the intent of the check? -This check identifies sequenced assignments as they usually confuse the reader. +This check identifies chained assignments to multiple variables as they usually confuse the reader. ```abap "anti-pattern @@ -13,18 +13,13 @@ x = y = z ### How to solve the issue? -Break it in multiple rows: +Break the assignment into multiple rows: + ```abap y = z. x = y. ``` -Alternatively, you can use the `xsdbool` if the target is a comparison: - -```abap -x = xsdbool( y = z ). -``` - ### What to do in case of exception? In exceptional cases, you can suppress this finding by using the pseudo-comment `"#EC EQUALS_CHAINING` which should be placed after the attribution: @@ -35,4 +30,4 @@ x = y = z. "#EC EQUALS_CHAINING ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#use-xsdbool-to-set-boolean-variables) +* [Clean ABAP - Don't chain assignments](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#dont-chain-assignments) diff --git a/docs/checks/external-call-in-ut.md b/docs/checks/external-call-in-ut.md index 66fa107f..a272899a 100644 --- a/docs/checks/external-call-in-ut.md +++ b/docs/checks/external-call-in-ut.md @@ -2,18 +2,17 @@ ## External Call in Unit Tests-Check -### What is the Intent of the Check? +### What is the intent of the check? -This check scans test classes and its contents searching for any kind of explicit redirection (external call changing the main workflow to another program) within test methods. Since every external call/redirection is considered to be a dependency, this should not be allowed in test code. +This check searches test classes for statements that change the current main program or cause parallel sessions to spawn. Test code should not change the control flow to other main programs or directly call GUI elements. ### How does the check work? -The check searches for statements which may lead to an external call or redirection (e.g.: `SUBMIT`) deviating the regular workflow (call stack) of a program to another program. -The check also searches for any Remote Calls (RFCs) as well as instances or usage of `CL_GUI_*` classes (these should also be avoid in Unit Tests). +The check searches for `SUBMIT` statements, remote function calls of any kind (`CALL FUNCTION ... DESTINATION`) and calls to `CL_GUI_*` classes. ### How to solve the issue? -The solution is either to remove or mock these external call/redirection with a proper dependency isolation technique. +Remove these external calls and/or mock them with a proper dependency isolation technique. ### What to do in case of exception? diff --git a/docs/checks/form-routine.md b/docs/checks/form-routine.md index 03afdfbc..9fcaa2b6 100644 --- a/docs/checks/form-routine.md +++ b/docs/checks/form-routine.md @@ -2,17 +2,17 @@ ## FORM Routine Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the usage of FORM Routines (procedural programming) since with the release of Object Oriented ABAP this syntax became obsolete. +This check searches for the usage of form subroutines since this concept became obsolete with the release of object-oriented ABAP. ### How does the check work? -This check searches for the usage of the `ENDFORM` statement. +This check searches for `ENDFORM.` statements. ### How to solve the issue? -Use classes and methods instead. Methods are similar to subroutines and can be used for modularization. +Use classes and methods instead as these are the intended tool for modularization in object-oriented ABAP. ### What to do in case of exception? @@ -26,4 +26,4 @@ ENDFORM. ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-object-orientation-to-procedural-programming) +* [Clean ABAP - Prefer object orientation to procedural programming](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-object-orientation-to-procedural-programming) diff --git a/docs/checks/interface-in-class.md b/docs/checks/interface-in-class.md index 3a55d00a..d7848439 100644 --- a/docs/checks/interface-in-class.md +++ b/docs/checks/interface-in-class.md @@ -2,17 +2,17 @@ ## Interface Missing Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for classes having public methods without an interface. +This check searches for classes with public methods that are not part of an interface. Methods that are not part of an interface make decoupling more difficult and, especially if the class is declared as final, are much harder to mock in unit tests of consumers. ### How does the check work? -Since every class having at least one public method should implement an interface, this check searches for public methods within a class without having an associated interface (being implemented). +This check searches for declarations of methods within the public section of a class that are not redefinitions. It does not care about static methods (`CLASS-METHODS`). ### How to solve the issue? -Make sure to implement an interface for the public methods. Even though this seems to be unnecessary in some cases, having an interface will easily allow mocking data in the future. +Make sure to create an interface for public methods. ### What to do in case of exception? @@ -26,4 +26,4 @@ ENDCLASS. ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#public-instance-methods-should-be-part-of-an-interface) +* [Clean ABAP - Public instance methods should be part of an interface](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#public-instance-methods-should-be-part-of-an-interface) diff --git a/docs/checks/magic-number.md b/docs/checks/magic-number.md index 7f80dfd2..5743ed3c 100644 --- a/docs/checks/magic-number.md +++ b/docs/checks/magic-number.md @@ -2,18 +2,13 @@ ## Magic Number Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for arbitrary values in the source code having no meaningful connotation. - -Using magic numbers has disadvantages: - -* The code is not readable as you need to understand the meaning of the number again and again. -* If these numbers need to be changed, modifications are required in every place in the code where this number is present. This makes the maintenance of the code inefficient and error prone. +This check searches for numeric literals used directly without a meaningful constant declaration, often called magic numbers. Magic numbers hide their meaning from the user (e.g. whether they are a default value or have a specific meaning in the current context) and make refactoring unnecessarily hard when the same literal occurs in several places. ### How does the check work? -It searches for numbers/values in the following statements: +The check searches for numeric literals in the following statements: 1. `IF` 2. `ELSEIF` @@ -21,15 +16,17 @@ It searches for numbers/values in the following statements: 4. `CHECK` 5. `DO` -REMARK: Magic Numbers associated with `SY-SUBRC` are not considered by this check. In addition, the numbers `0` and `1` are ignored. +A finding is reported for every such literal that is not `0`, `1` or a power of ten unless it is used as an index in a table expression or an operand in a comparison with the result of a `LINES` function. ### How to solve the issue? -Create constants. By the name of the constant the number becomes a meaning which increases the readability. In addition, when maintaining the code, you only need to change the constant. This change can be done without the risk of introducing new errors or without forgetting some places where this change is required. +Replace all numeric literals by constants with a meaningful name. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_MAGIC` which should be placed right after the statement containing the magic number: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_MAGIC` which should be placed right after the statement containing the magic number. + +Note that there is a very similar check in the Extended Program Check (SLIN) delivered by SAP, although it differs in its scope as it is not restricted to specific statement types. That check accepts a pragma `##NUMBER_OK` for suppressing its findings that Code Pal cannot evaluate (pragmas are inaccessible to ordinary Code Inspector checks). We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. ```abap DO 5 TIMES. "#EC CI_MAGIC diff --git a/docs/checks/maximum-nesting-depth.md b/docs/checks/maximum-nesting-depth.md index 2bd801bd..da5744a1 100644 --- a/docs/checks/maximum-nesting-depth.md +++ b/docs/checks/maximum-nesting-depth.md @@ -2,15 +2,15 @@ ## Nesting Depth Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the nesting-depth level of a method, function-module, form-routine or module. A high value of nesting depth is an indicator that the source code might be difficult to read (no readability). Furthermore, maintaining and extending the code is also more difficult. In addition, the risk of introducing bugs is increased with a high nesting depth value. +This check computes the maximal nesting depth of structures within a modularization unit. A high nesting depth is an indicator that the source code might be difficult to read and understand, making maintaining and extending the code also more difficult. ### How does the check work? -The check verifies if the number of nested code blocks within a method, function module, form routine or module has reached/exceeded a defined threshold (configurable). +The nesting depth of structures at a point is defined by the number of currently open structures (like `LOOP...ENDLOOP`, `IF...ENDIF`) at that point. In well-formatted code this usually corresponds to the level of indentation. -REMARK: The `TEST-SEAM` statement does not count to the level of nesting depth (it is ignored). +The check reports a finding when the maximum over all nesting depths inside a modularization unit exceeds the configured threshold. ### How to solve the issue? @@ -39,4 +39,4 @@ ENDMETHOD. "#EC CI_NESTING ### Further Readings & Knowledge -* [Clean ABAP - keep the Nesting Depth low](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#keep-the-nesting-depth-low) +* [Clean ABAP - Keep the Nesting Depth Low](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#keep-the-nesting-depth-low) diff --git a/docs/checks/message-easy-to-find.md b/docs/checks/message-easy-to-find.md index 9ae8ace4..7c652bec 100644 --- a/docs/checks/message-easy-to-find.md +++ b/docs/checks/message-easy-to-find.md @@ -2,13 +2,13 @@ ## Message Easy To Find -### What is the Intent of the Check? +### What is the intent of the check? -Based on the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#make-messages-easy-to-find), it searches for the `MESSAGE` statement that cannot be detected if you run a where-used search in the transaction `SE91`. +Since the specification of message classes and numbers by variables creates no entry in the where-used list of the corresponding message, this check searches for `MESSAGE` statements that do not statically specify their message by literals since these cannot be found in the where-used list of the message. ### How to solve the issue? -Declare the message class instead of making it dynamic. +Specify the message used by literals. ### What to do in case of exception? @@ -29,7 +29,6 @@ Before the check: MESSAGE i002(message_class). MESSAGE ID message_class type 'I' NUMBER message_id. - MESSAGE ID '00' type 'I' NUMBER '002'. ``` After the check: @@ -41,4 +40,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP: Make messages easy to find](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#make-messages-easy-to-find) +* [Clean ABAP - Make messages easy to find](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#make-messages-easy-to-find) diff --git a/docs/checks/message-translation.md b/docs/checks/message-translation.md index 1d1a5230..2937ed67 100644 --- a/docs/checks/message-translation.md +++ b/docs/checks/message-translation.md @@ -2,17 +2,19 @@ ## Message Translation -### What is the Intent of the Check? +### What is the intent of the check? -It identifies the `MESSAGES` statement where the text is hard-coded as it cannot be translated. +This check finds `MESSAGE` statements where an untranslatable text literal is specified instead of a text element or message from a message class. ### How to solve the issue? -Use [Message Classes](https://help.sap.com/doc/saphelp_nw75/7.5.5/en-us/4e/c242f66e391014adc9fffe4e204223/content.htm) or [Text Elements](https://help.sap.com/doc/saphelp_nw73ehp1/7.31.19/en-US/e3/9609f6eb0711d194d100a0c94260a5/content.htm) instead. +Use [message classes](https://help.sap.com/doc/saphelp_nw75/7.5.5/en-us/4e/c242f66e391014adc9fffe4e204223/content.htm) or [text elements](https://help.sap.com/doc/saphelp_nw73ehp1/7.31.19/en-US/e3/9609f6eb0711d194d100a0c94260a5/content.htm) instead. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC MSG_TRANSL` which has to be placed after the `MESSAGE` statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC MSG_TRANSL` which has to be placed after the `MESSAGE` statement. + +Note that this check is a subset of a similar check in the Extended Program Check (SLIN) delivered by SAP. That check accepts a pragma `##NO_TEXT` for suppressing its findings that Code Pal cannot evaluate (pragmas are inaccessible to ordinary Code Inspector checks). We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. ```abap MESSAGE 'File not found!' TYPE 'W'. "#EC MSG_TRANSL @@ -30,6 +32,10 @@ After the check: ```abap MESSAGE i002(00). - MESSAGE ID sy-msgid TYPE sy-msgty NUMBER sy-msgno. +``` + +or + +``` MESSAGE TEXT-001 TYPE 'W'. ``` diff --git a/docs/checks/method-output-parameter.md b/docs/checks/method-output-parameter.md index db6c85bd..35a2f6a2 100644 --- a/docs/checks/method-output-parameter.md +++ b/docs/checks/method-output-parameter.md @@ -2,13 +2,13 @@ ## Combination of Output Parameters Check -### What is the Intent of the Check? +### What is the intent of the check? This check searches for methods where a combination of `EXPORTING`, `CHANGING` and/or `RETURNING` parameters is used. ### How to solve the issue? -Use just one sort of output type per method. +Use just one sort of output type for each method. ### What to do in case of exception? diff --git a/docs/checks/method-return-bool.md b/docs/checks/method-return-bool.md index cbe30908..21c32389 100644 --- a/docs/checks/method-return-bool.md +++ b/docs/checks/method-return-bool.md @@ -2,13 +2,13 @@ ## Method Name Misleading for Boolean Return Check -### What is the Intent of the Check? +### What is the intent of the check? -This check aims for a meaningful name for methods returning boolean valued types. +This check searches for methods that return a boolean value whose name does not indicate that they return a boolean. In particular in the case of predicative method calls it is important that such methods are named in a way that expresses the boolean nature of their return value clearly. ### How does the check work? -The check searches for methods with a boolean returning value and then verifies if the name starts with one of the following words: +The check searches for method declarations whose returning parameter has the type `ABAP_BOOL` and reports a finding if the method does not either start with any of the following strings: * `is_` * `has_` @@ -23,7 +23,7 @@ The check searches for methods with a boolean returning value and then verifies * `was_` * `were_` -or if the name contains one of the following words: +or contains one of the following words: * `exist` * `equal` @@ -31,7 +31,7 @@ or if the name contains one of the following words: ### How to solve the issue? -Rename the method and start with is, has, are, have or contains. +Rename the method to properly reflect the boolean nature of its return value. ### What to do in case of exception? diff --git a/docs/checks/non-class-exception.md b/docs/checks/non-class-exception.md index 215eaa09..fbd68674 100644 --- a/docs/checks/non-class-exception.md +++ b/docs/checks/non-class-exception.md @@ -2,30 +2,33 @@ ## Non-Class Exception Check Usage -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for non-class based exceptions raised in your code. +This check searches locations where "classic" (i.e. not object-oriented) exceptions are raised. ### How does the check work? -It search for non class-based exceptions like: +The check searches for the following statements: -* `RAISE SYSTEM-EXCEPTIONS` -* `RAISE` ( without `EXCEPTION` or `RESUMABLE EXCEPTION` ) +* `RAISE` (without `EXCEPTION` or `RESUMABLE EXCEPTION`) * `MESSAGE with RAISING` ### How to solve the issue? -The solution is to use class-base exceptions like: +The solution is to use class-based exceptions instead: -* `RAISE EXCEPTION` +* `RAISE (RESUMABLE) EXCEPTION` * `RAISE RESUMABLE EXCEPTION` * `RAISE SHORTDUMP` -* `THROW` ( in conditions ) +* `THROW` (in conditions) + +Solely replacing the raising statement is usually not enough. You should also adjust the signature of the procedure to remove the "classic" exceptions and add the new class-based exceptions in a `RAISING` clause unless they are `CX_NO_CHECK` exceptions. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `“#EC NON_CL_EXCEPT`: +In exceptional cases, you can suppress this finding by using the pseudo comment `“#EC NON_CL_EXCEPT`. + +Note that this check is a subset of a similar check in the Extended Program Check (SLIN) delivered by SAP. That check accepts no pseudo comments or pragmas and places its findings on the location of defintion of the classic exception i.e. the `METHOD method_name EXCEPTIONS exception_name` declaration. We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. ```abap RAISE SYSTEM-EXCEPTIONS. "#EC NON_CL_EXCEPT diff --git a/docs/checks/number-attributes.md b/docs/checks/number-attributes.md index cbf51320..ee2a6308 100644 --- a/docs/checks/number-attributes.md +++ b/docs/checks/number-attributes.md @@ -2,21 +2,23 @@ ## Number of Attributes Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of attributes up to a maximum. When a class has has too many attributes, this is probably an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. +This check counts the number of attributes of an ABAP object and reports a finding when this exceeds a configurable threshold. When a class has many attributes, this can be an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) (SRP) is violated. ### How does the check work? -This check counts only `DATA` and `CLASS-DATA` within a global or local, `CLASS DEFINITION` or `INTERFACE`. Inherited attributes and all constants are not counted. A structure is counted as one attribute, no matter how many attributes are in the structure. +This check counts `DATA` and `CLASS-DATA` declarations within a global or local class definition or interface definition. Inherited attributes and constants are not counted as attributes for the purpose of this check. A structure is counted as one attribute, no matter how many components are in the structure. ### How to solve the issue? -Split the class or interface into multiple classes or interfaces which then contain less attributes. If there are many attributes related to one task, it's possible to group the attributes in structures. +If the SRP is violated, split the class or interface into multiple classes or interfaces, each containing a subset of its current attributes. Otherwise, if all attributes are needed for a single responsibility, consider grouping the attributes together in a meaningful structure or hierarchy of structures. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_ATTR` which should be placed right after the class definition header: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_ATTR` which should be placed right after the class definition header. + +Note that this check is equivalent to a subset of the "OO Size Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap CLASS class_name DEFINITION. "#EC NUMBER_ATTR diff --git a/docs/checks/number-events.md b/docs/checks/number-events.md index 9412a8f7..df1cfe50 100644 --- a/docs/checks/number-events.md +++ b/docs/checks/number-events.md @@ -2,21 +2,23 @@ ## Number of Events Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of events up to a maximum. When a class has too many events, it is an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. +This check counts the number of events of an ABAP object and reports a finding when this exceeds a configurable threshold. When a class has too many events, this can be an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) (SRP) is violated. ### How does the check work? -The check counts `EVENTS` and `CLASS-EVENTS` within a global or local, `CLASS DEFINITION` or `INTERFACE`. +The check counts `EVENTS` and `CLASS-EVENTS` statements within a global or local class definition or interface definition. Inherited events are not counted. ### How to solve the issue? -Split the class or interface into multiple classes or interfaces which then contain less events. +Split the class or interface into multiple classes or interfaces, each containing a subset of its current events. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_EVENTS` which should be placed right after the class definition header: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_EVENTS` which should be placed right after the class definition header. + +Note that this check is equivalent to a subset of the "OO Size Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap CLASS class_name DEFINITION. "#EC NUMBER_EVENTS diff --git a/docs/checks/number-executable-statements.md b/docs/checks/number-executable-statements.md index bff5a2e5..6dfc8918 100644 --- a/docs/checks/number-executable-statements.md +++ b/docs/checks/number-executable-statements.md @@ -2,26 +2,38 @@ ## Number of Executable Statements Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of non-declarative ABAP Statements per modularization unit up to a maximum. If there are too many statements in a code block, it is an indicator that the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. - -A high number of executable statements is an indicator that the source code might be not readable. In addition, the risk of introducing bugs is higher with a high number of executable statements in the code. +This check counts the number of executable ABAP statements per modularization unit and reports a finding when this exceeds a configurable threshold. If there are too many statements in a code block, this can be an indicator that the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) (SRP) is violated or that the code mixes different abstraction levels. ### How does the check work? -The check counts the number of non-declarative ABAP Statements, that is ABAP Statements without data declarations, program introduction statements, etc. +The check counts the number of executable ABAP statements, i.e. statement that are executed at runtime. This excludes statements like variable declarations that do not correspond to an action at runtime. + +```abap +DATA var TYPE i. +var = 1. +DATA(var_2) = 2. +``` + +The first statement is not executable, while the second and third are. ### How to solve the issue? -Modularize your code. Follow the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle). Split the method into other smaller methods (create service classes whenever possible). +Modularize your code by extracting smaller methods that each adhere to the SRP. Also, consider grouping these smaller methods in their own classes instead of having a lot of methods in the same class. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_NOES` which should be placed right after the `ENDMETHOD` statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC CI_NOES` which should be placed right after the `ENDMETHOD` statement. + +Note that this check is equivalent to a subset of the "Procedural Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap METHOD method_name. " ... ENDMETHOD. "#EC CI_NOES ``` + +### Further Reading + + - [Clean ABAP - Keep methods small](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#keep-methods-small) diff --git a/docs/checks/number-interfaces.md b/docs/checks/number-interfaces.md index a8abceef..fda5a98c 100644 --- a/docs/checks/number-interfaces.md +++ b/docs/checks/number-interfaces.md @@ -2,21 +2,23 @@ ## Number of Interfaces Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of interfaces up to a maximum. If there are too many interfaces in a class, it is an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. +This check counts the number of interfaces implemented by an ABAP object and reports a finding when this exceeds a configurable threshold. If there are too many interfaces in a class, this can be an indicator that the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. ### How does the check work? -This check counts `INTERFACES` within a global or local, `CLASS DEFINITION` or `INTERFACE`. +This check counts `INTERFACES` statements within a within a global or local class definition or interface definition. Inherited interfaces are not counted. ### How to solve the issue? -Split the class or interface into multiple classes or interfaces which then contain less interfaces. +If the interfaces belong to semantically distinct responsibilities, split the class or interface into multiple classes or interfaces, each with a single of the current interfaces. If multiple interfaces actually relate to the same kind of functionality, consider consolidating these interfaces into a single interface. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NMBR_INTERFACES` which should be placed right after the class definition header: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NMBR_INTERFACES` which should be placed right after the class definition header. + +Note that this check is equivalent to a subset of the "OO Size Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap CLASS class_name DEFINITION. "#EC NMBR_INTERFACES diff --git a/docs/checks/number-methods.md b/docs/checks/number-methods.md index ff07f647..e0dbcf97 100644 --- a/docs/checks/number-methods.md +++ b/docs/checks/number-methods.md @@ -2,21 +2,23 @@ ## Number of Methods Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of methods up to a maximum. If there are too many methods in a class, it is an indicator that the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. +This check counts the number of methods defined by an ABAP object and reports a finding when this exceeds a configurable threshold. If there are too many methods in a class, this can be an indicator that the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) is violated. ### How does the check work? -This check counts `METHODS` and `CLASS-METHODS` within a global or local, `CLASS DEFINITION` or `INTERFACE`. Inherited methods are not counted, however `REDEFINED METHODS` increment the counter. +This check counts `METHODS` and `CLASS-METHODS` statements within a global or local class definition or interface definition. Inherited methods are mostly not counted, but redefined inherited methods are. ### How to solve the issue? -The solution is to split the class or interface into multiple classes or interfaces which then contain less methods. +Split the class or interface into multiple classes or interfaces, each containing a subset of the current methods where the methods in subset relate to the same responsibility. ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_METHODS` which should be placed right after the class definition header: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUMBER_METHODS` which should be placed right after the class definition header. + +Note that this check is equivalent to a subset of the "OO Size Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap CLASS class_name DEFINITION. "#EC NUMBER_METHODS diff --git a/docs/checks/number-output-parameter.md b/docs/checks/number-output-parameter.md index c9a35a66..11ddff86 100644 --- a/docs/checks/number-output-parameter.md +++ b/docs/checks/number-output-parameter.md @@ -2,13 +2,17 @@ ## Number of Output Parameters Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for methods where more than one parameter in one of the output types (`EXPORTING`, `CHANGING` or `RETURNING`) is used. +This check searches for methods where more than one output parameter is used. Methods should [do one thing and do it well](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#do-one-thing-do-it-well-do-it-only), and having multiple output parameters indicates that that is not the case. + +### How does the check work? + +The check reports a finding whenever a method has more than one output parameter. An "output" parameter is any exporting, changing or returning parameter. ### How to solve the issue? -Use methods which return just one parameter per output type. If you have multiple output parameters which are related to each other, use a structure to group them. +If the method has only a single responsibility and the output parameters belong together logically, express that by making them all part of the same structure and using that structure as the output parameter. If the method has multiple responsibilities, follow the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) and create a dedicated method for each responsibility. ### What to do in case of exception? @@ -27,3 +31,8 @@ CLASS class_name DEFINITION. VALUE(result) TYPE c. "#EC NUM_OUTPUT_PARA ENDCLASS. ``` + +### Further Reading + + - [Clean ABAP - Parameter Number](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#parameter-number) + - [Clean ABAP - Parameter Types](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#parameter-types) \ No newline at end of file diff --git a/docs/checks/number-public-attributes.md b/docs/checks/number-public-attributes.md index 7f486369..ef7be877 100644 --- a/docs/checks/number-public-attributes.md +++ b/docs/checks/number-public-attributes.md @@ -2,21 +2,23 @@ ## Number of Public Attributes Check -### What is the Intent of the Check? +### What is the intent of the check? -This check counts the number of public attributes. All attributes should be private (by default) or protected (if needed). The [data encapsulation principle](https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)) helps you to protect your attributes from being changed and adds readability for others; the basic principle: ["One only sees what is needed"](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#members-private-by-default-protected-only-if-needed). +This check counts the number of public attributes and ABAP objects and reports a finding when it exceeds a configurable threshold. All attributes should be private (by default) or protected (if needed). The [data encapsulation principle](https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)) helps you to protect your attributes from being changed and adds readability for others; the basic principle is that [one only sees what is needed](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#members-private-by-default-protected-only-if-needed). ### How does the check work? -This Check counts only `DATA` and `CLASS-DATA` within a global or local, `CLASS DEFINITION` or `INTERFACE`. Inherited attributes and `CONSTANTS` aren't counted. A structure is counted as one attribute, no matter how many attributes are in the structure. +This check counts only `DATA` and `CLASS-DATA` statements within the public section of a global or local class definition or interface definition. Inherited attributes and constants are not counted as attributes for the purpose of this check. A structure is counted as one attribute, no matter how many components are in the structure. ### How to solve the issue? -Make those attributes `PRIVATE` or `PROTECTED`. You can grant the read access with a getter method (for example, `get_user_first_name`). With a setter, you can also grant write access and have the possibility to validate the inserted data (for example, `set_time_in_seconds` with a test to allow only positive numbers). +Make all attributes private or protected. You can grant read access with getter methods and write access with setter methods instead. For immutable attributes, [consider using `READ-ONLY` public attributes](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#consider-using-immutable-instead-of-getter). ### What to do in case of exception? -In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUM_PUBLIC_ATTR` which should be placed right after the `PUBLIC SECTION` statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC NUM_PUBLIC_ATTR` which should be placed right after the `PUBLIC SECTION` statement. + +Note that this check is equivalent to a subset of the "OO Size Metrics" check delivered by SAP. That check accepts no pseudo comments or pragmas. We recommend that you *either* use this Code Pal check *or* the SAP-delivered check, but not both, since if you use both you get two findings for the exact same issue. ```abap CLASS class_name DEFINITION. @@ -27,6 +29,6 @@ ENDCLASS. ``` ### Further Reading -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#members-private-by-default-protected-only-if-needed) +* [Clean ABAP - Private by default](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#members-private-by-default-protected-only-if-needed) diff --git a/docs/checks/omit-optional-exporting.md b/docs/checks/omit-optional-exporting.md index 41a07660..9fb1bfe9 100644 --- a/docs/checks/omit-optional-exporting.md +++ b/docs/checks/omit-optional-exporting.md @@ -2,13 +2,13 @@ ## Omit Optional EXPORTING -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the optional EXPORTING wording which makes the class method call needlessly longer: +In functional calls of methods that only have importing parameters and a returning parameter, the keyword `EXPORTING` in front of the parameter list at call sites is superfluous. This check searches for these superfluous instances of `EXPORTING`. Anti-Pattern: ```abap -modify->update( +update( EXPORTING node = /dirty/my_bo_c=>node-item key = item->key @@ -18,20 +18,19 @@ modify->update( Pattern: ```abap -modify->update( node = /clean/my_bo_c=>node-item - key = item->key - data = item - fields = changed_fields ). +update( node = /clean/my_bo_c=>node-item + key = item->key + data = item + fields = changed_fields ). ``` - ### How does the check work? -This check searches for the usage of the optional keyword EXPORTING in the class method calls. +This check searches for the usage of `EXPORTING` in functional method calls that have no `CHANGING`, `IMPORTING`, `RECEIVING` or `EXCEPTIONS` clauses. ### How to solve the issue? -Omit the optional keyword EXPORTING (it works implicitly). +Omit the optional keyword `EXPORTING`. ### What to do in case of exception? diff --git a/docs/checks/optional-parameters.md b/docs/checks/optional-parameters.md index bc31df3a..cacf4e40 100644 --- a/docs/checks/optional-parameters.md +++ b/docs/checks/optional-parameters.md @@ -2,19 +2,21 @@ ## Optional Parameters -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for OPTIONAL parameter in method signatures. It is recommended to avoid the usage of `OPTIONAL` parameters because they might confuse the consumers of the class: +This check searches for optional parameters in method signatures. It is recommended to avoid optional parameters because they might confuse the consumers of the class about questions like: * Which parameters are really required? * Which combination of parameters are valid? * Which parameters exclude each other? -Multiple methods with specific parameters for the use case avoid this confusion by giving clear guidance which parameter combinations are valid and expected. +It also becomes harder to perform any changes in the method since all possible combinations of its parameters must always be considered. + +Multiple methods with specific parameters for their respective use cases avoid this confusion by giving clear guidance which parameter combinations are expected. ### How to solve the issue? -Splitting methods (creating new ones) instead of adding `OPTIONAL` parameters. +Create new methods dedicated to their specific use case instead of adding optional parameters to a generic method. ### Example diff --git a/docs/checks/prefer-case-to-elseif.md b/docs/checks/prefer-case-to-elseif.md index 9a14e266..a747ef70 100644 --- a/docs/checks/prefer-case-to-elseif.md +++ b/docs/checks/prefer-case-to-elseif.md @@ -1,14 +1,16 @@ [code pal for ABAP](../../README.md) > [Documentation](../check_documentation.md) > [Prefer CASE to ELSE IF](prefer-case-to-elseif.md) -## Prefer CASE to ELSE IF +## Prefer CASE to ELSEIF -### What is the Intent of the Check? +### What is the intent of the check? -Prefer `CASE` to `ELSEIF` for multiple alternative conditions because `CASE` makes it easy to see a set of alternatives that exclude each other. It can be faster than a series of `IF`s because it can translate to a different microprocessor command instead of a series of subsequently evaluated conditions. You can introduce new cases quickly, without having to repeat the discerning variable over and over again. The statement even prevents some errors that can occur when accidentally nesting the `IF`-`ELSEIF`s. +Conditions with many branches, i.e. `IF` statements with many `ELSEIF` alternatives, are often hard to read. When applicable, a `CASE` statement where all branches are on equal footing as `WHEN` branches is often much more readable. -In short: If the IF-Statement has operations (like AND, OR, …) over several variables/attributes, it is not worthwhile to revert it to a CASE-Statement. In other words, conditions on multiple variables can be left in IF-statements. Only condition(s) on single variables should be converted to a CASE-Statement. +The check reports a finding when the number of branches exceeds a configurable threshold. -The threshold determines the maximum number of conditions. +### How does the check work? + +The check finds `IF`-`ELSEIF` chains where each condition consists only of a single logical expression and the first token of all logical expressions is the same. ### How to solve the issue? @@ -33,9 +35,9 @@ ENDIF. Before the check: ```abap -IF type = type-some_type. +IF type = types-some_type. " ... -ELSEIF type = type-some_other_type. +ELSEIF type = types-some_other_type. " ... ELSE. RAISE EXCEPTION NEW /dirty/unknown_type_failure( ). @@ -57,4 +59,4 @@ ENDCASE. ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-case-to-else-if-for-multiple-alternative-conditions) +* [Clean ABAP - Prefer CASE to ELSEIF](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-case-to-else-if-for-multiple-alternative-conditions) diff --git a/docs/checks/prefer-is-not-to-not-is.md b/docs/checks/prefer-is-not-to-not-is.md index ba67d8a7..f32b3852 100644 --- a/docs/checks/prefer-is-not-to-not-is.md +++ b/docs/checks/prefer-is-not-to-not-is.md @@ -2,7 +2,7 @@ ## Prefer IS NOT to NOT IS -### What is the Intent of the Check? +### What is the intent of the check? Prefer `IS ... NOT` to `NOT ... IS` because it requires a "mental turnaround" that makes it harder to understand the negation logic. @@ -67,4 +67,5 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-is-not-to-not-is) +* [Clean ABAP - Try to make conditions positive](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#try-to-make-conditions-positive) +* [Clean ABAP - Prefer `IS NOT` to `NOT IS`](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-is-not-to-not-is) diff --git a/docs/checks/prefer-new-to-create-object.md b/docs/checks/prefer-new-to-create-object.md index 9d44a227..e1693595 100644 --- a/docs/checks/prefer-new-to-create-object.md +++ b/docs/checks/prefer-new-to-create-object.md @@ -2,14 +2,13 @@ ## Prefer New to Create Object -### What is the Intent of the Check? +### What is the intent of the check? -Prefer `NEW` over `CREATE OBJECT` as it avoids needlessly longer statements. +This check searches for `CREATE OBJECT` statements and reports a finding if the type of the instance being created is known statically. Static instance creation with the functional `NEW` constructor allows for inline declarations and more concise code. ### How to solve the issue? -Preferably, use `NEW` for creating new objects/instances. - +Use `NEW` to create instances of objects when the type is known statically. ### What to do in case of exception? In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC PREF_NEW`: @@ -41,4 +40,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-new-to-create-object) +* [Clean ABAP - Prefer NEW to CREATE OBJECT](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-new-to-create-object) diff --git a/docs/checks/prefer-returning-to-exporting.md b/docs/checks/prefer-returning-to-exporting.md index 60796f0d..69ce0a3e 100644 --- a/docs/checks/prefer-returning-to-exporting.md +++ b/docs/checks/prefer-returning-to-exporting.md @@ -2,15 +2,14 @@ ## Prefer RETURNING to EXPORTING -### What is the Intent of the Check? +### What is the intent of the check? -Based on the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-returning-to-exporting), this check searches in classes and interfaces for methods that have only one exporting parameter. If it finds one, it will recommend you to change it from `EXPORTING` to `RETURNING`. +This check searches for methods in ABAP objects that have only a single exporting parameter. ### How to solve the issue? -Change the `EXPORTING` parameter to `RETURNING`. +Change the exporting parameter to a returning parameter, if possible. This may not be possible when the exporting parameter is not fully typed (i.e. at least partly generic) as returning parameters must be fully typed. -:bulb: [A RETURNING parameter must be fully typed (#218)](https://github.com/SAP/styleguides/issues/218) ### What to do in case of exception? diff --git a/docs/checks/pseudo-comment-usage.md b/docs/checks/pseudo-comment-usage.md index 39881821..fea70753 100644 --- a/docs/checks/pseudo-comment-usage.md +++ b/docs/checks/pseudo-comment-usage.md @@ -2,19 +2,19 @@ ## Pseudo Comment Usage Check -### What is the Intent of the Check? +### What is the intent of the check? This check lists the number of "code pal for ABAP" pseudo comments per object being used. Pseudo comments completely suppress the findings in ATC. Thus, this check can be used whether objects without any other finding use a lot of pseudo comments to suppress findings. ### How does the check work? -It simply counts the number of used code pal for ABAP pseudo comments. +It simply counts the number of code pal for ABAP pseudo comments being used. ### How to solve the issue? -Solve the other issues so that less pragmas are used. +Solve the other issues so that fewer pseudo comments are used. ### What to do in case of exception? -There is no exception as this check works as an indicator. Thus, it is not possible to suppress Code Inspector findings from this check. +There is no exception as this check is merely counting pseudo comments and so its findings do not represent any specific issue. diff --git a/docs/checks/receiving-usage.md b/docs/checks/receiving-usage.md index d9b93f77..6ab6a8e8 100644 --- a/docs/checks/receiving-usage.md +++ b/docs/checks/receiving-usage.md @@ -2,17 +2,22 @@ ## RECEIVING Statement Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the `RECEIVING` statement which should no longer be used. +This check searches for `RECEIVING` clauses in method calls which should no longer be used. The only case in which it is necessary to use `RECEIVING` rather than functional notation is when an `EXCEPTIONS` clause to catch classic exceptions is present. ### How to solve the issue? -`RECEIVING` shall not be used. +Replace the `RECEIVING` clause with its functional equivalent: ```abap DATA(sum) = aggregate_values( values ). ``` +instead of +```abap +DATA sum TYPE i. +aggregate_values( EXPORTING values = values RECEIVING result = sum ). +``` ### What to do in case of exception? @@ -28,4 +33,4 @@ aggregate_values( ### Further Readings & Knowledge -* [Clean ABAP - Omit RECEIVING Statement](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#omit-receiving) +* [Clean ABAP - Omit RECEIVING](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#omit-receiving) diff --git a/docs/checks/returning-name.md b/docs/checks/returning-name.md index 65ef5c9d..d5aa4c04 100644 --- a/docs/checks/returning-name.md +++ b/docs/checks/returning-name.md @@ -2,14 +2,13 @@ ## Returning Name -### What is the Intent of the Check? +### What is the intent of the check? -Good/proper method names are usually so meaningful, that the `RETURNING` parameter does not need a name of its own. -The name would do little more than parrot the method name or repeat something obvious. +Suitable method names are usually meaningful enough that the returning parameter does not need a name of its own. The name would do little more than parrot the method name or repeat something equally obvious, so this check reports a finding for all returning parameters that are not named `RESULT`. ### How to solve the issue? -Calling the `RETURNING` parameter as `RESULT`. +Call all returning parameters `RESULT`. ### What to do in case of exception? @@ -41,4 +40,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#consider-calling-the-returning-parameter-result) +* [Clean ABAP - Consider calling the RETURNING parameter RESULT](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#consider-calling-the-returning-parameter-result) diff --git a/docs/checks/scope-of-variable.md b/docs/checks/scope-of-variable.md index a0dd6d63..174dcab6 100644 --- a/docs/checks/scope-of-variable.md +++ b/docs/checks/scope-of-variable.md @@ -2,23 +2,14 @@ ## Scope of Variable -### What is the Intent of the Check? -If a variable is declared in a statement, it should be used/referred to inside this statement only (not outside). +### What is the intent of the check? +If a variable is declared inside a code block that forms a meaningful subdivision of scope (such as the branches of an `IF` statement), it should be used only inside that subdivision. ABAP lacks this sort of subdivided scope handling on a language level and does not prevent such confusing accesses - the smallest scope in ABAP is the method (excluding helper variables in certain constructor expressions) and any variable declared in a method remains visible until the end of the method. ### How does the check work? -It searches for `DATA` and `FIELD-SYMBOLS` declaration inside of `IF`, `ELSEIF`, `ELSE`, `DO`, `CASE/WHEN`, `LOOP`, and `WHILE` statements, and for its usage/reference outside this statement. - -ABAP lacks of proper scope handling. If a variable is declared in a IF-Block, it should only be used/referred inside this IF-block (not outside). The same applies for LOOP, DO, WHILE or any block structure. In other words, it is not allowed the usage of a variable outside the block/scope where it was declared. Thus, it is still possible to make usage of dynamic declarations inside of blocks with a single statement: - -```abap -IF condition = abap_true. - DATA(entry) = method(). -ENDIF. -entry = abap_true. "no longer accepted -``` +The check searches for variable declarations (`DATA`, `FIELD-SYMBOLS`) inside of `IF`, `ELSEIF`, `ELSE`, `DO`, `CASE/WHEN`, `LOOP`, and `WHILE` blocks and for the usage of these variables outside these blocks. ### How to solve the issue? -Relocate the declaration. +Relocate the declaration if the access to the variable is intended or use a different variable for the outside access. ### What to do in case of exception? In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC SCOPE_OF_VAR` which should be placed right after the variable usage/reference: @@ -44,8 +35,7 @@ ENDIF. After: ```abap -DATA value TYPE i. - +DATA(value) = 0. IF has_entries = abap_true. value = 1. ELSE. @@ -54,4 +44,4 @@ ENDIF. ``` ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#dont-declare-inline-in-optional-branches) +* [Clean ABAP - Don't declare inline in optional branches](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#dont-declare-inline-in-optional-branches) diff --git a/docs/checks/self-reference.md b/docs/checks/self-reference.md index 739a06ff..ec36b80c 100644 --- a/docs/checks/self-reference.md +++ b/docs/checks/self-reference.md @@ -2,13 +2,13 @@ ## Self-Reference -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the usage of self-reference `me->`. Since this self-reference is implicitly set by the system, you should omit it when calling an instance method. +This check searches for unnecessary usages of explicitly self-references using `me->` to qualify an instance variable or method. Since this self-reference is implicitly set by the system, it should be omitted. ### How to solve the issue? -Omitting the self-reference whenever calling an instance method. +Omit the self-reference. ### What to do in case of exception? @@ -34,4 +34,4 @@ After the check: ### Further Readings & Knowledge -* [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#omit-the-self-reference-me-when-calling-an-instance-method) +* [Clean ABAP - Omit self-references](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#omit-the-self-reference-me-when-calling-an-instance-attribute-or-method) diff --git a/docs/checks/sub-assign-read-table.md b/docs/checks/sub-assign-read-table.md index 168b08fc..09753613 100644 --- a/docs/checks/sub-assign-read-table.md +++ b/docs/checks/sub-assign-read-table.md @@ -4,24 +4,24 @@ ### What is the Intent of the Check? -This check aims to prevent undesired changes to internal tables using field symbols in context of the `READ TABLE` statement with `INTO` instead of `ASSIGNING`. +This check tries to prevent undesired changes to the content referenced by a field symbol. Using an `INTO` clause with a field symbol overwrites the referenced content when the programmer might have intended to assign the field symbol instead. ### How does the check work? -This check finds `READ TABLE` statements which use statement `INTO` for assignment but using a field symbol instead of a variable. +This check searches for `READ TABLE` statements which use an `INTO` clause with a field symbol as its target. ### How to solve the issue? -Use `ASSIGNING` instead of `INTO` in combination with field symbols as otherwise the selected table row will be overwritten. +Use `ASSIGNING` instead of `INTO` in combination with field symbols. ### What to do in case of exception? In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC SUB_ASSIGN` which should be placed after the closing dot of the `READ TABLE` statement: ```abap -READ TABLE itab ASSIGNING FIELD-SYMBOL() WHERE key = '1'. +READ TABLE itab ASSIGNING FIELD-SYMBOL() WITH KEY key = '1'. " Do something with -READ TABLE itab INTO WHERE key = '2'. "#EC SUB_ASSIGN +READ TABLE itab INTO WITH KEY key = '2'. "#EC SUB_ASSIGN " Do something with ``` diff --git a/docs/checks/test-seam-usage.md b/docs/checks/test-seam-usage.md index dab393f0..eadce07f 100644 --- a/docs/checks/test-seam-usage.md +++ b/docs/checks/test-seam-usage.md @@ -2,10 +2,9 @@ ## TEST-SEAM Statement Usage Check -### What is the Intent of the Check? +### What is the intent of the check? -This check searches for the usage of the ABAP statement: TEST-SEAM. -Test seams are invasive and tend to get entangled in private dependencies, such that they are hard to keep alive and stable in the long run. +This check searches for the usage of the ABAP statement `TEST-SEAM`. Test seams are invasive and tend to get entangled in private dependencies, so that they are hard to keep alive and stable in the long run. ### How does the check work? diff --git a/docs/checks/text-assembly.md b/docs/checks/text-assembly.md index ea9f7ea0..58887bbe 100644 --- a/docs/checks/text-assembly.md +++ b/docs/checks/text-assembly.md @@ -4,11 +4,11 @@ ### What is the Intent of the Check? -Following the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#use--to-assemble-text), this check searches for text assembly and suggests the usage of the `|` to perform it. +This check searches for locations where text is assembled from different fragments and string templates (marked by `|`) are not used. ### How to solve the issue? -Use `|` to assemble the text +Use string templates to assemble text from smaller pieces. ### What to do in case of exception? diff --git a/docs/checks/unit-test-coverages.md b/docs/checks/unit-test-coverages.md index be8721af..9ab6d747 100644 --- a/docs/checks/unit-test-coverages.md +++ b/docs/checks/unit-test-coverages.md @@ -2,15 +2,15 @@ ## Unit-Test Coverages (Statement, Branch and Procedural Coverage) -### What is the Intent of the Check? +### What is the intent of the check? -This check executes the ABAP Unit-Test framework and returns the current coverage percentile for the object in question. It verifies if the coverage is under the defined thresholds (customized in the checks). +This check executes the ABAP Unit framework and measures the current coverage percentile for the object in question. It reports a finding if the coverage is below a customizable threshold. -> :WARNING: Use this check with a small set of objects only (due to possible performance issues). +There are three versions of this check for the three different types of coverage that can be measured: Statement coverage, branch coverage and procedure coverage. ### How to solve the issue? -Improve the Unit-Test coverage(s) by writing Unit-Tests. +Improve the unit test coverage by writing additional unit tests. ### What to do in case of exception? diff --git a/docs/checks/unit_test_assert.md b/docs/checks/unit_test_assert.md index be4ab47a..ccd90113 100644 --- a/docs/checks/unit_test_assert.md +++ b/docs/checks/unit_test_assert.md @@ -2,16 +2,13 @@ ## Unit-Test Assert Validator -### What is the Intent of the Check? +### What is the intent of the check? -This check verifies invalid assertions in unit tests. -It supports the `CL_ABAP_UNIT_ASSERT=>ASSERT*` and `CL_AUNIT_ASSERT=>ASSERT*`. +This check searches for assertions in unit tests that do not fulfill any actual purpose. ### How does the check work? -It checks for actual (`act`) or expected (`exp`) invalid value(s), for instance: -- When both are using the same variable for the Assertion (which will always return TRUE); -- When both are hardcoded. +The check looks at the parameters `act` and `exp` of calls to methods whose name contains `assert`. It raises a finding if both parameters are identical or if both are literals. ### How to solve the issue? @@ -19,7 +16,7 @@ Fix the actual (`act`) or expected (`exp`) value(s) in the unit test assertion i ### What to do in case of exception? -In exceptional cases (if any), you can suppress this finding by using the pseudo comment `"#EC UT_ASSERT` which has to be placed after the assertion statement: +In exceptional cases, you can suppress this finding by using the pseudo comment `"#EC UT_ASSERT` which has to be placed after the assertion statement: ```abap cl_abap_unit_assert=>assert_equals( act = sum diff --git a/pages/how-to-install.md b/pages/how-to-install.md index a09db487..a96a2cf9 100644 --- a/pages/how-to-install.md +++ b/pages/how-to-install.md @@ -22,11 +22,3 @@ Start the transaction `SCI` again, and create a new global check variant. Then, select the `code pal for ABAP` group and save it. ![how to create code inspector variant](imgs/sci-check-variant.png) - -### 4. User Parameter - -It requires you to set the ABAP Test Cockpit (ATC) to run in Code Inspector mode. - -Start the transaction `SU3`, and add/set the user parameter `SATC_CI_MODE` to `X`: - -![user parameter](imgs/user-parameter.png) \ No newline at end of file From 7b5ad1d7e5a6f77482d102b66057eaba86fec4d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20J=C3=BCliger?= Date: Mon, 8 Aug 2022 14:39:27 +0200 Subject: [PATCH 4/5] Fix typo --- docs/checks/non-class-exception.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/checks/non-class-exception.md b/docs/checks/non-class-exception.md index fbd68674..596f0479 100644 --- a/docs/checks/non-class-exception.md +++ b/docs/checks/non-class-exception.md @@ -28,7 +28,7 @@ Solely replacing the raising statement is usually not enough. You should also ad In exceptional cases, you can suppress this finding by using the pseudo comment `“#EC NON_CL_EXCEPT`. -Note that this check is a subset of a similar check in the Extended Program Check (SLIN) delivered by SAP. That check accepts no pseudo comments or pragmas and places its findings on the location of defintion of the classic exception i.e. the `METHOD method_name EXCEPTIONS exception_name` declaration. We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. +Note that this check is a subset of a similar check in the Extended Program Check (SLIN) delivered by SAP. That check accepts no pseudo comments or pragmas and places its findings at the location of definition of the classic exception, i.e. the `METHOD method_name EXCEPTIONS exception_name` declaration. We recommend that you *either* use this Code Pal check *or* the corresponding SLIN check, but not both, since if you use both you get two findings for the exact same issue. ```abap RAISE SYSTEM-EXCEPTIONS. "#EC NON_CL_EXCEPT From 8aff4079c170876cf3824e320500852cdb6df9d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20J=C3=BCliger?= Date: Mon, 8 Aug 2022 15:01:06 +0200 Subject: [PATCH 5/5] closes #541 --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 7ddc474f..9397baf7 100644 --- a/README.md +++ b/README.md @@ -4,8 +4,7 @@ [![license](https://img.shields.io/github/license/SAP/code-pal-for-abap)](LICENSE) [![REUSE status](https://api.reuse.software/badge/github.com/SAP/code-pal-for-abap)](https://api.reuse.software/info/github.com/SAP/code-pal-for-abap) -Based on the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md), this tool contains a set of checks to guarantee the [Clean ABAP](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md) adherence. -Together, we both support you in writing a clean ABAP code. +This tool provides a set of checks to help adhering to the [Clean ABAP style guide](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md). While not all recommendations from the guide can be checked by static code analysis and in certain situations strict adherence to the guide might not be recommended, this provides robust automated support at least for a subset of recommendations. ❣️ It's **free** and **open-source**.