Merge pull request #199 from Trivadis/bugfix/violations-in-code-blocks

Fix/explain violations of unrelated guidelines

Author: PhilippSalvisberg

.github/workflows/build.yml

@@ -18,13 +18,13 @@ jobs:
           fetch-depth: 0  # Shallow clones should be disabled for a better relevancy of analysis
       - name: db* CODECOP Issues
         run: |
-          export COP=4.3.0
-          export VALIDATOR=4.3.0
+          export COP=4.3.1
+          export VALIDATOR=4.3.1
           wget https://github.com/Trivadis/plsql-cop-cli/releases/download/v$COP/tvdcc-$COP.zip
           unzip tvdcc-$COP.zip -d .
           wget -P tvdcc-$COP/plugin/ https://github.com/Trivadis/plsql-cop-validators/releases/download/v$VALIDATOR/sonar-plsql-cop-custom-validators-plugin-$VALIDATOR.jar
           echo $TVDCC_LIC | base64 -d > tvdcc-$COP/tvdcc.lic
-          tvdcc-$COP/tvdcc.sh path=docs skip=none html=false excel=false validator=com.trivadis.tvdcc.validators.TrivadisGuidelines3Plus
+          tvdcc-$COP/tvdcc.sh path=docs skip=0 html=false excel=false validator=com.trivadis.tvdcc.validators.TrivadisGuidelines3Plus
         env:
           TVDCC_LIC: ${{ secrets.TVDCC_LIC }}
       - name: SonarCloud Scan

.gitignore

@@ -13,6 +13,9 @@
 /pdf
 /docs/9-appendix/PLSQL-and-SQL-Coding-Guidelines.pdf
 
+# Files generated by tvdcc
+/tvdcc_report.json
+
 # Distribution files
 /dist
 /mkdocs_material.egg-info

docs/3-coding-style/coding-style.md

@@ -19,33 +19,37 @@ Rule | Description
 ### Example
 
 ``` sql
-create or replace procedure set_salary(in_employee_id in employees.employee_id%type) is
-   cursor c_employees(p_employee_id in employees.employee_id%type) is
-      select last_name
-            ,first_name
-            ,salary
-        from employees
-       where employee_id = p_employee_id
-       order by last_name
-            ,first_name;
-
-   r_employee   c_employees%rowtype;
-   l_new_salary employees.salary%type;
-begin
-   open c_employees(p_employee_id => in_employee_id);
-   fetch c_employees into r_employee;
-   close c_employees;
-
-   new_salary(in_employee_id => in_employee_id
-             ,out_salary     => l_new_salary);
-
-   -- Check whether salary has changed
-   if r_employee.salary <> l_new_salary then
-      update employees
-         set salary = l_new_salary
-       where employee_id = in_employee_id;
-   end if;
-end set_salary;
+create or replace package body employee_api is
+   procedure set_salary(in_employee_id in integer) is
+      co_employee_id constant employees.employee_id%type := in_employee_id;
+
+      cursor c_employees(p_employee_id in employees.employee_id%type) is
+         select last_name
+               ,first_name
+               ,salary
+           from employees
+          where employee_id = p_employee_id
+          order by last_name
+               ,first_name;
+
+      r_employee     c_employees%rowtype;
+      l_new_salary   employees.salary%type;
+   begin
+      open c_employees(p_employee_id => co_employee_id);
+      fetch c_employees into r_employee;
+      close c_employees;
+
+      new_salary(in_employee_id => in_employee_id
+                ,out_salary     => l_new_salary);
+
+      -- Check whether salary has changed
+      if r_employee.salary <> l_new_salary then
+         update employees
+            set salary = l_new_salary
+          where employee_id = in_employee_id;
+      end if;
+   end set_salary;
+end employee_api;
 ```
 
 ## Code Commenting

docs/4-language-usage/1-general/g-1020.md

@@ -38,7 +38,7 @@ begin
 
    <<basic_loop>>
    loop
-      exit basic_loop;
+      exit basic_loop when true;
    end loop;
 
    <<for_loop>>
@@ -78,7 +78,7 @@ begin
 
    <<basic_loop>>
    loop
-      exit basic_loop;
+      exit basic_loop when true;
    end loop basic_loop;
 
    <<for_loop>>

docs/4-language-usage/1-general/g-1040.md

@@ -13,22 +13,22 @@ Any part of your code, which is no longer used or cannot be reached, should be e
 declare
    co_dept_purchasing constant departments.department_id%type := 30;
 begin
-   if 2 = 3 then
+   if 2 = 3 then -- G-1050 violated, dead code detection works with literals only
       null; -- some dead code here
    end if;
 
    null; -- some enabled code here
 
    <<my_loop>>
    loop
-      exit my_loop;
+      exit my_loop when true;
       null; -- some dead code here
    end loop my_loop;
 
    null; -- some other enabled code here
 
    case
-      when 1 = 1 and 'x' = 'y' then
+      when 1 = 1 and 'x' = 'y' then -- G-1050 violated, dead code detection works with literals only
          null; -- some dead code here
       else
          null; -- some further enabled code here
@@ -40,7 +40,7 @@ begin
         from employees
        where department_id = co_dept_purchasing
           or commission_pct is not null
-         and 5 = 6
+         and 5 = 6 -- G-1050 violated, dead code detection works with literals only
    ) 
    -- "or commission_pct is not null" is dead code 
    loop

docs/4-language-usage/1-general/g-1080.md

@@ -12,24 +12,34 @@ This rule ignores operators `+`, `*` and `||`, and expressions: `1=1`, `1<>1`, `
 ## Example (bad)
 
 ``` sql
