1357 246-
Anti-Patterns
Rpg
Imported data items should not be updated
Imported data items should not be updated
IMPORTed values should be treated as read-only. Doing otherwise could result in unpleasant surprises down the line.
Debugging statements DEBUG(*YES) and DUMP should not be used
Debugging statements DEBUG(*YES) and DUMP should not be used
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.
Procedures should have return values
Procedures should have return values
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.
The correct ENDxx statement should be used
The correct ENDxx statement should be used
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.
Expressions should not be too complex
Expressions should not be too complex
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.
MONITOR statements with identical ON-ERROR blocks should be merged
MONITOR statements with identical ON-ERROR blocks should be merged
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.
Labels should be on lines by themselves
Labels should be on lines by themselves
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.
Record formats should be cleared before each use
Record formats should be cleared before each use
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.
ON-ERROR clauses should not be empty
ON-ERROR clauses should not be empty
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.
UNDERLINE should not be used
UNDERLINE should not be used
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.
The parameters of a CALL or CALLB statement should be defined as a PLIST
The parameters of a CALL or CALLB statement should be defined as a PLIST
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`.
Track lack of copyright and license headers
Track lack of copyright and license headers
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.
/COPY should be avoided
/COPY should be avoided
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.
The *srcstmt header option should be used
The *srcstmt header option should be used
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.
All opened USROPN files should be explicitly closed
All opened USROPN files should be explicitly closed
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.
Spacing types should not be mixed
Spacing types should not be mixed
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.
Related IF/ELSEIF statements and WHEN clauses in a SELECT should not have the same condition
Related IF/ELSEIF statements and WHEN clauses in a SELECT should not have the same condition
Named constants should be used for indicators
Named constants should be used for indicators
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.
Compile-time arrays should not be used
Compile-time arrays should not be used
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.
E should be found in F-spec lines
E should be found in F-spec lines
Externally described files standardize file access, and result in simpler, easier to read code.
Procedures that check parameter count should mark parameters optional
Procedures that check parameter count should mark parameters optional
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.
Optional operation codes should be omitted
Optional operation codes should be omitted
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.
IF ... ELSEIF constructs should end with ELSE clauses
IF ... ELSEIF constructs should end with ELSE clauses
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.
The first parameter of a CHAIN/READx statement should be a KLIST
The first parameter of a CHAIN/READx statement should be a KLIST
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.
Subroutine names should comply with a naming convention
Subroutine names should comply with a naming convention
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:
Control flow statements IF, FOR, DO, ... should not be nested too deeply
Control flow statements IF, FOR, DO, ... should not be nested too deeply
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.
Numeric fields should be defined as odd length packed fields
Numeric fields should be defined as odd length packed fields
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
Subprocedures should be used instead of subroutines
Subprocedures should be used instead of subroutines
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.
/COPY statements should include specification letters
/COPY statements should include specification letters
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.
Subroutines should be documented
Subroutines should be documented
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.
Subprocedures should not reference global variables
Subprocedures should not reference global variables
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.
Error handling should be defined in F specs
Error handling should be defined in F specs
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.
GOTO statements should not be used
GOTO statements should not be used
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.
The data area structure for IN should be defined in D spec lines.
The data area structure for IN should be defined in D spec lines.
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.
OVERLAY should be used for overlapping fields
OVERLAY should be used for overlapping fields
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.
Calculations should use free-form syntax
Calculations should use free-form syntax
Free-format syntax is preferred because it is clearer, easier to write, and easier for newcomers to the language than fixed-format syntax.
External program names should not be listed redundantly
External program names should not be listed redundantly
There’s no need to repeat the program name in the free-format definition of an external program.
Library names should not be hard-coded
Library names should not be hard-coded
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.
SELECT WHEN clauses should not have too many lines
SELECT WHEN clauses should not have too many lines
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.
Line count data should be retrieved from the file information data structure
Line count data should be retrieved from the file information data structure
When using a PRINTER defined file, an INFDS keyword should be used to read the file’s line count from *PSSR positions 367-368.
Non-input files should be accessed with the no lock option
Non-input files should be accessed with the no lock option
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).
Multiple occurrence data structures should not be used
Multiple occurrence data structures should not be used
While the OCCURS keyword still works, it is slower than its more modern replacement: DIM, which should be used instead.
Standard figurative constants *ON, *OFF and *BLANK should be used in place of 1, 0 and
Standard figurative constants *ON, *OFF and *BLANK should be used in place of 1, 0 and
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.
Optional parameters should be checked before use
Optional parameters should be checked before use
The use of options( *nopass ) on a parameter makes it optional, so it should always be checked before use.
Length notation should be used for data structure definitions
Length notation should be used for data structure definitions
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.
Prototypes should be used
Prototypes should be used
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.
Blank lines should not have end-of-line comments
Blank lines should not have end-of-line comments
Shared coding conventions allow teams to collaborate effectively. To improve readability, blank lines in a program should not have end-of-line comments.
/EJECT should be used after F, D and C specification sections
/EJECT should be used after F, D and C specification sections
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.
Select/omit rules should not be used
Select/omit rules should not be used
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.
Prototypes should be used
Prototypes should be used
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.
Result data structures should be used for file I/O
Result data structures should be used for file I/O
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.
IF statements should not be conditioned on Indicators
IF statements should not be conditioned on Indicators
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.
Indicators should be used on CHAIN statements
Indicators should be used on CHAIN statements
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.
Inz() should not be used on non-static variables
Inz() should not be used on non-static variables
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.
QUALIFIED data structures should be used
QUALIFIED data structures should be used
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.
File operations should specify a file name
File operations should specify a file name
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.
LIKE keyword should be used to define work fields
LIKE keyword should be used to define work fields
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.
CONST should be used for parameters that are not modified
CONST should be used for parameters that are not modified
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.
Delivering code in production with debug features activated is security-sensitive
Delivering code in production with debug features activated is security-sensitive
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.
Unused variables should be removed
Unused variables should be removed
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.
Identical expressions should not be used on both sides of a binary operator
Identical expressions should not be used on both sides of a binary operator
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.
Variables should not be self-assigned
Variables should not be self-assigned
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.
String literals should not be duplicated
String literals should not be duplicated
Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.
Columns to be read with a SELECT statement should be clearly defined
Columns to be read with a SELECT statement should be clearly defined
SELECT * should be avoided because it releases control of the returned columns and could therefore lead to errors and potentially to performance issues.
Two branches in a conditional structure should not have exactly the same implementation
Two branches in a conditional structure should not have exactly the same implementation
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.