Posted by I was doing some code review today of a PL/SQL package, and came across a routine to generate a list of employees for a department. Truth be told, it wasn’t employees and departments but some anonymity is called for here. 
SQL> create or replace
2 function emp_list(p_dept varchar2) return varchar2 is
3 l_emps varchar2(4000) := ':';
4 begin
5 for i in (
6 select empno
7 from emp
8 where deptno = p_dept )
9 loop
10 l_emps := l_emps || i.empno ||':';
11 end loop;
12 return l_emps;
13 end;
14 /
Function created.
SQL>
SQL> select emp_list(30) from dual;
EMP_LIST(30)
----------------------------------------------------------------
:7499:7521:7654:7698:7844:7900:
Savvy readers will straight away know that rather than “reinvent the wheel”, the function could re-coded with a LISTAGG function, and since at that point, it is just a standard SQL expression, then potentially the PL/SQL function is not required at all. (We’d only know that by knowing the full context of the function’s usage).
SQL> create or replace
2 function emp_list(p_dept varchar2) return varchar2 is
3 l_emps varchar2(4000);
4 begin
5 select ':'||listagg(empno,':') within group ( order by empno )||':'
6 into l_emps
7 from emp
8 where deptno = p_dept;
9 return l_emps;
10 end;
11 /
Function created.
SQL>
SQL> select emp_list(30) from dual;
EMP_LIST(30)
---------------------------------------------------------------------------
:7499:7521:7654:7698:7844:7900:
However, we can’t be too critical here. We don’t know the background and history of the code – perhaps it was written well before LISTAGG was available? So whilst its an obvious improvement, I’m happy to let that one slide.
But it is easy to be led astray by the allure of “reuse”, and the very next procedure in the package implements the business requirement of: “Does a nominated employee belong to a nominated department?”
Let’s look at the implementation.
SQL> create or replace
2 function emp_in_dept(p_emp int, p_dept int) return boolean is
3 l_emps varchar2(4000);
4 begin
5 l_emps := emp_list(p_dept); <<<==== our previous function
6 return instr(l_emps,':'||p_emp||':') > 0;
7 end;
8 /
Function created.
SQL>
SQL> set serverout on
SQL> begin
2 --
3 -- should be true
4 --
5 if emp_in_dept(7900,30) then dbms_output.put_line('Yes 7900,30'); end if;
6 --
7 -- should be false
8 --
9 if emp_in_dept(7499,20) then dbms_output.put_line('Yes 7499,20'); end if;
10 end;
11 /
Yes 7900,30
PL/SQL procedure successfully completed.
All I can really say is …. don’t do that. There really is no excuse for this one, because even without the LISTAGG version, the first procedure indicates that:
- the developer knew of the EMP table and its columns
- the developer knew the data was the DEPT/EMP relationship
- the developer knew how to query the table for a given DEPT
so why not implement the routine with the trivial SQL to meet the requirement?
SQL> create or replace
2 function emp_in_dept(p_emp int, p_dept int) return boolean is
3 l_dept int;
4 begin
5 select count(case when deptno = p_dept then 1 end)
6 into l_dept
7 from emp
8 where empno = p_emp;
9 return l_dept > 0;
10 end;
11 /
Function created.
Therein lies the rub. As developers, we have drilled into us constantly “reuse, reuse, reuse” as if re-using code is the goal as opposed a good guiding principle.
So please…by all means look for opportunities to modularise and re-use your code, but don’t force it into your codebase just for sake for claiming your doing it. That isn’t code RE-use, that is brain NO-use
Learning is not a spectator sport 