Not only does this code have stupid constants, it has stupid code:
SELECT NVL(MAX(XXX_BATCH_JOB_R),0) INTO nBatchNo FROM XXX_BATCH_JOB_CONTROL WHERE XXX_BATCH_JOB_N = sBatchName;
IF nBatchNo = 0 THEN
WHEN NO_DATA_FOUND THEN
:= nBatchNo + 1;
8 similar references to these variables:
LPAD (NVL (preBatchNo,0),6,0) ||
LPAD (NVL (nBatchNo,0),6,0)
Some_Func(sBatchName, nBatchNo, v_updtPgm, nretcode);
There are no other references to these variables.
What is wrong with this code? Let me count the ways:
- The IF statement checks the 0 condition, even though 0 will work just fine.
- If it equals 0, it sets it to 0. Why?
- An EXCEPTION clause is used.
- The EXCEPTION is impossible, as MAX() is an aggregate function, and aggregate functions always return data
- The return is stored in one variable then immediately moved to another variable.
- The new new variable has a non-sensical name.
- The new variable breaks the Hungarian notation used elsewhere.
- The new variable is used eight times with formatting, and no where else. But the formatting is not done in the variable tiself.
- The new variable is always used with the old variable, for formatted output, yet they are kept separate.
- Both variables are NVL()ed over and over again, even though that was handled at the beginning.
There are other issues, such as the lack of a SEQUENCE to truly have unique numbers. But, those are trivial compared to the absolute stupidity of the block itself.
So, how should that code have been written?
SELECT NVL(MAX(XXX_BATCH_JOB_R), 0) INTO nBatchNo FROM XXX_BATCH_JOB_CONTROL WHERE XXX_BATCH_JOB_N = sBatchName;
:= LPAD(preBatchNo, 6, 0) || LPAD(nBatchNo + 1, 6, 0)
Between the block and the NVL()s, i feel that the original coder was a blockhead that added no value at all.