Is this statement repeating itself (code review) or can the last 2 be combined [message #435534] |
Wed, 16 December 2009 10:21 |
sjackson1972
Messages: 13 Registered: December 2009
|
Junior Member |
|
|
Global Variable
year varchar2(4)
ssn number(10)
name varchar2(50)
error boolean
cmd varchar2(500)
step number(10)
id_count number(10)
tsn number(10)
last_ids varchar2(20)
Procedure School (test1 in varchar2:= 'none'
test1 in varchar2:= 'all',
auto in varchar2:= 'n');
if auto="n" then
if (test1 is null or test1='none') and
(test2 is null or test 2 ='all') then
score :=A;
dbms_output.put_line('open book1');
open book1;
elsif (test1 is not null and test1 =! 'none) then
score:=B;
this_test1 := test1 || %;
dbms_output.put_line('open book2');
open book2;
elsif (test2 is not null and test2 != all) then
score:=C;
this_test2 := test2 || %;
dbms_output.put_line('open book3');
open book3;
endif;
else
// I think the last 2 are repeating or can be combined
[b]if score = A then
dbms_output.put_line(open book1);
open book1;
elsif score = B then
this_test1 := test1 || %;
dbms_output.put_lineoutput(open book2)
open book2;
elsif score = c then
dbms_output.put_line(Open book3)
open book3;
endif;
endif;
Loop
Begin
Error := 'false'
Cmd := 'fetch book'
Step := '001'
Id_count : = 0;
Tsn := ' ';
Last_ids :=' ' ;
if score = A then
fetch book 1 into ssn,year,name
if book1%notfound then
exit;
endif;
elsif score = B then
fetch book 2 into ssn, year,name
if book2%not found then
exit;
endif;
elsif core =c then
fetch book3 into ssn,year,name;
if book3%not found then
exit;
endif;[/b]
[Updated on: Wed, 16 December 2009 10:38] Report message to a moderator
|
|
|
|
|
|
|
Re: Is this statement repeating itself (code review) or can the last 2 be combined [message #435560 is a reply to message #435558] |
Wed, 16 December 2009 16:51 |
sjackson1972
Messages: 13 Registered: December 2009
|
Junior Member |
|
|
i have already combined the cursors after i posted the question.
the only thing i have so far is a print out of the field names data types and table names. I don't have an er diagram or anything, so basically i'm just shooting from the hip hoping I can get help..This procedure is about 9 pages long and I'm trying to analyze the code bit by bit. I was thinking that I could change some of the if/elsif statements to case statements..
|
|
|
|
|
|
|
|
Re: Is this statement repeating itself (code review) or can the last 2 be combined [message #435568 is a reply to message #435567] |
Wed, 16 December 2009 19:36 |
sjackson1972
Messages: 13 Registered: December 2009
|
Junior Member |
|
|
The code was written back in 1994 (oracle 8i). The client feels that the code written is to long. I posted in a previous post that the manager handed me 54 pages worth of pl/sql code. Its a pl/sql package with about 9 or more procedures doing this execution. They just basically just want a shorter(condensed)version of what they have been using in the past. My job is to see if some of the sql statements,procedure,loops etc can be consolidated. So, i'm guessing instead of having 10 procedures, 30 loops etc..they want me to clean up the code that is already there . So, i'm going through all 54 pages bit by bit or should i say procedure by procedure and trying to make each procedure shorter but at the sametime obtaining the same results/output.(this I will not know until my system gets set up)
Now, my resources are limited as you can tell, so I had to seek help from this forum. So, in a nutshell i guess i'm taking what is already done and putting the long version into a short version. I do know that they will be running this in Oracle 11, so anything as far as syntax errors or code not compiling will come later.
for example here is an example of 2 of the queries written in a cursor
Select ta.country,ta.id, sa.city, sa.state,s.zipcode
from site ta, location sa, area s
where process in 9
and s.site_seq = ta.site_seq
and sa.site_nbr=ta.site_nbr
and ta.datefile is not null
and ta.movefile is not null
and to_char(ta.datetime,'yyyy')=this_year
order by id,ta. country,s.zipcode,sa.city
Select ta.country,ta.id, sa.city, sa.state,s.zipcode
from site ta, location sa, area s
where process in 9
and s.site_seq = ta.site_seq
and sa.site_nbr=ta.site_nbr
and (ta.datefile is null or ta.autostep is null)
and to_char(ta.datetime,'yyyy')=this_year
order by id,ta. country,s.zipcode,sa.city
I hope this makes since.
[Updated on: Wed, 16 December 2009 19:39] Report message to a moderator
|
|
|
Re: Is this statement repeating itself (code review) or can the last 2 be combined [message #435579 is a reply to message #435568] |
Wed, 16 December 2009 23:19 |
Frank
Messages: 7901 Registered: March 2000
|
Senior Member |
|
|
If this is 8i code, they are plain lucky the code never failed.
All the unconditional dbms_output calls might very well have caused a buffer overflow in the dbms_output buffer.
It is a bad habit to have such (unconditional) calls in production code, so I would advise to create some logging procedure that makes it possible to switch logging on or off from the outside.
Furthermore, how about rewriting this 9(!) page long procedure into something more structured, like a package? That will definitely improve the readability of it plus it will ease the task of reusing pieces of code. To not break existing code, the original procedure can be a wrapper for the package.
But said all this, I agree with the comments of others: having to alter this with only a hard copy is insane.
|
|
|
|
|
|
Re: Is this statement repeating itself (code review) or can the last 2 be combined [message #435636 is a reply to message #435611] |
Thu, 17 December 2009 03:04 |
ThomasG
Messages: 3212 Registered: April 2005 Location: Heilbronn, Germany
|
Senior Member |
|
|
Or, if it's only a print-out, then delete every second line, and when it doesn't compile blame it on the persons that typed the print-out into the database again.
Without at least SOME specification about what the package actually does, AND a electronic copy of the script you should charge a hefty extra fee for reverse-engineering what it actually does.
|
|
|