trivadis / plsql-and-sql-coding-guidelines Goto Github PK

Section "Try to use ANSI-join syntax”.

The text suggests that the bad example is not ANSI. As far as I know it is ANSI, ANSI-89 to be precise. The good example is ANSI-92.

__
I refer to the pdf, commit cf4d9b0; however, I guess it's true for the doc as well.

Language Usage / Control Structures / Flow Control
Avoid using RETURN from within a loop

Language Usage / Controll Structures / CASE ...
Avoid using equal conditions in different branches of the same IF or CASE statement

Language Usage / DML & SQL / General
Always specify explicitly the direction in an ORDER BY clause

I was looking for version 3.3 in PDF to download it. But only can download 3.4-SNAPSHOT.
¿Is there any chance to have the PDF in the Releases section?
Great work!

Language Usage / Stored Objects / Procedures
Always assign values to OUT parameters

Language Usage / General
never assign a column or variable to itself

Language Usage / General
Never repeat an unary operator multiple times
(happens often as a typo or caused by lack of consentration)

bad:
l_value := ++5
IF l_boolean IS NOT NOT TRUE

good:
l_value := +5
IF l_boolean IS NOT TRUE

** Language Usage / General** or Language Usage / Variables & Types / General
Always use a format model in the functions TO_DATE and TO_TIMESTAMP

Language Usage / Exception Handling
when using DBMS_UTILITY.FORMAT_ERROR_STACK to display an error always follow up with DBMS_UTILITY.FORMAT_ERROR_BACKTRACE

The rule 8420 is defined in the chapter "4.8.5 Use dbms_application_info package to follow progress of a process". Based on the numbering scheme the guidelines should be renamed to 8510.

Language Usage / DML & SQL / General
Never use ROWNUM at the same query level as ORDER BY

Language Usage / Control Structures / Flow Control
Never reuse a label within its own scope

from SonarQube

Labels should not be reused in inner scopes

Using the same name for multiple purposes reduces the understandability of the code and might eventually lead to bugs.

This rule verifies that no label is reused in an inner scope.

Noncompliant Code Example

<<foo>>
DECLARE
  a CONSTANT PLS_INTEGER := 0;
BEGIN
  <<foo>>
  DECLARE
    b CONSTANT PLS_INTEGER := 42;
  BEGIN
    DBMS_OUTPUT.PUT_LINE('x = ' || foo.b);
  END;
END;

Compliant Solution

<<foo>>
DECLARE
  a CONSTANT PLS_INTEGER := 0;
BEGIN
  <<bar>>
  DECLARE
    b CONSTANT PLS_INTEGER := 42;
  BEGIN
    DBMS_OUTPUT.PUT_LINE('x = ' || bar.b);
  END;
END;

Language Usage / DML & SQL /General
Always include values for NOT NULL Columns in an INSERT statement

Language Usage / General
Avoid labels that are not used after declaration

from SonarQube

Unused labels should be removed

If a label is declared but not used in the program, it can be considered as dead code and should therefore be removed.

This will improve maintainability as developers will not wonder what this label is used for.

Noncompliant Code Example

<<foo>>
BEGIN
  DBMS_OUTPUT.PUT_LINE('Hello, world!');
END;

Compliant Solution

BEGIN
  DBMS_OUTPUT.PUT_LINE('Hello, world!');
END;

Language Usage / Variables & Types / General
Avoid assigning values to local variables that are not used by a subsequent statement

from SonarQube

Dead stores should be removed

A dead store happens when a local variable is assigned a value that is not read by any subsequent instruction. Calculating or retrieving a value only to then overwrite it or throw it away, could indicate a serious error in the code. Even if it's not an error, it is at best a waste of resources. Therefore all calculated values should be used.

Noncompliant Code Example

declare
  my_user VARCHAR2(30);
  my_date VARCHAR2(30);
begin
  my_user := user();
  my_date := sysdate();
  dbms_output.put_line('User:' || my_user || ', date: ' || my_user);
end;

Compliant Solution

declare
  my_user VARCHAR2(30);
  my_date VARCHAR2(30);
begin
  my_user := user();
  my_date := sysdate();
  dbms_output.put_line('User:' || my_user || ', date: ' || my_date);
end;

Language Usage / Stores Objects / Packages
Avoid using RETURN in the inizialisation block of a package body

Language Usage / Control Structures / CURSOR or Language Usage / Variables & Types / General
Avoid usage of weak REF CURSOR types

from SonarCube

Weak "REF CURSOR" types should not be used

