Code I Regret: Refactoring as Penance

Recently I refactored some PL/SQL for sending emails – code that I wrote way back in 2004. The number of “WTF“‘s per minute has not been too high; however, I’ve cringed more times than I’d like…

1. Overly-generic parameter types

When you send an email, it will have at least one recipient, and it may have many recipients. However, no email will have more than one sender. Yet, I wrote the package procedure like this:

TYPE address_type IS RECORD
 (name          VARCHAR2(100)
 ,email_address VARCHAR2(200)
 );
TYPE address_list_type IS TABLE OF address_type
 INDEX BY BINARY_INTEGER;
PROCEDURE send
 (i_sender     IN address_list_type
 ,i_recipients IN address_list_type
 ,i_subject    IN VARCHAR2
 ,i_message    IN VARCHAR2
 );

Why I didn’t have i_sender be a simple address_type, I can’t remember. Internally, the procedure only looks at i_sender(1) – if a caller were to pass in a table of more than one sender, it raises an exception.

2. Functional programming to avoid local variables

Simple is best, and there’s nothing wrong with using local variables. I wish I’d realised these facts when I wrote functions like this:

FUNCTION address
 (i_name          IN VARCHAR2
 ,i_email_address IN VARCHAR2
 ) RETURN address_list_type;
FUNCTION address
 (i_address       IN address_list_type
 ,i_name          IN VARCHAR2
 ,i_email_address IN VARCHAR2
 ) RETURN address_list_type;

All that so that callers can avoid *one local variable*:

EMAIL_PKG.send
 (i_sender     => EMAIL_PKG.address('joe','joe@company.com')
 ,i_recipients => EMAIL_PKG.address(
                  EMAIL_PKG.address(
                  'jill', 'jill@company.com')
                 ,'bob', 'bob@company.com')
 ,i_subject    => 'hello'
 ,i_message    => 'world'
 );

See what I did there with the recipients? Populating an array on the fly with just function calls. Smart eh? But rather useless, as it turns out; when we need to send multiple recipients, it’s usually populated within a loop of unknown sized, so this method doesn’t work anyway.

Go ahead – face your past and dig up some code you wrote 5 years ago or more. I think, if you don’t go “WTF!” every now and then, you probably haven’t learned anything or improved yourself in the intervening years. Just saying 🙂


“It works, just not for the reason you think”

A somewhat mistaken attempt to format a number left-padded with up to six zeroes, made me cringe a little:

SELECT LPAD(number_column, 6, 000000) FROM ...

Yes, it does work (the domain of number_column is limited to positive integers), but not for the reason the writer thought, I think.

(Hint: what is the data type of the third parameter to LPAD?)


Negative, Captain

Seen in the wild:

... WHERE substr(amount,0, 1) != '-'

If you wanted to query a table of monetary transactions for any refunds (i.e. where the transaction amount is negative), how would you do it? Perhaps you’d think about avoiding problems that might occur if the default number format were to change, hm?

(before you say it: no, there is no index on amount, so it wasn’t a misguided attempt to avoid an index access path…)


Mood Swings

An interesting column comment encountered:

MOOD_SWINGS_IND VARCHAR2(1) (Y/N) “indicates that the person was in a swinging mood at the time of the episode”


Generic Audit

Looking at the column comments on this table, I can sympathise with the poor soul who after painstakingly describing 100 columns just decided that it just wasn’t worth it…

table audit_history (column comments in quotes):

id number(18) “Unique identifier”
aud_table_name varchar2(30) “audit record table”
audit_action varchar2(50) “audit action e.g. update”
aud_timestamp date “timestamp of the audit”
aud_user_id varchar2(100) “user performing the action”
aud_col_1 varchar2(4000) “audit column 1”
aud_col_2 varchar2(4000) “audit column 2”
aud_col_3 varchar2(4000) “audit column 3”
… (etc.) …
aud_col_99 varchar2(4000) “audit column 99”
aud_col_100 varchar2(4000) “audit column 100”
aud_col_101 varchar2(4000)
aud_col_102 varchar2(4000)
… (etc.) …
aud_col_139 varchar2(4000)
aud_col_140 varchar2(4000)

Tip: Don’t let anyone even think about using this kind of design for their change history auditing requirements. It might look elegant to them, but it is not good. Just, don’t.


Fun with copy-and-paste code

Came across this in a form (6i) to be run on a 9i db. Not only is this code about 33 lines of code too long and issues any number of unnecessary database queries, its name is quite unrelated to its intended function. Needless to say it was easily replaced with a single call to INSTR.

PROCEDURE alpha_check
(ref_in IN VARCHAR2
,ref_out OUT VARCHAR2) IS
-- Procedure included to distinguish
-- ref_in between ID or reference.
  l_alpha_char  VARCHAR2 (1);
  l_alpha_pos   NUMBER;
  l_found_pos   NUMBER;
  l_search_string VARCHAR2 (100) := ' ';
 

  CURSOR cur_get_next_alpha(N NUMBER) IS
  SELECT SUBSTR(l_search_string,N,1)
  FROM dual;

  CURSOR cur_check_for_alpha(C VARCHAR2)IS
  SELECT INSTRB(ref_in,C, 1)
  FROM dual;

BEGIN
  IF ref_in IS NULL THEN
    ref_out := 'X';
    RETURN;
  END IF;

  FOR I IN 1..LENGTH(l_search_string) LOOP
    OPEN cur_get_next_alpha(I);
    FETCH cur_get_next_alpha
    INTO l_alpha_char;
    CLOSE cur_get_next_alpha;

    FOR J IN 1..LENGTH(ref_in) LOOP
      OPEN cur_check_for_alpha(l_alpha_char);
      FETCH cur_check_for_alpha
      INTO l_found_pos;
      CLOSE cur_check_for_alpha;

      IF l_found_pos > 0 THEN
        ref_out := 'N';
        RETURN;
      END IF;
    END LOOP;
  END LOOP;

  ref_out := 'Y';
EXCEPTION
  WHEN OTHERS THEN
    pc_ref_out := 'X';
END;

Looks like it may have been copied from the same source as “As bad as it gets”.