Four Resolutions for Better Code

It’s always time to write better PL/SQL.

By Steven Feuerstein

March/April 2015

On the one hand, it’s a little late for New Year’s resolutions. On the other hand, it is never too late to explore ways to improve the quality of our code. And because I am writing this article in January, it sure seems like the right time for me to think about ways to write better PL/SQL code.

Have you ever noticed that a lot of the code you write looks a lot like code you wrote last week or last month? We often follow patterns in our software: write a cursor FOR loop like this, construct a function like that. And because humans are such creatures of habit, we also tend to follow antipatterns: suboptimal ways of writing code.

In this article, I offer up several small-scale antipatterns: the kinds of bad habits that can creep (or carry over from the past) into our code on a line-by-line basis. But I don’t stop there. I then offer patterns that you and the people with whom you work should be happy to follow this year and in the years to come.

Long ago in a database far away, when PL/SQL was just a young language, it did not support all the standard SQL functions such as SYSDATE. So if you needed to get the current system date, you needed to “fall back” on SQL as follows:

DECLARE
  now DATE;
BEGIN
  SELECT SYSDATE INTO now FROM dual;
END;

The need to use SQL to execute SYSDATE has long been a thing of the past.

Many developers also think that when they need to get the next value from a sequence in PL/SQL, they have to write code like this:

DECLARE
  l_nextkey my_table.my_pky%TYPE;
BEGIN
  SELECT my_table_seq.NEXTVAL
    INTO l_nextkey
    FROM dual;
END;

Not true! As of Oracle Database 11g, you can now natively call both the NEXTVAL and CURRVAL directly inside PL/SQL, as in

DECLARE
  l_nextkey my_table.my_pky%TYPE;
BEGIN
  l_nextkey := my_table_seq.NEXTVAL;
END;

The bottom line is that if you are using Oracle Database 11g or higher, the days of needing to use SQL to execute functionality not available in PL/SQL are pretty much gone.

If you see a “SELECT FROM dual” in your code, I suggest you ask: “Do I really need that? Can’t I just call it directly in PL/SQL?”

And, of course, it is easy to discover the answer to that question: give it a try!

I like the BOOLEAN datatype. I am glad it is available in PL/SQL and look forward to the day when I can create a column in a table of type BOOLEAN.

I like BOOLEAN because so much code in programs involves conditional logic, such as “If x is true, then y” or “While x is true, execute loop body.”

Without a BOOLEAN datatype, each instance of “x” must be an expression, and that expression can be quite complex, as in

IF (l_employee.salary < c_max_salary
    AND
    l_employee.hire_date <
      ADD_MONTHS (SYSDATE, -60))
    OR
    override_limit_in = 1
THEN

That complexity then leads us to write comments to explain the code. A comment for the above logic might be

/* If employed for more than
   five years and not yet
   at maximum salary or user
   requested override... */

With a BOOLEAN datatype, everything gets clearer. Here’s a rewrite of that complex condition expression:

IS... previous declarations ...
  c_below_max_salary CONSTANT BOOLEAN :=
    l_employee.salary < c_max_salary;
  c_employed_at_least_5_years CONSTANT
    BOOLEAN :=
    l_employee.hire_date <
       ADD_MONTHS (SYSDATE, -60));
  c_override_flag CONSTANT BOOLEAN :=
    override_limit_in = 1;
BEGIN
  IF (c_below_max_salary AND
      c_employed_at_least_5_years) OR
      c_override_flag = TRUE
  THEN

So one antipattern to watch out for is the complicated expression embedded inside an IF statement. Instead, use Boolean variables and constants to hide the details behind a name.

There is, though, a second Boolean antipattern lurking in that rewritten code:

c_override_flag = TRUE

All too often, I come across code that looks like

IF l_flag = TRUE

or

CASE
   WHEN l_at_min_balance
        = FALSE

or

WHILE (l_found_it <> TRUE) LOOP

This code is valid, but it is not as intuitive as it could be and it reflects a misunderstanding of Booleans.

Here’s the thing: a Boolean can be only TRUE or FALSE (or, in the world of Oracle Database, TRUE, FALSE, or NULL). So there’s no reason to write “IF boolean_var = TRUE”. You can just write “IF boolean_var”. And then you will quickly discover whether you named your variable properly.

Consider the IF statement that contained Boolean constants:

IF (c_below_max_salary AND
    c_employed_at_least_5_years) OR
   c_override_flag = TRUE
THEN

I argue that the “= TRUE” is entirely unnecessary. That expression could simply be

IF (c_below_max_salary AND
    c_employed_at_least_5_years) OR
   c_override_flag
THEN

OK, so there’s a little bit less code, and that’s almost always a good thing. But now when I read that expression, it doesn’t sound right: “If this employee is below the maximum salary and started more than 5 years, or override flag.”

Huh?

Whenever you declare anything, you want to make sure the name describes as accurately as possible the value that thing will hold (if it’s a variable) or represents (such as a type). That is especially true for Booleans.

My constant, c_override_flag, is a flag—an indicator of the state of the override request. But the name of the constant does not reveal that state: was the override requested or not? A small change in the name removes that ambiguity, resulting in more-readable code:

IF (c_below_max_salary AND
    c_employed_at_least_5_years) OR
   c_override_requested
THEN

So remember: a Boolean can be only TRUE, FALSE, or NULL. Don’t write, “IF my_boolean IS TRUE”. Just write, “IF my_boolean”. And then make sure you use descriptive names for your constants and variables so that the code tells its story smoothly and clearly.

By the way, Boolean is capitalized because that datatype is named after George Boole, the father of symbolic logic (more information on Boole is available at wikipedia.org/wiki/George_Boole).

If something goes wrong when you execute your code, the PL/SQL runtime engine will raise an exception. You can then choose to handle that exception in your block or let it propagate out to the enclosing block.

Or if you came to PL/SQL from another language that did not offer a similar exception model, you might write code like this:

FUNCTION total_salary (
   total_out OUT NUMBER)
   RETURN PLS_INTEGER
IS
   l_total NUMBER;
BEGIN... some_code ...
   l_total := total_out;
   RETURN 1;
EXCEPTION
   WHEN OTHERS THEN
      RETURN 0;
END;

You might then call this function:

BEGIN
   l_status :=
      total_salary (l_total);
   IF l_status = 1 THEN ... continue ...
   END IF;

Yes, you can write PL/SQL code this way, but it is a bad idea, for multiple reasons:

• You (and anyone else calling this function) then have to take responsibility for checking the status code.
• Status codes usually end up being hard-coded, reducing readability and increasing opportunities for bugs.
• You end up with functions that have OUT parameters. This is allowed in PL/SQL but should be avoided, because it restricts the ways that function can be used. (Most importantly, it cannot be called from within a SQL statement.)
• You are working around the native paradigm of the language, something that should be done only when absolutely necessary.

And that approach to exception handling is certainly not necessary in this case.

Use the built-in exception raising and handling framework of PL/SQL. If you do not need to trap an exception, let it propagate out unhandled. If you need to implement special logic for specific errors, then write a handler just for that:

FUNCTION total_salary RETURN NUMBER
   RETURN PLS_INTEGER
IS
   l_total NUMBER;
BEGIN ... some_code ...
   RETURN l_total;
EXCEPTION
   WHEN ZERO_DIVIDE
   THEN
      RETURN 0;
END;

Then you leave it to the invokers of your function to decide what to do about exceptions. If they choose to do nothing special (the equivalent of forgetting to check the status code after calling the first version of total_salary), no problem. If an exception propagates out of total_salary, the invoking block will be terminated by PL/SQL.

The resulting code is simpler, easier to maintain, and far easier to support.

BEGIN
   l_total :=
      total_salary ();
   ... continue ...
EXCEPTION
   WHEN OTHERS THEN
      log_error;
END;

One of the mantras of structured programming is, “One way in, one way out.”

For example, there should be just one way into a loop or a function and just one way to leave that loop or function.

In PL/SQL there is generally only one way to go into or start a chunk of code: you call the function, and in you go. You start the loop, and in you go. But it is all too easy to write loops with multiple termination paths. It is also certainly possible, and all too common, to write functions with multiple RETURN statements.

Here’s an example:

FUNCTION status_desc (
   cd_in IN VARCHAR2)
   RETURN VARCHAR2
IS
BEGIN
   IF cd_in = 'C'
      THEN RETURN 'CLOSED';
   ELSIF cd_in = 'O'
      THEN RETURN 'OPEN';
   ELSIF cd_in = 'A'
      THEN RETURN 'ACTIVE';
   ELSIF cd_in = 'I'
      THEN RETURN 'INACTIVE';
   END IF;
END;

The problem with having multiple ways out is that it makes the debugging process much harder. I call my function. It returns the wrong value. OK, but which RETURN returned the wrong value? Which logic branch did it follow? And if I want to trace (write to a log table) what my function is doing, I have to insert trace code before each return. Finally, with multiple RETURNs, it is more likely that there will be a pathway that does not execute any RETURN statement. The following demonstrates the result when I call the status_desc function with an undefined code:

BEGIN
   DBMS_OUTPUT.PUT_LINE (
      status_desc ('X'));
END;
/
ORA-06503: PL/SQL: Function returned
without value

This is an embarrassing error for a user to have to report. It sends a clear message that I designed and tested my code very poorly.

Fortunately, it is quite easy to fix this antipattern: limit yourself to just one RETURN in the executable section of the function, and make it the very last statement in that section.

I rewrote the status_desc function to follow this approach:

FUNCTION status_desc (
   cd_in IN VARCHAR2)
   RETURN VARCHAR2
IS
BEGIN
   RETURN
      CASE cd_in
        WHEN 'C' THEN 'CLOSED'
        WHEN 'O' THEN 'OPEN'
        WHEN 'A' THEN 'ACTIVE'
        WHEN 'I' THEN 'INACTIVE'
      END;
END;

Now it is literally impossible for this function to cause an ORA-06503 error. Either the last (and only) statement in the function is executed, or an exception is raised.

