Develop a keen eye for unnecessary code

We've been offering quizzes on the PL/SQL Challenge (and now the new Oracle Dev Gym - still in an "early adaptor" user testing phase) since April 2010. We've covered hundreds of topics in PL/SQL, and hundreds more in SQL. Most quizzes are multiple choice, and one of my favorite question style is to ask: what code in my program unit is unnecessary?

By "unnecessary" we mean that the code can be removed without affecting the behavior of the program unit.

There can be two reasons, roughly, for a chunk of code to be unnecessary:

1. The code "reinforces" or explicitly defines default behavior. If you remove it, the default comes into play. So no harm done, but it is often beneficial to be explicit.

2. You misunderstand how the language works and therefore write code that should not be there at all, and is likely to cause maintenance issues later (and maybe even lead to bugs).

I offer an exercise below in identifying unnecessary code. See if you can figure out what can be removed, before looking at the explanations below the procedure definition.

You can also check out the quiz behind this post.

So here goes: what code can be removed from this procedure definition without change the behavior of running this block:

BEGIN 
   plch_busy_proc; 
END;
/
CREATE OR REPLACE PROCEDURE plch_busy_proc (
      n_in NUMBER DEFAULT NULL) 
   AUTHID DEFINER
IS
   PRAGMA AUTONOMOUS_TRANSACTION;

   CURSOR objects_cur
   IS
        SELECT *
          FROM all_objects
         WHERE ROWNUM < 5
      ORDER BY object_name;

   object_rec objects_cur%ROWTYPE;
BEGIN
   FOR object_rec IN objects_cur
   LOOP
      DBMS_OUTPUT.put_line (object_rec.object_name);
   END LOOP;

   IF object_rec.object_name IS NOT NULL
   THEN
      DBMS_OUTPUT.put_line (object_rec.object_name);
   END IF;
EXCEPTION
   WHEN OTHERS
   THEN
      IF objects_cur%ISOPEN
      THEN
         CLOSE objects_cur;
      END IF;

      DBMS_OUTPUT.put_line (STANDARD.SQLERRM);
END;
/

OK, time to pause.

Examine the code. above

Do not look below for the answers.

Try to figure it out yourself first.

Enhancing your skill at analyzing code greatly improves your ability to write it in the first place

What Can Be Removed?

We'll move from the top on down to the bottom.

(n_in NUMBER DEFAULT NULL) 

Yes, it is true: the entire parameter list can be removed. Why? The only test case you are measuring this against is the anonymous block shown above, which calls the procedure without any arguments. It does not need to pass a value for n_in, because it has a default value of NULL.

Should you therefore remove the parameter list? Very likely not! :-)

Almost certainly, other program units will call the procedure with a non-null value.

So while it is good to understand how PL/SQL takes advantage of default values for parameters, it is not a motivation to remove parameters!

AUTHID DEFINER

Yep, this clause can be removed, because DEFINER is the default setting for AUTHID. The AUTHID clause determines if your program unit runs under one of the following paradigms:

1. Definer Rights (AUTHID DEFINER): when you invoke the program unit, it executes with the privileges of the defining (owning) schema. So even if your user does not have, for the above procedure SELECT or READ privileges on ALL_OBJECTS, the procedure will still execute, because the owner of the procedure did have the ability to select from ALL_OBJECTS.

2. Invoker Rights (AUTHID CURRENT_USER): when you invoke the program unit, it executes with your privileges (the current user). This means that the PL/SQL runtime engine resolves all references to database objects before executing the code.

Check out the doc for more information on these two approaches.

And, just to be really clear (I was not when I initially published this post, as SvenW's comment makes clear): while you can remove AUTHID DEFINER, you should not. You should always include an explicit AUTHID specification for your program units.

At a minimum, its presence makes an unambiguous statement to those coming along later: "This should be a definer rights program unit. Don't change it without a very good reason and thorough analysis!"

PRAGMA AUTONOMOUS_TRANSACTION;

This pragma (a command to the compiler) can be removed because the procedure contains no non-query DML (insert, update, delete, merge). Since it does not change any data in tables, defining the procedure as an autonomous transaction is irrelevant.

An autonomous transaction program unit is one in which all changes made within the unit can and must be committed or rolled back, without affecting other un-committed changes in your session. It is most commonly used in error logging routines, since you want to save the error information to your table, but you certainly do not want to commit changes in your most likely "broken" transaction (you do, after all, have an error). And when you rollback the transaction, you don't want to lose your error log insertion. Autonomous transaction to the rescue!

Check out the doc on this pragma.

object_rec objects_cur%ROWTYPE;

What? I can remove the whole declaration of this record? But how will the procedure even compile, then? Don't I use it here, for example?

    DBMS_OUTPUT.put_line (object_rec.object_name);

Welllllll, yes and no. Yes, you do reference a record with the name "OBJECT_REC". No, you do not reference the record which was explicitly defined in the declaration section.

Instead, you are referencing this record, declared implicitly by PL/SQL for use within the cursor FOR loop:

    FOR object_rec IN objects_cur

I have seen developers declare their own variable as a sort of "programmer's insurance." The thinking seems to go like this: "I will declare a variable with that name, just in case."

"Nooooooooo!" (that's me howling in dismay)

Never write code "just in case". Instead, make sure you know how the technology works and use it as directed, and as necessary.

In this case, because I declared that unnecessary variable, I could write even more, more confusing code, namely:

IF object_rec.object_name IS NOT NULL
THEN
   DBMS_OUTPUT.put_line (object_rec.object_name);
END IF;

Yes, that's right. The entire IF statement can - and should - be removed.

The object_rec.object_name field (of the object_rec variable explicitly declared) will always be NULL. That's because all the fields in the variable are defaulted to NULL initially, and those values are never changed.

"But, but...what about in the cursor FOR loop?" you might be asking yourself.

Oh yes, the value of object_rec.object_name is set  with each new row fetched, but remember: the object_rec modified inside the cursor FOR loop is not the same object_rec that is declared and referenced outside the loop.

This is a great example of how buggy, confusing code can creep into your programs if you do not thoroughly understand the programming language you are using.

IF objects_cur%ISOPEN
THEN
   CLOSE objects_cur;
END IF;

The objects_cur cursor is only open inside the cursor FOR loop. If an error was raised from within the loop (and it is quite difficult to see how that could happen), the PL/SQL engine would automatically close the cursor. So it will never be open in the exception handler, and this code is both unnecessary and misleading.

It should be removed.

STANDARD.

Really? I can remove the name of the package in which SQLERRM is defined? Yes, because STANDARD is one of two very special packages in PL/SQL (the other being DBMS_STANDARD).

As the doc explains, "The package specification declares public types, variables, exceptions, subprograms, which are available automatically to PL/SQL programs...The contents of package STANDARD are directly visible to applications. You need not qualify references to its contents by prefixing the package name."

Sure, you could include the "STANDARD." prefix, but I suggest you do not. It will mostly just raise questions from other developers on your team. It is a fundamental aspect of the way PL/SQL works and is not going to change. So don't clutter up your code.

So did I miss anything? Are there any other chunks of code you believe could and/or should be removed from this program unit?