For quite a long time I did not work anymore with PL/SQL and was quite happy when I had the chance to review some code at a customer. The status today: I am not that happy anymore 🙁 Let me explain why and show you some examples on what was discovered. Of course all of the identifiers have been obfuscated and this is not to blame anyone. It is more to make people aware of what you should not do and that you have to be as exact as possible when you do programming. Even more important: Code that you write must be maintainable and understandable by others.

We start with a simple “if-then-else-end if” block:

         IF c <= d
         THEN
            IF c = d
            THEN
               l2 := l2 || l3;
            ELSE
               l2 := l2 || l3 || ',';
            END IF;
         END IF;

So what is wrong with this? The code does what it is supposed to do, true. But this is more complicated than it needs to be. Lets look at the first line:

         IF c <= d

If we enter this “IF” then we know that c is either less than d or equal to d, correct? On line 3 we know that c is d if we enter that “IF”. What does this imply for line 6 (the “ELSE”)?
It implies that c is less than d when we enter the “ELSE”. But then we could also write it like this:

            IF c = d
            THEN
               l2 := l2 || l3;
            ELSIF c < d THEN
               l2 := l2 || l3 || ',';
            END IF;

This is much more clear and less code. We are only interested if c is equal to d or less than d, that’s it. Then we should design the code exactly for that use case.

The next example is about the usage of a record. This is the code block:

         ...

         ll_col1 SCHEMA.TABLE.COLUMN1%TYPE;
         ll_col2 SCHEMA.TABLE.COLUMN2%TYPE;
         ll_col3 SCHEMA.TABLE.COLUMN3%TYPE;
         ll_col4 SCHEMA.TABLE.COLUMN4%TYPE;
         ...
         DECLARE
            TYPE MyRecord IS RECORD
            (
               l_col1    SCHEMA.TABLE.COLUMN1%TYPE,
               l_col2    SCHEMA.TABLE.COLUMN2%TYPE,
               l_col3    SCHEMA.TABLE.COLUMN3%TYPE,
               l_col4    SCHEMA.TABLE.COLUMN4%TYPE
            );
            rec1   MyRecord;
         BEGIN
            v_sql := 'SELECT col1, col2, col3, col4 FROM SCHEMA.TABLE WHERE col3 = '''
               || ll_col3
               || '''';
 
            EXECUTE IMMEDIATE v_sql INTO rec1;

            ll_col1 := rec1.l_col1;
            ll_col2 := rec1.l_col2;
            ll_col3 := rec1.l_col3;
            ll_col4 := rec1.l_col4;
         EXCEPTION
            WHEN NO_DATA_FOUND
            THEN
               ll_col3 := 0;
         END;

Beside that declaring a new block inside an existing block is really ugly what is wrong here? There is the definition of a new record which is then used to fetch the data from the dynamic sql statement. Nothing wrong here. But then the fields of this record are written back to four variables which are valid in and outside of that block, why that? Probably because the record is only valid in the “declare-begin-end” block. But for what do I need the record then at all? Really confusing 🙂

The next one is more about how easy it is to understand code written by someone else. This is the code block:

      SELECT COUNT (*)
        INTO l_count
        FROM USER_INDEXES
       WHERE INDEX_NAME = '' || l_index_name || '';

      IF l_count > 0
      THEN
         EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || '';
      END IF;

I really had to read this carefully until I understood why it was written this way. Probably the developer had this intension: “I want to know if a specific index exists and if yes, then I want to drop it”.
But what he did actually code is: I want to know if there are multiple indexes with the same name. As the query is against user_tables the answer can not be more than 1, correct? But then the correct and much more easy to understand way of writing this would be:

      SELECT COUNT (*)
        INTO l_count
        FROM USER_INDEXES
       WHERE INDEX_NAME = '' || l_index_name || '';

      IF l_count = 1
      THEN
         EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || '';
      END IF;

This is just a small change and does not affect the functionality at all but it is much more clear. Even more clear would be something like this:

      BEGIN
        SELECT 'index_exists'
          INTO l_exists
          FROM USER_INDEXES
         WHERE INDEX_NAME = '' || l_index_name || '';
      EXCEPTION
        WHEN NO_DATA_FOUND THEN l_exists := 'index_does_not_exist';
      END;
      IF l_exists = 'index_exists'
      THEN
         EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || '';
      END IF;

Using our language we can make the code much more easy to understand. Beside that the concatenation in the where clause and in the execute immediate statement is not required so the final code could look like this:

      BEGIN
        SELECT 'index_exists'
          INTO l_exists
          FROM USER_INDEXES
         WHERE INDEX_NAME = l_index_name;
      EXCEPTION
        WHEN NO_DATA_FOUND THEN l_exists := 'index_does_not_exist';
      END;
      IF l_exists = 'index_exists'
      THEN
         EXECUTE IMMEDIATE 'DROP INDEX ' || l_index_name;
      END IF;

In the next post we’ll look at some other examples which can be improved by changing the way we think about what we code and how others may interpret it.