The PL/SQL compile time warnings feature can help you with this antipattern. There are no warnings for “Multiple RETURNS in your function.” But the PL/SQL compiler can tell you if it has identified at least one possible logic path in your function that would result in no RETURN being executed (and the raising of the ORA-06503 error at runtime): the PLW-05005 warning.

Assuming that the first version of status_desc with multiple returns is still compiled into the database, I will see the following when I turn on warnings and recompile:

ALTER SESSION SET plsql_warnings =
'ENABLE:5005'
/
ALTER FUNCTION status_desc COMPILE
/
PLW-05005: subprogram STATUS_DESC
returns without value at line 15

But the function is valid, so even if the compiler produces this warning, the function can still be executed.

If that bothers you, tell the compiler to treat PLW-05005 as a compile error (because you really don’t want to embarrass yourself in the eyes of your users on this one):

ALTER SESSION SET plsql_warnings =
'ERROR:5005'
/
ALTER FUNCTION status_desc COMPILE
/
PLS-05005: subprogram STATUS_DESC
returns without value at line 15

Note that the message now starts with “PLS” and not “PLW”. This indicates that 5005 was treated as an error and not a warning. The status of this function is now INVALID.

The situation with loops is similar to that with functions, but the thing to watch out for is one or more EXIT statements, rather than RETURNs. Consider the following block:

BEGIN
  FOR indx IN 1 .. my_collection.COUNT
  LOOP    ... some code ... 
     IF my_collection (indx) =
        l_termination_value
     THEN
        EXIT;
     END IF;
  END LOOP;
END;

There are a couple of problems with this block. First of all, from the standpoint of code compactness, that IF statement should be replaced with an EXIT WHEN, as in

EXIT WHEN
   my_collection (indx) =
  l_termination_value

But the bigger problem is that the EXIT WHEN should not be used inside a FOR loop. A FOR loop says to anyone reading the code: “I am going to execute the body N times.” In other words, the built-in way “out” of a FOR loop is to execute the body the Nth time.

If, however, there is an EXIT statement inside the FOR loop, you have a potential second exit path. In this case, you should switch to either a simple loop or a WHILE loop. Here’s the WHILE loop implementation:

DECLARE
   c_coll_count PLS_INTEGER :=
      my_collection.COUNT;
   l_found_terminator BOOLEAN
      := FALSE;
   l_index PLS_INTEGER := 1;
BEGIN
   WHILE (NOT l_found_terminator AND
           l_index < c_coll_count)
   LOOP
      ... some code ... 
      l_found_terminator :=
          my_collection (indx) =
          l_termination_value;
      l_index := l_index + 1;
   END LOOP;
END;

I am certain that some readers will object. This rewrite contains a lot more code. Surely the original version is clear enough, even if there are multiple exit pathways?

For this simple example, I would agree. But when you are working on a more complex piece of code and a loop body that contains 50 or 100 lines of code, the possible pain of debugging will better justify the rewrite.

So remember:

• Include only one RETURN in your function’s executable section—and make it the very last line of that section.
• Use compile time warnings to identify potential runtime issues in your code.
• Avoid EXIT and EXIT WHEN statements in FOR and WHILE loops.

When we build applications, we have to think of and code everything. As a result, even the smallest points of confusion or ambiguity can accumulate into a hard-to-manage application. Every resolution of an antipattern, either large or small, contributes to a high-quality application that developers can understand and enhance more easily.

CREATE OR REPLACE PROCEDURE plch_bad_proc
IS
   l_number   NUMBER;
BEGIN
   l_number := 'abc';
END;
/
BEGIN
   plch_bad_proc;
   /* Rest of program */
   NULL;
EXCEPTION
   WHEN VALUE_ERROR
   THEN
      DBMS_OUTPUT.put_line (SQLCODE);
END;
/

CREATE OR REPLACE FUNCTION plch_bad_func
   RETURN INTEGER
IS
   l_number   NUMBER;
BEGIN
   l_number := 'abc';
   RETURN 0;
EXCEPTION
   WHEN OTHERS
   THEN
      RETURN SQLCODE;
END;
/
DECLARE
   l_status PLS_INTEGER;
BEGIN
   l_status := plch_bad_func;
   IF l_status = 0
   THEN
      /* Continue! */
      NULL;
   ELSE
      DBMS_OUTPUT.put_line (l_status);
      /* Rest of program */
      NULL;
   END IF;
END;
/

CREATE OR REPLACE PACKAGE plch_err_pkg
IS
   most_recent_error   PLS_INTEGER := 0;
END;
/
CREATE OR REPLACE PROCEDURE plch_bad_proc
IS
   l_number   NUMBER;
BEGIN
   l_number := 'abc';
EXCEPTION
   WHEN OTHERS
   THEN
      plch_err_pkg.most_recent_error := SQLCODE;
END;
/
BEGIN
   plch_bad_proc;
   IF plch_err_pkg.most_recent_error = 0
   THEN
      /* Continue! */
      NULL;
   ELSE
      DBMS_OUTPUT.put_line (plch_err_pkg.most_recent_error);
      plch_err_pkg.most_recent_error := 0;
   END IF;
END;
/