G-7130: Always use parameters or pull in definitions rather than referencing external variables in a local program unit.

Major

Maintainability, Reliability, Testability

Reason

Local procedures and functions offer an excellent way to avoid code redundancy and make your code more readable (and thus more maintainable). Your local program refers, however, an external data structure, i.e., a variable that is declared outside of the local program. Thus, it is acting as a global variable inside the program.

This external dependency is hidden, and may cause problems in the future. You should instead add a parameter to the parameter list of this program and pass the value through the list. This technique makes your program more reusable and avoids scoping problems, i.e. the program unit is less tied to particular variables in the program. In addition, unit encapsulation makes maintenance a lot easier and cheaper.

Example (bad)

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
create or replace package body employee_api is
   procedure calc_salary (in_employee_id in employee.employee_id%type) is
      r_employee employee%rowtype;

      function commission return number is
         l_commission employee.salary%type := 0;
      begin
         if r_employee.commission_pct is not null
         then
            l_commission := r_employee.salary * r_employee.commission_pct;
         end if;

         return l_commission;
      end commission;
   begin
      select *
        into r_employee
        from employee
       where employee_id = in_employee_id;

      sys.dbms_output.put_line(r_employee.salary + commission());
   exception
      when no_data_found then
         null;
      when too_many_rows then
         null;
   end calc_salary;
end employee_api;
/

Example (good)

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
create or replace package body employee_api is
   procedure calc_salary (in_employee_id in employee.employee_id%type) is
      r_employee employee%rowtype;

      function commission (in_salary   in employee.salary%type
                          ,in_comm_pct in employee.commission_pct%type)
         return number is
         l_commission employee.salary%type := 0;
      begin
         if in_comm_pct is not null then
            l_commission := in_salary * in_comm_pct;
         end if;

         return l_commission;
      end commission;
   begin
      select *
        into r_employee
        from employee
       where employee_id = in_employee_id;

      sys.dbms_output.put_line(
         r_employee.salary + commission(in_salary   => r_employee.salary
                                  ,in_comm_pct => r_employee.commission_pct)
      );
   exception
      when no_data_found then
         null;
      when too_many_rows then
         null;
   end calc_salary;
end employee_api;
/