×

Welcome to the Slashdot Beta site -- learn more here. Use the link in the footer or click here to return to the Classic version of Slashdot.

Thank you!

Before you choose to head back to the Classic look of the site, we'd appreciate it if you share your thoughts on the Beta; your feedback is what drives our ongoing development.

Beta is different and we value you taking the time to try it out. Please take a look at the changes we've made in Beta and  learn more about it. Thanks for reading, and for making the site better!

Mini rant: Stupid code

Chacham (981) writes | about 4 months ago

User Journal 2

Not only does this code have stupid constants, it has stupid code:

Not only does this code have stupid constants, it has stupid code:

BEGIN
                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
                nBatchNo := 0;
            END IF;

        EXCEPTION
                WHEN NO_DATA_FOUND THEN
                        nBatchNo := 0;
        END;
              preBatchNo := nBatchNo;
              nBatchNo := 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:

  1. The IF statement checks the 0 condition, even though 0 will work just fine.
  2. If it equals 0, it sets it to 0. Why?
  3. An EXCEPTION clause is used.
  4. The EXCEPTION is impossible, as MAX() is an aggregate function, and aggregate functions always return data
  5. .

  6. The return is stored in one variable then immediately moved to another variable.
  7. The new new variable has a non-sensical name.
  8. The new variable breaks the Hungarian notation used elsewhere.
  9. The new variable is used eight times with formatting, and no where else. But the formatting is not done in the variable tiself.
  10. The new variable is always used with the old variable, for formatted output, yet they are kept separate.
  11. 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;
preBatchNo := 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.

2 comments

Reminiscent of posts on http://thedailywtf.com/ (1)

insanecarbonbasedlif (623558) | about 4 months ago | (#45728145)

Definitely reminds me of posts I've seen on http://thedailywtf.com/ [thedailywtf.com] , though I have to admit I have made my fair share of bad contributions due to mid task restructuring or time or ignorance, so I always keep that in mind when laughing at the terrible code I run across.

Re:Reminiscent of posts on http://thedailywtf.com/ (1)

Chacham (981) | about 4 months ago | (#45728307)

Heh. But this stuff should be really, really basic.

On another note, Going through the effort to write it up here helps me release the want to scream.

Check for New Comments
Slashdot Account

Need an Account?

Forgot your password?

Don't worry, we never post anything without your permission.

Submission Text Formatting Tips

We support a small subset of HTML, namely these tags:

  • b
  • i
  • p
  • br
  • a
  • ol
  • ul
  • li
  • dl
  • dt
  • dd
  • em
  • strong
  • tt
  • blockquote
  • div
  • quote
  • ecode

"ecode" can be used for code snippets, for example:

<ecode>    while(1) { do_something(); } </ecode>
Sign up for Slashdot Newsletters
Create a Slashdot Account

Loading...