Re: Request PL/SQL Advice

"nigel_puntridge" <someone_at_microsoft.com> wrote in message news:xzf29.176653$%%2.7289091_at_news2.east.cox.net...
> I have written the following procedure to generate a userid for an
employee
> who is hired into an HR system. It first checks for the existence
> lastname_firstname, if it exists, then lastname_firstname_mi, then it uses
a
> numeric index as a tie breaker.
>
> The logic seems ok, and everything works, but I would like some critiques
> from the experts. I write PL/SQL about once a year. Is there a better
way
> to do this?
>
> Another problem I have is that if someone enters and invalid empid (e.g.
> select gen_user_id('bad emp id') from dual), I get an error saying that
the
> function is returning a null value. Is this normal, and is there a way to
> make it more graceful?
>
>
>
>
> CREATE OR REPLACE FUNCTION gen_user_id(p_empid IN emp.empid%TYPE)
> RETURN VARCHAR2
> IS
> BEGIN
> FOR x IN (SELECT
> n.last_name || '_' || n.first_name) AS LNFN,
> n.last_name || '_' || n.first_name || n.middle_initial) AS
> LNFNMI,
> FROM emp n
> WHERE n.empid = p_empid)
> LOOP
> IF x.lnfn = NULL THEN
> RETURN NULL;
> END IF;
>
> FOR a IN (SELECT count(*) acount
> FROM system_users a
> WHERE a.user_id = x.lnfn)
> LOOP
> IF a.acount = 0 THEN
> RETURN x.lnfn; -- No pre-existing LASTNAME || FIRSTNAME,
so
> return it
> END IF;
> END LOOP;
>
> FOR b IN (SELECT count(*) bcount
> FROM system_users b
> WHERE b.user_id = x.lnfnmi)
> LOOP
> IF b.bcount = 0 THEN
> RETURN x.lnfnmi; -- No pre-existing LASTNAME || FIRSTNAME
||
> MI, so return it
> END IF;
> END LOOP;
>
> FOR c IN (SELECT to_char(count(*) + 1) ccount
> FROM system_users c
> WHERE c.user_id like x.lnfnmi || '%')
> LOOP
> RETURN x.lnfnmi || c.ccount;
> END LOOP;
> END LOOP;
> END;
> /
>
>
>

You really want this to be commented?
The code is absolutely awful and the logic is most likely NOT ok. First of all:
If you are selecting either 1 or 0 records, you shouldn't use a for loop, and you shouldn't use count(*), you should use a SELECT INTO and proper exception handling.
Your other FOR LOOP is *wrong*. If p_empid can't be found in the table, the FOR LOOP won't retrieve anything and your code falls through instead of raising an exception.
You are allowing for NULL first_name, last_name combinations, which is why you get NULL returned, you have coded this yourself.

This
> FOR a IN (SELECT count(*) acount
> FROM system_users a
> WHERE a.user_id = x.lnfn)
> LOOP
> IF a.acount = 0 THEN
> RETURN x.lnfn; -- No pre-existing LASTNAME || FIRSTNAME,
so
> return it
> END IF;
> END LOOP;
>

is *awful* as the select will ,assuming user_id to be unique, return 1 or 0 rows.
It should read
begin
select 'x'
into dummy
from systems_users a
where a.user_id = x.lnfn
exception
 when no_data_found then

      return x.lnfn;
end;
This piece
> FOR b IN (SELECT count(*) bcount
> FROM system_users b
> WHERE b.user_id = x.lnfnmi)
> LOOP
> IF b.bcount = 0 THEN
> RETURN x.lnfnmi; -- No pre-existing LASTNAME || FIRSTNAME
||
> MI, so return it
> END IF;
> END LOOP;

Has the same problem

This piece

> FOR c IN (SELECT to_char(count(*) + 1) ccount
> FROM system_users c
> WHERE c.user_id like x.lnfnmi || '%')
> LOOP
> RETURN x.lnfnmi || c.ccount;
> END LOOP;
allows for multiple records because of the '%' wildcard, and it will as the return
returns from the function, it will see the first only. I will leave it to you to draw your conclusions from my comments, as far as I am concerned, they are quite obvious.

Regards

--
Sybrand Bakker
Senior Oracle DBA

to reply remove '-verwijderdit' from my e-mail address