-select emp.first_name
-      ,emp.last_name
-      ,emp.salary
-      ,emp.hire_date
-  from employees emp
- where emp.salary > 3000
-    or emp.salary > 3000
- order by emp.last_name,emp.first_name;
+declare
+   co_max_salary constant emp.salary%type := 3000;
+begin
+   select emp.first_name
+         ,emp.last_name
+         ,emp.salary
+         ,emp.hire_date
+     from employees emp
+    where emp.salary > co_max_salary
+       or emp.salary > co_max_salary
+    order by emp.last_name,emp.first_name;
+end;
+/
 ```
 
 ## Example (good)
 
 ``` sql
-select emp.first_name
-      ,emp.last_name
-      ,emp.salary
-      ,emp.hire_date
-  from employees emp
- where emp.salary > 3000
- order by emp.last_name,emp.first_name;
+declare
+   co_max_salary constant emp.salary%type := 3000;
+begin
+   select emp.first_name
+         ,emp.last_name
+         ,emp.salary
+         ,emp.hire_date
+     from employees emp
+    where emp.salary > co_max_salary
+    order by emp.last_name,emp.first_name;
+end;
+/
 ```

docs/4-language-usage/2-variables-and-types/1-general/g-2135.md

@@ -13,15 +13,16 @@ Expending resources calculating and assigning values to a local variable and nev
 create or replace package body my_package is
    procedure my_proc is
       co_employee_id constant employees.employee_id%type := 1042;
+      co_hello       constant type_up.text               := 'Hello, ';
       l_last_name    employees.last_name%type;
-      l_message      varchar2(100 char);
+      l_message      types_up.text;
    begin
       select emp.last_name
         into l_last_name
         from employees emp
        where emp.employee_id = co_employee_id;
 
-      l_message := 'Hello, ' || l_last_name;
+      l_message := co_hello || l_last_name;
    exception
       when no_data_found then
          null; -- handle_no_data_found;
@@ -38,15 +39,16 @@ end my_package;
 create or replace package body my_package is
    procedure my_proc is
       co_employee_id constant employees.employee_id%type := 1042;
+      co_hello       constant type_up.text               := 'Hello, ';
       l_last_name    employees.last_name%type;
-      l_message      varchar2(100 char);
+      l_message      types_up.text;
    begin
       select emp.last_name
         into l_last_name
         from employees emp
        where emp.employee_id = co_employee_id;
 
-      l_message := 'Hello, ' || l_last_name;
+      l_message := co_hello || l_last_name;
 
       message_api.send_message(l_message);
    exception

docs/4-language-usage/2-variables-and-types/1-general/g-2145.md

@@ -11,12 +11,15 @@ There is no reason to assign a variable to itself. It is either a redundant stat
 
 ``` sql
 declare
-   l_function_result pls_integer;
-   l_parallel_degree pls_integer;
+   co_parallel_degree constant types_up.name%type := 'parallel_degree';
+   l_function_result  pls_integer;
+   l_parallel_degree  pls_integer;
 begin
-   l_function_result := maintenance.get_config('parallel_degree');
-   l_parallel_degree := l_parallel_degree;
-   do_something(l_parallel_degree);
+   l_function_result := maintenance.get_config(co_parallel_degree);
+   if l_function_result is not null then
+      l_parallel_degree := l_parallel_degree;
+      do_something(l_parallel_degree);
+   end if;
 end;
 /
 ```
@@ -25,12 +28,15 @@ end;
 
 ``` sql
 declare
-   l_function_result pls_integer;
-   l_parallel_degree pls_integer;
+   co_parallel_degree constant types_up.name%type := 'parallel_degree';
+   l_function_result  pls_integer;
+   l_parallel_degree  pls_integer;
 begin
-   l_function_result := maintenance.get_config('parallel_degree');
-   l_parallel_degree := l_function_result;
-   do_something(l_parallel_degree);
+   l_function_result := maintenance.get_config(co_parallel_degree);
+   if l_function_result is not null then
+      l_parallel_degree := l_function_result;
+      do_something(l_parallel_degree);
+   end if;
 end;
 /
 ```

docs/4-language-usage/2-variables-and-types/1-general/g-2180.md

@@ -11,11 +11,12 @@ Quoted identifiers make your code hard to read and maintain.
 
 ``` sql
 declare
-   "sal+comm"     integer;
-   "my constant"  constant integer := 1;
-   "my exception" exception;
+   "sal+comm"     integer;               -- violates also naming conventions (G-9102)
+   "my constant"  constant integer := 1; -- violates also naming conventions (G-9114)
+   "my exception" exception;             -- violates also naming conventsion (G-9113)
 begin
    "sal+comm" := "my constant";
+   do_something("sal+comm");
 exception
    when "my exception" then
       null;
@@ -32,6 +33,7 @@ declare
    e_my_exception exception;
 begin
    l_sal_comm := co_my_constant;
+   do_something(l_sal_comm);
 exception
    when e_my_exception then
       null;

docs/4-language-usage/2-variables-and-types/1-general/g-2185.md

@@ -12,10 +12,11 @@ You should ensure that the name you have chosen well defines its purpose and usa
 ``` sql
 declare
    i integer;
-   c constant integer := 1;
-   e exception;
+   c constant integer := 1; -- violates also naming conventions (G-9114)
+   e exception;             -- violates also naming conventions (G-9113)
 begin
    i := c;
+   do_something(i);
 exception
    when e then
       null;
@@ -32,6 +33,7 @@ declare
    e_my_exception exception;
 begin
    l_sal_comm := co_my_constant;
+   do_something(l_sal_comm);
 exception
    when e_my_exception then
       null;