1357 246-
Get Started
- CodeAnt AI
- Setup
- Control Center
- Pull Request Review
- IDE
- Compliance
- Anti-Patterns
- Code Governance
- Infrastructure Security Database
- Application Security Database
Anti-Patterns
Rpg
IMPORTed values should be treated as read-only. Doing otherwise could result in unpleasant surprises down the line.
D VAR2 S 5 0 IMPORT
* Noncompliant
C MOVE MYVAR VAR2
C/FREE
// Noncompliant
VAR2=MYVAR;
/END-FREE
The DEBUG(*YES) and DUMP statements are useful during development and debugging, but could expose sensitive information to attackers and should not be included in production code.
H*-------------------------------------------------------------------------
H DATEDIT(*YMD) DEBUG(*YES)
H**************************************************************************
C SR990 BegSR
C 'CVTERR' DUMP DUMP for error
C Move *on *INLR
Even if there’s no data to be returned from a procedure, each procedure should at least return a boolean to indicate when procedure execution failed, so callers can respond accordingly.
D MyProc PI
D param1 10A const
/free
// do work...
return; // Noncompliant
/end-free
While `END will adequately close a statement, it is less clear than the use of the relevant, statement-specific ENDxx. Thus, the statement-specific version is preferred to facilitate code maintenance and enhance clarity.
This rule is applied to the following operations: CASxx, DO, DOU, DOUxx, DOW, DOWxx, FOR, IF, IFxx and SELECT` groups.
C W0PKEX DOUEQ W0ON
...
C END
C W0PKEX IFEQ W0ON
...
C END
The complexity of an expression is defined by the number of AND, OR.
A single expression’s complexity should not become too high to keep the code readable.
C IF (condition1 AND condition2)
C OR (condition2 AND condition2)
C OR (condition3 AND condition4)
When multiple, adjacent MONITOR statements have duplicate ON-ERROR blocks, they should be merged to consolidate the ON-ERROR logic for cleaner, more readable code. Note that this applies even when there is intervening code outside any MONITOR block.
/free
monitor;
// do something...
on-error;
CALLP HandleError(*param);
endmon;
// do un-monitored thing
monitor;
// do something else...
on-error; // Noncompliant
CALLP HandleError(*param);
endmon;
/end-free
Shared coding conventions allow teams to collaborate effectively. This rule checks that labels are on lines by themselves, which makes them more immediately visible as such.
LBLVAL: IF COND(&ERRFLG = ' ') THEN (DO)
CHGVAR VAR(&MSG) VALUE('Validation Completed')
ENDDO
The format used to write a record to a file should be cleared before each use. Otherwise stale data left in the format from previous records may be saved into the current record if it does not have data for all the fields in the format.
C IF X <> Y
...
C ENDIF
C WRITE RECFMT
Leaving an ON-ERROR block empty means that the exception in question is neither handled nor passed forward to callers for handling at a higher level. Suppressing errors rather than handling them could lead to unpredictable system behavior and should be avoided.
/free
monitor;
// ...
on-error *FILE; // Noncompliant
endmon;
/end-free
The use of UNDERLINE makes normal text printed to the screen difficult to read, and prevents titles from being read at all. For these reasons, UNDERLINE should not be used.
R DETAIL1
* Noncompliant; next line contains UNDERLINE
NAME 25 2 2UNDERLINE
ADD1 25 3 2
R DETAIL3
ADD2 25 2SPACEB(1)
Using a unique symbolic `PLIST name provides a single definition point for the parameters used by any external routine being called. This ensures that the same information is passed every time the external routine is called, reducing the potential for mistakes in operation.
This rule flags non-compliant instances of CALL and CALLB`.
C CALLB(D) 'XR01'
C PARM WSXR07
C PARM WSFLD1
Each program should contain an initial section of H* comments lines, which includes a copyright and/or license statement. This rule must be fed with the header text that is expected in the header.
H* Copyright (C) 2008-2013 SonarSource
H* mailto:contact AT sonarsource DOT com
H*
H* SonarQube is free software; you can redistribute it and/or
H* modify it under the terms of the GNU Lesser General Public
H* License as published by the Free Software Foundation; either
H* version 3 of the License, or (at your option) any later version.
H*
H* SonarQube is distributed in the hope that it will be useful,
H* but WITHOUT ANY WARRANTY; without even the implied warranty of
H* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
H* Lesser General Public License for more details.
H*
H* You should have received a copy of the GNU Lesser General Public License
H* along with this program; if not, write to the Free Software Foundation,
H* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
Historically /COPY procedures contained all of the specifications required for each procedure, so for example xxxxF /COPY would be used to copy the F lines from another F spec. However, this practice results in code that is difficult to understand and maintain. Instead, the use of /COPY statements should be replaced with explicit declarations.
F/COPY HRSILERPG,RSX36
Turning on the `*srcstmt header option means that any line numbers cited in error statements will actually correspond correctly to the code. Otherwise, object line numbers will be shown, making errors difficult to debug.
The *srcstmt option is also recommended for accurate line numbers during debugging, particularly in combination with *nodebugio, (H option(*srcstmt:*nodebugio)`) which prevents debug operations from stopping on I/O.
H*
H* AMENDED BY : G. Ann Campbell
H* DATE : 16JUL2014
H* H24 - Miscellaneous Online Maintenance/Enquiry/Printing
H*
F/EJECT
If a file is defined without the USROPN statement then the natural RPG logic will deal with opening and closing it. However, files defined with USROPN, must be both explicitly OPENed and CLOSEd.
FEWPCCR1 O E PRINTER USROPN
F INFDS(WSFD01)
F INFSR(*PSSR)
C OPEN EWPCCR1
C CLOSE *ALL
Using both SPACEA (space-after) and SPACEB (space-before) in the same printer file can make it difficult to understand how many blank lines will be printed where. Therefore either one command or the other should be used but not both.
A R REP001 TEXT('Detail line. ')
A SPACEB(1)
A SPACEA(1)
A 2'Additional Details for Student :'
A VAR01 6 36TEXT('Additional Details for +
A Student')
A Select or a chain of If/Elseif statements is evaluated from top to bottom. At most, only one branch will be executed: the first one with a condition that evaluates to true.
Therefore, duplicating a condition automatically leads to dead code. Usually, this is due to a copy/paste error. At best, it’s simply dead code and at worst, it’s a bug that is likely to induce further bugs as the code is maintained, and obviously it could lead to unexpected behavior.
C IF X = 1
C EXSR SR01
C ELSEIF X = 1 Noncompliant
C EXSR SR02
C ENDIF
Using a named constant to refer to an indicator makes the content of the field clearer, and therefore makes the code easier to read and maintain.
D accountTotal c 25
/Free
If *In(accountTotal);
// Process contents...
EndIf;
Having all the pieces in front of you makes it easier to understand and debug or maintain a piece of code. Unfortunately, that’s often not the case with compile-time arrays, since they are often separated from the code that uses them by many, many lines. Instead, data structures are often a better option.
D directions S 5 0 dim(4) ctdata perrcd(1)
Externally described files standardize file access, and result in simpler, easier to read code.
FEPBLMSL2 IF F 100 DISK INFSR(*PSSR)
F*
F*
Testing %PARMS before the use of one or more parameters is a clear indication in the code that at least some parameters are treated as optional. However, failure to actually mark those parameters as such with options( *nopass ) denies callers the ability to treat them as optional. Since all parameters after the first one explicitly declared options( *nopass ) are also implicitly optional, this marking is only required once. However, for clarity you should consider explicitly marking all optional parameters.
DCL-PR VIOLATION1 EXTPGM; // Noncompliant
LASTNAME Char(10);
FIRSTNAME Char(10);
END-PR;
DCL-PI VIOLATION1;
LASTNAME Char(10);
FIRSTNAME Char(10);
END-PI;
dsply FIRSTNAME;
IF %PARMS > 1;
dsply LASTNAME;
EndIf;
*inlr = *on;
Eval and Callp are the only two exceptions to the rule that each free-format line must start with an operation code. Since you can leave these two opcodes out, you should for cleaner, more readable code.
Eval Regpay = hours * perHour;
Callp calcTax();
This rule applies whenever an `IF statement is followed by one or more ELSEIF statements; the final ELSEIF should be followed by an ELSE statement.
The requirement for a final ELSE statement is defensive programming.
The ELSE statement should either take appropriate action or contain a suitable comment as to why no action is taken. This is consistent with the requirement to have a final OTHER clause in a SELECT` statement.
C IF RESULT = 0
C ...
C ELSEIF RESULT > 0
C ...
C ENDIF
Using a unique `KLIST to access files ensures that the correct key is used at all times and creates a standard method of accessing the data throughout the code, simplifying maintenance and improving readability.
This rule flags non-compliant instances of CHAIN, DELETE, READE, READPE, SETGT, and SETLL` operations.
C 'D001 B' CHAIN KYCUSTN 20
...
C DEPT CHAIN KYCUSTN 20
Shared naming conventions allow teams to collaborate efficiently.
This rule raises an issue when a subroutine name does not match a provided regular expression.
For example, with the default provided regular expression ^SR[a-zA-Z0-9]*$, the following subroutines:
C XXXXX BEGSR
...
C ENDSR
Nested control flow statements CASxx, DO, DOU, DOW, DOUxx, DOWxx, FOR, IF, IFxx, MONITOR and SELECT are often key ingredients in creating
what’s known as “Spaghetti code”. This code smell can make your program difficult to understand and maintain.
When numerous control structures are placed inside one another, the code becomes a tangled, complex web. This significantly reduces the code’s readability and maintainability, and it also complicates the testing process.
IF A <> B
IF B <> C
DOW B <> A
IF A <> D
...
ENDIF
ENDDO
ENDIF
ENDIF
When storing numeric fields, using an odd number of digits allows the sign to be included in the storage area without wasting any space.
For example the number -1234567, when packed as 7 digits is stored as follows.
thus taking up only 4 data slots. but when packed as 8 characters it uses 5 data slots
02467 0135-
or when 7 digits, non-packed, it will be stored as 8 data slots, including the sign.
FFFFFFF6 12345670
D NUMFLD S 8P 0
D NUMFLD S 7S 0
Subprocedures and subroutines are both mechanisms to segregate logic, but subprocedures are preferred for three reasons:
their local files and variables make maintenance faster and cleaner. They allow you to create variables without worrying about name clashes, and to change fields without worrying about negatively impacting other parts of the program.
their local files and variables make code reuse easy.
they can be called with parameters as functions, yielding clearer more readable code.
D FirstName S 20A
D Initial S 1A
D LastName S 20A
D FullName S 43A
/free
FirstName = 'John';
Initial = 'A';
LastName = 'Smith';
EXSR SPFullName;
DSPLY FullName;
...
begsr SPFullName;
FullName = FirstName + ' ' + Initial + ' ' + LastName;
endsr;
/end-free
Shared coding conventions allow teams to collaborate effectively. While it is possible to omit the specification letter before a /COPY statement, it is advisable not to do so because including a specification on the line indicates to other developers what type of statements will be added to the program by the copybook. Further, using a specification letter on the line makes it clearer to those who might be skimming the file that the line is not just a comment, but does contain code.
/COPY MBR1
Every subroutine should be commented to explain its goal and how it works. This non-empty comment must be located before or after the subroutine definition.
C SR012 BEGSR
C ...
C ENDSR
Global variables can seem like a handy way to pass state information around in a program, but the use of global variables only works well in very small programs. As the code base grows, you’ll need to understand every subprocedure’s impact on the global state in order to understand how the program works. This is a task that quickly becomes impossible.
To control the situation, only the main procedure should be allowed access to global variables; it can then pass that state information to subprocedures as parameters.
D FirstName S 20A
D LastName S 20A
/free
FirstName = 'John';
LastName = 'Smith';
DSPLY FullName();
/end-free
P FullName B
D FullName PI 41A
/free
return FirstName + ' ' + LastName;
/end-Free
P E
Error handling should be defined on file operations to ensure correct processing of program failures rather than defaulting to an abnormal program end. This rule checks two things:
INFSRerror handling is defined on file operations.error handling is delegated to an appropriately named routine.
F* Noncompliant
FIPBDCCP IF E K DISK
FEP210 CF E WORKSTN
F SFILE(EP21003S:RELN03)
FEP471R1 O E PRINTER
F USROPN
F INFDS(W1SF01)
GOTO is an unstructured control flow statement. It makes code less readable and maintainable, and should only be used to branch to defined ENDSR points within the source. For normal loop functions use ITER or LEAVE instead.
C LOOP1 TAG
...
C GOTO LOOP1
Both the name and the field definitions of a parameter 2 data area used in an IN should be coded in the D specification. This ensures that all definition data is kept together, and the specific data is not manually extracted in the code, thus making the code easier to read and maintain.
D WSFD08 S 30A 0
C IN WSFD08
C MOVEL WSFD08 VAR9S
The use of positional notation to describe the overlapping of one field on another can quickly become confusing, particularly in large, or complicated data structures. Instead, the OVERLAY keyword should be used to make such overlaps more readily identifiable.
D Message DS QUALIFIED
D Id 7
* Noncompliant
D Type 1 3
Free-format syntax is preferred because it is clearer, easier to write, and easier for newcomers to the language than fixed-format syntax.
C IF A > 10
C EVAL B = 1
C ENDIF
There’s no need to repeat the program name in the free-format definition of an external program.
DCL-PR PGM001 EXTPGM('PGM001');
END-PR;
Because library names could change from environment to environment, they should never be hard-coded in a program. Instead, you should use a variable to specify the library name and set that variable with data external to the program.
CLRPFM FILE(XPJLIB/FILE01)
The SELECT statement should be used only to clearly define some new branches in the control flow. As soon as a WHEN clause contains too many statements this highly decreases the readability of the overall control flow statement. In such case, the content of WHEN clause should be extracted in a dedicated subroutine.
C SELECT
C WHEN X=1
C MOVE A B
C MOVE C D
C MOVE E F
C MOVE J K
C MOVE L M
C WHEN X=2
C ....
C ENDSL
/free
select;
when X=1;
B = A;
D = C;
F = E;
K = J;
M = L;
when X=2;
...
endsl;
/end-free
When using a PRINTER defined file, an INFDS keyword should be used to read the file’s line count from *PSSR positions 367-368.
FCSO602R1 O E PRINTER USROPN
or ...
FCSO602R1 O E PRINTER USROPN
F INFDS(WSFD01)
F INFSR(*PSSR)
D WSFD01 DS
D W1ST01 *STATUS
D W1SPNO 123 124B 0
When accessing a non-input-only file with a CHAIN/READx operation, the record is locked and becomes inaccessible to the system, and unreadable other programs. This is fine if you are about to update the record immediately, but if you have data processing to do following the read, it can leave the record unavailable for too long. Therefore, non-input files using the CHAIN, READ, READE, READP, and READPE operation codes should always be accessed with the no lock option, (N).
FDDDLJNL7 UF E K DISK INFSR(*PSSR)
...
C READ DDDLJNL7
While the OCCURS keyword still works, it is slower than its more modern replacement: DIM, which should be used instead.
D cussls ds Occurs(200)
D CustNo 7p 0
D MonthlySls 11P 2
Initializing an alphanumeric field with the literal character ’ ’, ‘1’, or ‘0’ has the same effect as using the equivalent figurative constant *BLANK, *ON, or *OFF, but the constant version removes ambiguity and makes the code more readable.
C MOVE ' ' FLD1
The use of options( *nopass ) on a parameter makes it optional, so it should always be checked before use.
DCL-PR VIOLATION1 EXTPGM;
LASTNAME Char(10);
FIRSTNAME Char(10) OPTIONS(*NOPASS:*OMIT);
END-PR;
DCL-PI VIOLATION1;
LASTNAME Char(10);
FIRSTNAME Char(10) OPTIONS(*NOPASS:*OMIT);
END-PI;
dsply FIRSTNAME;
dsply LASTNAME; // Noncompliant; LASTNAME is optional
*inlr = *on;
The use of positional notation can quickly become confusing, especially in a large or complex data structure. To keep the code clean and understandable, use length notation instead.
D PERSON DS QUALIFIED
* Noncompliant
D FName 1 30
D LName 31 60
Prototypes should be used for program calls because their use allows compile-time checking of the number of parameters and the type and size of those parameters. Without the use of a prototype, that checking happens only at run-time.
A secondary benefit of the use of prototypes and CALLP, is that prototypes allow the caller to gain a clearer understanding of the data expected for each parameter because parameter names are purely descriptive.
----
=== Compliant solution
[source,rpg]
Shared coding conventions allow teams to collaborate effectively. To improve readability, blank lines in a program should not have end-of-line comments.
I/COPY VALSRC,VALTEST FIX001
FIX001
** Data structure for subroutine VALDS FIX001
I DS FIX001
I 5 60MMA FIX001
I 7 80DDA FIX001
* FIX001
I DS FIX001
I 5 60MMB FIX001
I 7 80DDB FIX001
The /EJECT compiler directive adds a page break when the code is printed, and should be used at the end of each F, D, and C specification section to make the structure of the code clearer and to enhance overall readability.
F* Conversion Job Activity File (CV)
F*
FCVJBAVP IF E K DISK
F* Conversion Job Dependency File (CZ)
F*
FCVJBDPP IF E K DISK
F*
DP0RTCD S 1
DP0JBNM S 10A
DP0ERMG S 100A
DK0JBNM S LIKE(CVJBNM)
DK1JBNM S LIKE(CVJBNM)
DJST S 10A DIM(10)
DI S 3 0 INZ
C
C EXSR SR999
C
C EXSR SR100
SELECT and OMIT allow you to choose records from a logical file based on the values of specific fields. SQL views allow you to make the same distinctions, but their use is more efficient, and easier to read and understand. Therefore views are preferred over SELECT and OMIT statements.
Index: SKCOU03
*************** Beginning of data *************************************
A R SKCOU TEXT('Coupon Rate Details')
A PFILE(SKCOUP)
A K COUASS
A K COUEFF DESCEND
A S COUSTS COMP(EQ 'L')
A S COUSTS COMP(EQ 'X')
****************** End of data ***************************************
Specifying the parameters to a procedure with a PLIST makes that procedure unusable from free-format code. Instead, prototypes should be used - both when defining a procedure and when calling it. They have the additional benefit of allowing you to use keywords such as Const to better-specify how parameters are passed to a procedure. Further, the use of a prototype instead of a PLIST is cleaner and more consistent with the code required for subprocedures.
* Noncompliant; PLIST specified
C *ENTRY PLIST
C PARM ZipCode 5 0
C PARM City 20
...
* Noncompliant; PLIST used in call
C CALL 'OTHERPROG'
C PARM ZipCode
C PARM City
The use of a result data structure with file I/O improves performance because it moves the data in one large block from file to data structure (or vice versa) rather than field by field.
Additionally using a data structure can limit the problems caused by having bad data in a file. Without a data structure, the entire READ operation will fail at the first bad value. With one, the error comes only when the bad field is used.
F MyFile IF E Disk
* Noncompliant
C READ Record1
Indicators should not be used on “IF” statements because they foster poor logic flow and result in code that is difficult to read and maintain.
C 99FLD1 IFEQ FLD2
A results indicator should be used on a CHAIN statement because it leads to cleaner, more readable, and more correct code. Using an indicator results in the conditions record not found, and end of file being automatically handled correctly, without the need for further error handling.
C K0GMCP CHAIN SSGMCPR
Non-static variables initialized with INZ() are only reliably initialized on the first run through the code in a “terminate and stay resident” (TSR) program. Subsequent calls to the program within the same job do not re-initialize the variable with the value from INZ() unless the last record indicator is set at the end of the program.
Without setting the last record indicator, the assumptions the code makes about the variable’s initial value will be wrong every time but one, potentially leading to bad program behavior.
D TESTINZ PR extpgm('TESTINZ')
D Parm1 15A
D Parm2 15A CONST
DMUTVAR S 15A INZ('ABC') // Noncompliant; *inlr not set
D TESTINZ PI
D Parm1 15A
D Parm2 15A CONST
/Free
IF %PARMS > 1 and Parm2 <> '';
MUTVAR=Parm2;
ENDIF;
DSPLY(E) ('MUTVAR:' + MUTVAR);
Parm1=MUTVAR;
return;
/End-free
QUALIFIED data structures result in cleaner code because you can’t reference the fields without using the qualifying name. They also allow you to have multiple sub-fields with the same name, meaning subfield names don’t have to be convoluted for uniqueness, and can be expressive instead.
* Noncompliant
D Employee DS
D EmpId 7P 0
D EFName 30A
D ELName 30A
D EPhone 11P 0
* Noncompliant
D Contractor DS
D CntId 7P 0
D CFName 30A
D CLName 30A
D CPhone 11P 0
/free
EmpId = '000220';
/end-free
While a file I/O call will compile without a file name, such code is almost always an error. The omission of the file name, can cause unexpected results in multi-file programs.
* Noncompliant
C READ DS1 90
Use of the LIKE statement for fields with the same specification that are being used together in the same statements makes the relevance of individual fields more understandable, and gives easy reference information, and makes relationships between those fields more obvious.
D NAME S 20A
D PNAM S 20A
The CONST keyword on a subprocedure’s parameter indicates that the parameter value will not be changed by the subprocedure. This is not just a nice way to communicate with the programmers who will call the procedure. It also offers performance benefits, because it allows the compiler to produce more optimized code. Further, using CONST means that a field of a similar data type will automatically be converted to the correct type and size for the parameter.
D X S 15A INZ('ABC')
P SubProc1 B
D SubProc1 PI
D Parm1 15A // Noncompliant; read-only. Should be CONST
D Parm2 15A
/Free
X = Parm1;
Parm2 = X;
return;
/End-free
P SubProc1 E
Development tools and frameworks usually have options to make debugging easier for developers. Although these features are useful during development, they should never be enabled for applications deployed in production. Debug instructions or error messages can leak detailed information about the system, like the application’s path or file names.
H*-------------------------------------------------------------------------
H DATEDIT(*YMD) DEBUG(*YES)
H**************************************************************************
C SR990 BegSR
C 'CVTERR' DUMP DUMP for error
C Move *on *INLR
If a private field is declared but not used locally, its limited visibility makes it dead code.
This is either a sign that some logic is missing or that the code should be cleaned.
Cleaning out dead code decreases the size of the maintained codebase, making it easier to understand and preventing bugs from being introduced.
D I S 5P 0 INZ
D* Noncompliant; X is not used in C-Specs
D X S 5P 0 INZ
C EVAL I = I + 1
Using the same value on both sides of a binary operator is a code defect. In the case of logical operators, it is either a copy/paste error and, therefore, a bug, or it is simply duplicated code and should be simplified. In the case of bitwise operators and most binary mathematical operators, having the same value on both sides of an operator yields predictable results and should be simplified as well.
C IF X = X
There is no reason to re-assign a variable to itself. Either this statement is redundant and should be removed, or the re-assignment is a mistake and some other value or variable was intended for the assignment instead.
/free
name = name;
/end-free
Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.
C MOVEL 'CLEAR' W1CLR Noncompliant
C MOVEL '*DECOD' W1DCDE
C MOVEL '*ERROR' W1ERR
C MOVEL '*EXIT ' W1EXIT
C MOVEL 'CLEAR' W1FIRT
C MOVEL 'CLEAR' W1HELP
SELECT * should be avoided because it releases control of the returned columns and could therefore lead to errors and potentially to performance issues.
SELECT *
FROM persons
INTO newyorkers
WHERE city = 'NEW YORK'
Having two WHEN in the same SELECT statement or branches in the same IF structure with the same implementation is at best duplicate code, and at worst a coding error.
C IF X = 1
C EXSR SR01
C EXSR SR01
C ELSEIF X = 2
C EXSR SR02
C ELSEIF X = 3
C EXSR SR01 Noncompliant; duplicates first condition
C EXSR SR01
C ENDIF