Weak REF CURSOR types are harder to work with than ones with a return type. Indeed, the compiler's type-checker is unable to make some verifications, which are then delayed till runtime.

When the use of weak REF CURSOR is required, it is best to use the SYS_REFCURSOR built-in type instead of defining a new one.

This rule's sysRefCursorAllowed parameter can be used to control whether or not the usage of SYS_REFCURSOR is allowed.

Noncompliant Code Example

DECLARE
  TYPE dualCursorType IS REF CURSOR;
  dualCursor dualCursorType;

  otherCursor SYS_REFCURSOR;                              -- Compliant or non-compliant, depending on the "sysRefCursorAllowed" parameter
BEGIN
  otherCursor := dualCursor;                              -- Works
END;

Compliant Solution

DECLARE
  TYPE dualCursorType IS REF CURSOR RETURN DUAL%ROWTYPE;
  dualCursor dualCursorType;

  TYPE otherCursorType IS REF CURSOR RETURN a%ROWTYPE;
  otherCursor otherCursorType;
BEGIN
  otherCursor := dualCursor;                              -- raises PLS-00382: expression is of wrong type, which makes debugging easier
END;

Language Usage / Controll Structures / Flow Control
Never use conditions that evaluate unconditionally to TRUE or FALSE

Language Usage / General
Avoid using COMMIT within a loop

In the title of the page for rule G-7320 (https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/7-stored-objects/3-procedures/g-7320/) the "G-"-prefix is missing for the rule-number.

Language Usage / Exception Handling
Avoid "EXCEPTION WHEN ... THEN" clauses that only execute a RAISE
(if this rule gets implementet, the good-example of rule G-5060 needs to be changed)

** Language Usage / Stored Objects / General**
always use CREATE OR REPLACE instead of only CREATE

Language Usage / Exception Hadling
Always use a WHEN OTHERS clause at the end of a EXCEPTION block

On mobile devices long lines in code are wrapped. Here is a screenshot:

Language Usage / Control Structures / CASE ...
Avoid inverting boolean checks

from SonarQube

Boolean checks should not be inverted

It is needlessly complex to invert the result of a boolean comparison. The opposite comparison should be made instead.

Noncompliant Code Example

IF NOT x <> y THEN
  -- ...
END IF;

Compliant Solution

IF x = y THEN
  -- ...
END IF;

Language Usage / DML & SQL / General
Always specify columns in a ORDER BY clause explicitly

Add PL/SQL Cop for SonarQube in the tool support chapter

Language Usage / DML & SQL / General
Avoid lists for IN clauses with more than 1000 items
(not applicable for IN clauses with a subselect)

from SonarQube

Large item lists should not be used with "IN" clauses

Oracle supports at most 1000 items in a SQL query's IN clause. When more items are given, the exception ORA-01795 maximum number of expressions in a list is 1000 is raised. Thus, IN clauses are not as scalable as joins.

Noncompliant Code Example

BEGIN
  SELECT *
  INTO result
  FROM my_table
  WHERE col1 IN (1, 2, 3, ..., 1001);
END;

Compliant Solution

BEGIN
  SELECT my_table.*
  INTO result
  FROM my_table
  JOIN new_in_table
  WHERE my_table.col1 = new_in_table.value;
END;

Language Usage / Control Structures / Flow Control
Avoid using jump statements that direct the flow to the default direction

from SonarQube

Jump statements should not be redundant

Jump statements, such as RETURN and CONTINUE let you change the default flow of program execution, but jump statements that direct the control flow to the original direction are just a waste of keystrokes.

Noncompliant Code Example

CREATE PROCEDURE print_numbers AS
BEGIN
  FOR i in 1..4 LOOP
    DBMS_OUTPUT.PUT_LINE(i);
    CONTINUE;
  END LOOP;
  RETURN;
END;

Compliant Solution

CREATE PROCEDURE print_numbers AS
BEGIN
  FOR i in 1..4 LOOP
    DBMS_OUTPUT.PUT_LINE(i);
  END LOOP;
END;

The rule says:

... should be followed for any other stored object having more than one parameter.

But the example is based on a single parameter.

Add in the tool support chapter plscope-utils, plscope-utils. These tools may be used beside PL/SQL Cop for SQL Developer to check naming conventions for PL/SQL.

The current naming rules for "Database Object Naming Conventions" are missing prefix for "Check constraint". I prefer to name all constraints. Check constraints I name with prefix "_ck", and the special case "NOT NULL" I prefix "_nn".

Language Usage / DMS & SQL / General
Always use wildcards in a LIKE clause

