Re: Request PL/SQL Advice

From: nigel_puntridge <someone_at_microsoft.com>
Date: Thu, 01 Aug 2002 23:41:25 GMT
Message-ID: <FGj29.177291$%%2.7333722@news2.east.cox.net>

Thanks for your comments. If you think my PL/SQL is good, you should see my Java code. I applied your suggestions. One minor point: A Count function without a GROUP BY will always return one and only 1 row, irrespective of the WHERE clause.

But now, when I call the function, and try and insert into the system_users table, I get a Mutating Table error (04091).

Time to call it a day.

CREATE OR REPLACE FUNCTION gen_userid(p_empid IN empA.empid%TYPE)

   RETURN VARCHAR2
IS
  dummy VARCHAR2(1);
  l_reccount VARCHAR2(2);
BEGIN
    FOR x IN (SELECT

             UPPER(n.last_name || '_' || n.first_name) AS LNFN,
              UPPER(n.last_name || '_' || n.first_name ||
substr(rtrim(n.middle_name,' '),1,1)) AS LNFNMI
              FROM empa n
              WHERE n.empid = p_empid)
    LOOP
      BEGIN
          SELECT 'x'
          INTO dummy
          FROM system_users a
          WHERE a.userid = x.lnfn;
       EXCEPTION
          WHEN NO_DATA_FOUND THEN
          RETURN x.lnfn;
       END;
       BEGIN
          SELECT 'x'
          INTO dummy
          FROM system_users a
          WHERE a.userid = x.lnfnmi;
       EXCEPTION
          WHEN NO_DATA_FOUND THEN
          RETURN x.lnfnmi;
       END;
       BEGIN
          SELECT TO_CHAR(count(*) + 1)
          INTO l_reccount
          FROM system_users c
          WHERE c.userid like  x.lnfnmi || '%';
          RETURN x.lnfnmi || l_reccount;
       END;

    END LOOP;
EXCEPTION
   WHEN NO_DATA_FOUND THEN
   RETURN NULL;
END;
/

"Sybrand Bakker" <postbus_at_sybrandb.demon.nl> wrote in message news:ukj94k8srihjde_at_corp.supernews.com...
>
> "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
>
>
>
Received on Thu Aug 01 2002 - 18:41:25 CDT