PL/SQL Maintenance Nightmares (Learning to avoid…)

I’ve worked on some new PL/SQL packages and Forms which work quite well. They are efficient in their use of resources, and are easy for me to debug and maintain. After some other developers have had to do some changes (some from changes to requirements, some from bugs I didn’t find), I’ve learned that the highly modular style of code I use is difficult to modify without introducing new bugs.

The modular style is great while developing; it reduces duplication and provides useful abstractions. When it comes to maintaining that code, however, a new developer has to read all that code and follow its tortuous logic around in order to debug it. It’s much easier to just patch the existing code to force it to work.

This is obviously not the best outcome; more new code is added, instead of the existing code being fixed. The module thus becomes more complex and unmaintainable. After some time, a new developer confronts a hodge-podge of duplicated functionality and cut-and-paste monstrosities, and exclaims, “Who wrote this rubbish?” – perhaps with justification.

What I’ve learned is that after developing a new module, I must go back and refactor it to improve maintainability. This includes a number of steps (this list is not intended to be exhaustive):

1. Delete simple wrappers for library functions/procedures, and replace all calls to them with calls to the original library functions/procedures.
   
2. Don’t use constants for internal codes (e.g. names of items and blocks) – use literals instead.
   
3. If a fairly simple function or procedure is only called once throughout the final code – delete it and move the code to where it was called from; the exception is if this would make the calling code more difficult to read or understand.
   
4. Code that generates dynamic SQL should set the SQL in entirety in single statements if possible, instead of building it procedurally. Another developer should be able to easily find the exact SQL that will be generated just by looking at the code, instead of having to mentally build it procedurally, or having to printline the SQL and run it to see what it generates.
   
5. Add comments that document the philosophy of how the code has been laid out.

Some examples to illustrate the above:

1. We have a library procedure called lk_item.lp_show_item (pc_item, pn_visible, pn_enabled) which is used to show, hide, enable, and/or disable a form item. I wanted to write code like the following: lk_item.lp_show_item(‘my_item’, some_boolean_expression, another_boolean_expression) but I couldn’t because the parameters require PROPERTY_TRUE or PROPERTY_FALSE, which are numbers. My first cut included the following wrapper for lk_item.lp_show_item:
   

   PROCEDURE cp_show_hide
   (pc_item IN VARCHAR2
   ,pb_visible IN BOOLEAN
   ,pb_enabled IN BOOLEAN) IS
    FUNCTION CF_PROPERTY_TF
    (pb_value IN BOOLEAN) RETURN NUMBER IS
    BEGIN
     IF pb_value THEN
      RETURN PROPERTY_TRUE;
     ELSE
      RETURN PROPERTY_FALSE;
     END IF;
    END;
   BEGIN
    lk_item.lp_show_item(pc_item,
     CF_PROPERTY_TF(pb_visible),
     CF_PROPERTY_TF(pb_enabled));
   END;

   
   In the end, however, I only called this procedure two times, and within a single procedure. So I deleted the wrapper procedure, and called lk_item.lp_show_item directly. I kept my Boolean expressions, but wrapped them in a locally declared function that did the translation.
   

   FUNCTION CF_PROPERTY_TF
   (pb_value IN BOOLEAN) RETURN NUMBER IS
   BEGIN
    IF pb_value THEN
     RETURN PROPERTY_TRUE;
    ELSE
     RETURN PROPERTY_FALSE;
    END IF;
   END;
   …
   lk_item.lp_show_item(‘ITEM_NAME’,
    CF_PROPERTY_TF(some expression),
    CF_PROPERTY_TF(some other expression));

2. I had a procedure that populated a large number of items in the block in response to the user selecting a new record. Because the user can select a new record using any of several different methods (effectively, different search criteria), I used a parameter called “search mode” that is set to various constant strings, e.g. “GNL_ID”, “OFF_ID_DETAILS”, “COLLAR_NO”. Some of these strings were named after significant database table columns, some were generic – they all, however, simply differentiated slightly different conditions under which the procedure must operate.

   Initially I had a set of constants that took these values; however, because this code passed these values to a database package as well, the constants were defined in the database package as well. In the end the code was much simpler and easier to read by removing all the constants and just encoding the codes as literal strings.

3. (no example)

4. A first cut of the code might look like (this is a very simplified example):
   

   cp_lexical := ‘SELECT aaa, bee, cee ‘;
   IF (some expression) THEN
    cp_lexical := cp_lexical || ‘, dee FROM bla ‘;
   ELSE
    cp_lexical := cp_lexical || ‘FROM dee, eff ‘;
   END IF;
   IF (some other expression) THEN
    cp_lexical := cp_lexical
     || ‘, gee WHERE eee = eff AND gee = oh ‘;
   ELSE
    cp_lexical := cp_lexical
     || ‘, hat WHERE eee = oh AND ii = jay ‘;
   END IF;
   cp_lexical := cp_lexical
    || ‘ AND kay = ell AND emm = enn’;

   
   The final, more easily maintained code would look something like:
   

   IF (some expression)
      AND (some other expression) THEN
    cp_lexical := ‘SELECT aaa, bee, cee, dee ‘
                    || ‘FROM   bla, gee ‘
                    || ‘WHERE  eee = eff ‘
                    || ‘AND    gee = oh ‘
                    || ‘AND    kay = ell ‘
                    || ‘AND    emm = enn’;
   ELSIF (some expression) THEN
    cp_lexical := ‘SELECT aaa, bee, cee, dee ‘
                    || ‘FROM   bla, hat ‘
                    || ‘WHERE  eee = oh ‘
                    || ‘AND    ii = jay ‘
                    || ‘AND    kay = ell ‘
                    || ‘AND    emm = enn’;
   ELSIF (some other expression) THEN
    cp_lexical := ‘SELECT aaa, bee, cee ‘
                    || ‘FROM   dee, eff, gee ‘
                    || ‘WHERE  eee = eff ‘
                    || ‘AND    gee = oh ‘
                    || ‘AND    kay = ell ‘
                    || ‘AND    emm = enn’;
   ELSE
    cp_lexical := ‘SELECT aaa, bee, cee ‘
                    || ‘FROM   dee, eff, hat ‘
                    || ‘WHERE  eee = oh ‘
                    || ‘AND    ii = jay ‘
                    || ‘AND    kay = ell ‘
                    || ‘AND    emm = enn’;
   END IF;

   
   This doesn’t solve all the maintenance problems (i.e. if someone makes a change to one part of the code they might not know that they should make the same change everywhere else that code has been duplicated). However, it does make maintenance a bit easier because the code is easy to follow.

5. I generally follow the following pattern when encapsulating logic in procedures and packages:
   
   • Code that populates data in items (e.g. in response to changes in other items) is encapsulated in procedures or packages named POPULATE_xxx.
     
   • Code that changes item properties (but not their values) is encapsulated in procedures or packages named SETUP_xxx.
     
   • Code that checks data entry errors is encapsulated in procedures or packages named VALIDATE_xxx.
   
   These conventions are documented so that another developer can more easily read the code; for example, a when-validate-item trigger might have code like this:
   

   cp_validate_something;
   cp_populate_something;
   cp_setup_something;

   
   The developer might still need to follow through all the code to debug it, but they might more easily see where the code should be. Hopefully they’ll recognise that validation code should go in cp_validate_something, rather than cp_populate_something.