Language Usage / Variables & Types / Character Data Types
Never assign a string to a variable or column that is to small to hold it

Hi, I'm using Trivadis plugin for SQL Developer.
The correct code for guideline G-3120 is:

SELECT emp.last_name
,emp.first_name
,dept.department_name
FROM
employees
emp
JOIN departments dept ON (emp.department_id = dept.department_id)
WHERE EXTRACT(MONTH FROM emp.hire_date) = EXTRACT(MONTH FROM SYSDATE);

but if I add the EXTRACT function, G-3120 appears, but I think this is not a rule violation.

SELECT emp.last_name
,emp.first_name
,dept.department_name
, EXTRACT(MONTH FROM SYSDATE) as datesample
FROM
employees
emp
JOIN departments dept ON (emp.department_id = dept.department_id)
WHERE EXTRACT(MONTH FROM emp.hire_date) = EXTRACT(MONTH FROM SYSDATE);

¿What can I do to fix the code?

Language Usage / Exception Handling
Always use the SAVE EXCEPTIONS clause in a FORALL statement

Language Usage / DML & SQL / General
Avoid subqueries in EXIST clauses, use joins instead

from [SonarQube](https://rules.sonarsource.com/plsql/type/Code Smell/RSPEC-1138)

SQL EXISTS subqueries should not be used

SQL queries that use EXISTS subqueries are inefficient because the subquery is re-run for every row in the outer query's table. There are more efficient ways to write most queries, ways that do not use the EXISTS condition.

Noncompliant Code Example

SELECT e.name
FROM employee e
WHERE EXISTS (SELECT * FROM department d WHERE e.department_id = d.id AND d.name = 'Marketing');

Compliant Solution

SELECT e.name
FROM employee e INNER JOIN department d
  ON e.department_id = d.id AND d.name = 'Marketing';

Language Usage / Stored Objects / Triggers
Avoid compound triggers that have only one trigger implemented

from SonarQube

Compound triggers should define at least two triggers

Compound triggers were introduced to ease the implementation of multiple triggers which need to work in cooperation.

Typically, a FOR EACH ROW trigger accumulates facts, and an AFTER STATEMENT trigger performs the actual changes.

The compound trigger can hold a state common to all the triggers it defines, thereby removing the need to use package variables. This approach is sometimes the only possible one, as when avoiding a mutating table ORA-04091 error, or it can be used to get better performance.

However, there is no point in defining a compound trigger which contains only a single trigger, since there is no state to be shared. In such cases, a simple trigger should be used instead.

Noncompliant Code Example

CREATE OR REPLACE TRIGGER my_trigger
FOR INSERT ON my_table
COMPOUND TRIGGER

AFTER EACH ROW IS
BEGIN
  DBMS_OUTPUT.PUT_LINE('New row inserted!');
END AFTER EACH ROW;

END;

Compliant Solution

CREATE OR REPLACE TRIGGER my_trigger
  AFTER INSERT
  ON my_table
  FOR EACH ROW
BEGIN
  DBMS_OUTPUT.PUT_LINE('New row inserted!');
END;

Language Usage / DML & SQL / General
Never use SELECT * in a (sub-)query that selects directly from tables or views

from SonarQube

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.

Noncompliant Code Example

DECLARE
  myvar CHAR;
BEGIN
  SELECT * INTO myvar FROM DUAL;
 END;

Compliant Solution

DECLARE
  myvar CHAR;
BEGIN
  SELECT dummy INTO myvar FROM DUAL;
END;

Exceptions

Wrapper queries using ROWNUM are ignored.

SELECT *
FROM ( SELECT fname, lname, deptId
    FROM employee
    ORDER BY salary
  )
WHERE rownum <= 10

Hi, i was wondering if you're going to release PL/SQL Cop for SQL Developer free extension for Trivadis PL/SQL & SQL Coding Guidelines Version 3.3 (latest release).

Language Usage / Stored Objects / Triggers
Never use multiple UPDATE OF statements in the event clause of a trigger

bad:
CREATE OR REPLACE TRIGGER xyz BEFORE UPDATE OF column1 OR UPDATE OF column2 ...

good:
CREATE OR REPLACE TRIGGER xyz BEFORE UPDATE OF column1, column2 ...

Language Usage / Control Structures / CASE ...
Avoid comparing boolean variables or colums with TRUE or FALSE

from SonarQube

Boolean literals should not be redundant

Redundant Boolean literals should be removed from expressions to improve readability.

Noncompliant Code Example

DECLARE
  foo BOOLEAN := TRUE;
BEGIN
  IF foo = FALSE THEN
    DBMS_OUTPUT.PUT_LINE('foo = false!');
  ELSIF foo = TRUE THEN
    DBMS_OUTPUT.PUT_LINE('foo = true!');
  END IF;
END;

Compliant Solution

DECLARE
  foo BOOLEAN := TRUE;
BEGIN
  IF NOT foo THEN
    DBMS_OUTPUT.PUT_LINE('foo = false!');
  ELSIF foo THEN
    DBMS_OUTPUT.PUT_LINE('foo = true!');
  END IF;
END;

Some rules such as G-1010 or G-4340 do not have a reason. For the SonarQube plugin the reason has been defined. Take the reason from there.

The PDF document is fully generated, but managed as a source file. We should treat the PDF the same way as the HTML site. This means provide the generated stuff on the gh-pages branch only.

Making the PDF document accessible from the HTML site would simplify the version specific access.

Language Usage / General
Never use the same expression on both sides of a relational comparison operator or a logical operator

On the page for rule G-8510 (https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/8-patterns/4-ensure-single-execution-at-a-time-of-a-program-unit/g-8410/)
the example-code is incomplete:

• in the code-window "Example (bad)" is the code for the package used in the code-window "Example (good)"

• therefore a bad code-example is missing

` cur2_aid address.aid%type;
cur2_country address.country%type;
cur2_city address.city%type;
cur2_state address.state%type;
cur2_streetAddress address.street_name%type;
cur2_locality address.locality%type;
cur2_houseNo address.house_no%type;

begin
open cur1;
loop
fetch cur1 into cur1_aid,cur1_country,cur1_state,cur1_city,cur1_streetAddress,cur1_locality,cur1_houseNo;
exit when cur1%NOTFOUND;
open cur2;
loop
fetch cur2 into cur2_aid,cur2_country,cur2_state,cur2_city,cur2_streetAddress,cur2_locality,cur2_houseNo;
exit when cur2%NOTFOUND;
if(cur1_country=cur2_country) and (cur1_state=cur2_state) and (cur1_city=cur2_city) and (cur1_streetAddress=cur2_streetAddress) and (cur1_locality=cur2_locality) and (cur1_houseNo=cur2_houseNo) then
if (cur1_aid!=cur2_aid) then
update employee_add set aid=cur1_aid where aid=cur2_aid;
delete address where aid=cur2_aid;
end if;
end if;
end loop;
close cur2;
end loop;
close cur1;
DELETE FROM employee_add a
WHERE ROWID > (SELECT MIN(ROWID) FROM employee_add b
WHERE b.eid=a.eid and b.aid=a.aid
);
end;
/`

G-7110 is reported for indexes of nested tables. I.e. I have this code in procedure:

      --define columns and column types
      <<col_types>>
      for i in 1..p_columns.count
      loop
      
         --note: in case it would be required to support other data type, sys.dbms_sql does not describe wich col_type 
         --      value is for which data type. Numbers for each data type can be found in sys.dbms_types package
         if (p_columns(i).col_type = 2) then
            sys.dbms_sql.define_column(c => p_cursor_id,
                                       position => i,
                                       column => l_number);
                                       
         elsif (p_columns(i).col_type = 12) then
            sys.dbms_sql.define_column(c => p_cursor_id,
                                       position => i,
                                       column => l_date);
         
         else
            sys.dbms_sql.define_column(c => p_cursor_id,
                                       position => i,
                                       column => l_varchar2,
                                       column_size => 4000);

         end if;   

      end loop col_types;

where p_columns is input parameter of procedure. G-7110 is reported on the first if statement on "i". I think that nested table index should not be reported.

Currently the Trivadis PL/SQL & SQL Coding Guidelines are maintained in a single Microsoft Word document. This is not optimal. For example:

• Changes cannot be easily tracked within a version control system (at least not without additional tooling).
• Multi-user support is limited, it is difficult to ensure that no changes are lost when merging.
• Keeping track of changes after adapting the guidelines for an application/project is difficult (identifying relevant changes and merging them).
• There is no HTML target format. This could simplify linking to chapters/rules from other documents.

The idea is to split the current document into markup files, which may be processed by Jekyll to produce static HTML files and also a PDF file similar to the current one.

This is just a "reformatting" task. It is not the idea to change the content in the first step. As long as this issue is not implemented fully in a satisfactory way, the Word document ist still the master and might be evolved further.

As an inspiration look into the MvvmCross GitHub repository. They generate the documentation with Jekyll including also some JavaScript for search capabilities. See the result.